Re: [PATCH v2 1/2] target/ppc: Implement the DEXCR and HDEXCR

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 12/9/22 11:43, Nicholas Miehlbradt wrote:
> Define the DEXCR and HDEXCR as special purpose registers.
> 
> Each register occupies two SPR indicies, one which can be read in an
> unprivileged state and one which can be modified in the appropriate
> priviliged state, however both indicies refer to the same underlying
> value.
> 
> Note that the ISA uses the abbreviation UDEXCR in two different
> contexts: the userspace DEXCR, the SPR index which can be read from
> userspace (implemented in this patch), and the ultravisor DEXCR, the
> equivalent register for the ultravisor state (not implemented).
> 
> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> ---
> v2: Clearing of upper 32 bits of DEXCR is now performed on read from
> problem state rather than on write in privileged state.
> ---
>   target/ppc/cpu.h        | 19 +++++++++++++++++++
>   target/ppc/cpu_init.c   | 25 +++++++++++++++++++++++++
>   target/ppc/spr_common.h |  1 +
>   target/ppc/translate.c  | 19 +++++++++++++++++++
>   4 files changed, 64 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 81d4263a07..0ed9f2ae35 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1068,6 +1068,21 @@ struct ppc_radix_page_info {
>       uint32_t entries[PPC_PAGE_SIZES_MAX_SZ];
>   };
>   
> +/*****************************************************************************/
> +/* Dynamic Execution Control Register */
> +
> +#define DEXCR_ASPECT(name, num)                    \
> +FIELD(DEXCR, PNH_##name, PPC_BIT_NR(num), 1)       \
> +FIELD(DEXCR, PRO_##name, PPC_BIT_NR(num + 32), 1)  \
> +FIELD(HDEXCR, HNU_##name, PPC_BIT_NR(num), 1)      \
> +FIELD(HDEXCR, ENF_##name, PPC_BIT_NR(num + 32), 1) \
> +
> +DEXCR_ASPECT(SBHE, 0)
> +DEXCR_ASPECT(IDRTPB, 1)
                 ^^^^^^ IBRTPD ?
> +DEXCR_ASPECT(SRAPD, 4)
> +DEXCR_ASPECT(NPHIE, 5)
> +DEXCR_ASPECT(PHIE, 6)
> +
>   /*****************************************************************************/
>   /* The whole PowerPC CPU context */
>   
> @@ -1674,9 +1689,11 @@ void ppc_compat_add_property(Object *obj, const char *name,
>   #define SPR_BOOKE_GIVOR13     (0x1BC)
>   #define SPR_BOOKE_GIVOR14     (0x1BD)
>   #define SPR_TIR               (0x1BE)
> +#define SPR_UHDEXCR           (0x1C7)
>   #define SPR_PTCR              (0x1D0)
>   #define SPR_HASHKEYR          (0x1D4)
>   #define SPR_HASHPKEYR         (0x1D5)
> +#define SPR_HDEXCR            (0x1D7)
>   #define SPR_BOOKE_SPEFSCR     (0x200)
>   #define SPR_Exxx_BBEAR        (0x201)
>   #define SPR_Exxx_BBTAR        (0x202)
> @@ -1865,8 +1882,10 @@ void ppc_compat_add_property(Object *obj, const char *name,
>   #define SPR_RCPU_L2U_RA2      (0x32A)
>   #define SPR_MPC_MD_DBRAM1     (0x32A)
>   #define SPR_RCPU_L2U_RA3      (0x32B)
> +#define SPR_UDEXCR            (0x32C)
>   #define SPR_TAR               (0x32F)
>   #define SPR_ASDR              (0x330)
> +#define SPR_DEXCR             (0x33C)
>   #define SPR_IC                (0x350)
>   #define SPR_VTB               (0x351)
>   #define SPR_MMCRC             (0x353)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index cbf0081374..6433f4fdfd 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5727,6 +5727,30 @@ static void register_power10_hash_sprs(CPUPPCState *env)
>               hashpkeyr_initial_value);
>   }
>   
> +static void register_power10_dexcr_sprs(CPUPPCState *env)
> +{
> +    spr_register(env, SPR_DEXCR, "DEXCR",
> +            SPR_NOACCESS, SPR_NOACCESS,
> +            &spr_read_generic, &spr_write_generic,
> +            0);
> +
> +    spr_register(env, SPR_UDEXCR, "DEXCR",
> +            &spr_read_dexcr_ureg, SPR_NOACCESS,
> +            &spr_read_dexcr_ureg, SPR_NOACCESS,
> +            0);
> +
> +    spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> +            SPR_NOACCESS, SPR_NOACCESS,
> +            SPR_NOACCESS, SPR_NOACCESS,
> +            &spr_read_generic, &spr_write_generic,
> +            0);
> +
> +    spr_register(env, SPR_UHDEXCR, "HDEXCR",
> +            &spr_read_dexcr_ureg, SPR_NOACCESS,
> +            &spr_read_dexcr_ureg, SPR_NOACCESS,
> +            0);
> +}
> +
>   /*
>    * Initialize PMU counter overflow timers for Power8 and
>    * newer Power chips when using TCG.
> @@ -6402,6 +6426,7 @@ static void init_proc_POWER10(CPUPPCState *env)
>       register_power8_rpr_sprs(env);
>       register_power9_mmu_sprs(env);
>       register_power10_hash_sprs(env);
> +    register_power10_dexcr_sprs(env);
>   
>       /* FIXME: Filter fields properly based on privilege level */
>       spr_register_kvm_hv(env, SPR_PSSCR, "PSSCR", NULL, NULL, NULL, NULL,
> diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h
> index b5a5bc6895..91a74cec0f 100644
> --- a/target/ppc/spr_common.h
> +++ b/target/ppc/spr_common.h
> @@ -195,6 +195,7 @@ void spr_read_ebb_upper32(DisasContext *ctx, int gprn, int sprn);
>   void spr_write_ebb_upper32(DisasContext *ctx, int sprn, int gprn);
>   void spr_write_hmer(DisasContext *ctx, int sprn, int gprn);
>   void spr_write_lpcr(DisasContext *ctx, int sprn, int gprn);
> +void spr_read_dexcr_ureg(DisasContext *ctx, int sprn, int gprn);

Order of sprn, gprn appears different in funcn defn below, need to 
correct at either place.

>   #endif
>   
>   void register_low_BATs(CPUPPCState *env);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 19c1d17cb0..fcb1180712 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1249,6 +1249,25 @@ void spr_write_ebb_upper32(DisasContext *ctx, int sprn, int gprn)
>       gen_fscr_facility_check(ctx, SPR_FSCR, FSCR_EBB, sprn, FSCR_IC_EBB);
>       spr_write_prev_upper32(ctx, sprn, gprn);
>   }
> +
> +void spr_read_dexcr_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * Access to the (H)DEXCR in problem state is done using seperate
> +     * SPR indexes which are 16 below the SPR indexes which have full
> +     * access to the (H)DEXCR in privileged state. Problem state may
s/may/can ?
> +     * only read bits 32:63, bits 0:31 return 0.
> +     *
> +     * See section 9.3.1-9.3.2 of PowerISA v3.1B
> +     */
> +
> +    gen_load_spr(t0, sprn + 16);
> +    tcg_gen_ext32u_tl(cpu_gpr[gprn], t0);
> +
> +    tcg_temp_free(t0);
> +}
>   #endif
>   
>   #define GEN_HANDLER(name, opc1, opc2, opc3, inval, type)                      \

Otherwise, looks good to me!