Re: [PATCH] ppc/pnv: Add PowerPC Special Purpose Registers

["Nicholas Piggin" <npiggin@gmail.com>]

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 <dantan@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