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. > --- > 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. > + > +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. Thanks, Nick