On Thu, 18 Jan 2024 12:27:12 +1000, Nicholas Piggin wrote:
> On Thu Jan 18, 2024 at 8:34 AM AEST, dan tan wrote:
>> The handling of the following two registers are added -
>> DAWR1 (0x0bd, 189) - Data Address Watchpoint 1
>> DAWRX1 (0x0b5, 181) - Data Address Watchpoint Extension 1
>>
>> Signed-off-by: dan tan <dan...@linux.vnet.ibm.com>
>
> Small nit, but there's some extra whitespace on the left here and in
> Subject header which is normally not required.
>
I will fix that on the respin
>> ---
>> target/ppc/cpu.c | 51
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> target/ppc/cpu.h | 6 ++++++
>> target/ppc/cpu_init.c | 10 ++++++++++
>> target/ppc/excp_helper.c | 11 ++++++++++-
>> target/ppc/helper.h | 2 ++
>> target/ppc/machine.c | 1 +
>> target/ppc/misc_helper.c | 10 ++++++++++
>> target/ppc/spr_common.h | 2 ++
>> target/ppc/translate.c | 12 ++++++++++++
>> 9 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
>> index e3ad8e0..8a77328 100644
>> --- a/target/ppc/cpu.c
>> +++ b/target/ppc/cpu.c
>> @@ -188,6 +188,57 @@ void ppc_store_dawrx0(CPUPPCState *env, uint32_t val)
>> env->spr[SPR_DAWRX0] = val;
>> ppc_update_daw0(env);
>> }
>> +
>> +void ppc_update_daw1(CPUPPCState *env)
>> +{
>> + CPUState *cs = env_cpu(env);
>> + target_ulong deaw = env->spr[SPR_DAWR1] & PPC_BITMASK(0, 60);
>> + uint32_t dawrx = env->spr[SPR_DAWRX1];
>> + int mrd = extract32(dawrx, PPC_BIT_NR(48), 54 - 48);
>> + bool dw = extract32(dawrx, PPC_BIT_NR(57), 1);
>> + bool dr = extract32(dawrx, PPC_BIT_NR(58), 1);
>> + bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
>> + bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
>> + bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
>> + vaddr len;
>> + int flags;
>> +
>> + if (env->dawr1_watchpoint) {
>> + cpu_watchpoint_remove_by_ref(cs, env->dawr1_watchpoint);
>> + env->dawr1_watchpoint = NULL;
>> + }
>> +
>> + if (!dr && !dw) {
>> + return;
>> + }
>> +
>> + if (!hv && !sv && !pr) {
>> + return;
>> + }
>> +
>> + len = (mrd + 1) * 8;
>> + flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
>> + if (dr) {
>> + flags |= BP_MEM_READ;
>> + }
>> + if (dw) {
>> + flags |= BP_MEM_WRITE;
>> + }
>> +
>> + cpu_watchpoint_insert(cs, deaw, len, flags, &env->dawr1_watchpoint);
>> +}
>
> I would say this is just beyond the point where we should share
> code with daw0. You could make a function that takes DAWR(x) SPR
> numbers or values, and a pointer to the watchpoint to use.
>
Noted. Will make the change
>> +
>> +void ppc_store_dawr1(CPUPPCState *env, target_ulong val)
>> +{
>> + env->spr[SPR_DAWR1] = val;
>> + ppc_update_daw1(env);
>> +}
>> +
>> +void ppc_store_dawrx1(CPUPPCState *env, uint32_t val)
>> +{
>> + env->spr[SPR_DAWRX1] = val;
>> + ppc_update_daw1(env);
>> +}
>> #endif
>> #endif
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index f8101ff..ab34fc7 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1237,6 +1237,7 @@ struct CPUArchState {
>> ppc_slb_t slb[MAX_SLB_ENTRIES]; /* PowerPC 64 SLB area */
>> struct CPUBreakpoint *ciabr_breakpoint;
>> struct CPUWatchpoint *dawr0_watchpoint;
>> + struct CPUWatchpoint *dawr1_watchpoint;
>> #endif
>> target_ulong sr[32]; /* segment registers */
>> uint32_t nb_BATs; /* number of BATs */
>> @@ -1552,6 +1553,9 @@ void ppc_store_ciabr(CPUPPCState *env, target_ulong
>> value);
>> void ppc_update_daw0(CPUPPCState *env);
>> void ppc_store_dawr0(CPUPPCState *env, target_ulong value);
>> void ppc_store_dawrx0(CPUPPCState *env, uint32_t value);
>> +void ppc_update_daw1(CPUPPCState *env);
>> +void ppc_store_dawr1(CPUPPCState *env, target_ulong value);
>> +void ppc_store_dawrx1(CPUPPCState *env, uint32_t value);
>> #endif /* !defined(CONFIG_USER_ONLY) */
>> void ppc_store_msr(CPUPPCState *env, target_ulong value);
>>
>> @@ -1737,9 +1741,11 @@ void ppc_compat_add_property(Object *obj, const char
>> *name,
>> #define SPR_PSPB (0x09F)
>> #define SPR_DPDES (0x0B0)
>> #define SPR_DAWR0 (0x0B4)
>> +#define SPR_DAWR1 (0x0B5)
>> #define SPR_RPR (0x0BA)
>> #define SPR_CIABR (0x0BB)
>> #define SPR_DAWRX0 (0x0BC)
>> +#define SPR_DAWRX1 (0x0BD)
>> #define SPR_HFSCR (0x0BE)
>> #define SPR_VRSAVE (0x100)
>> #define SPR_USPRG0 (0x100)
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 40fe14a..d75c359 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -5119,11 +5119,21 @@ static void register_book3s_207_dbg_sprs(CPUPPCState
>> *env)
>> SPR_NOACCESS, SPR_NOACCESS,
>> &spr_read_generic, &spr_write_dawr0,
>> KVM_REG_PPC_DAWR, 0x00000000);
>> + spr_register_kvm_hv(env, SPR_DAWR1, "DAWR1",
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + &spr_read_generic, &spr_write_dawr1,
>> + KVM_REG_PPC_DAWR, 0x00000000);
>> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
>> SPR_NOACCESS, SPR_NOACCESS,
>> SPR_NOACCESS, SPR_NOACCESS,
>> &spr_read_generic, &spr_write_dawrx0,
>> KVM_REG_PPC_DAWRX, 0x00000000);
>> + spr_register_kvm_hv(env, SPR_DAWRX1, "DAWRX1",
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + &spr_read_generic, &spr_write_dawrx1,
>> + KVM_REG_PPC_DAWRX, 0x00000000);
>
> These are new for POWER10, no? This is adding them for P8/9 too.
>
I did not realize that. Will move these out to POWER10 only
> Thanks,
> Nick
Thanks,
/dan
