Re: [PATCH 11/16] target/arm: Add minimal RAS registers

[Peter Maydell]

On Sat, 9 Apr 2022 at 01:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Add only the system registers required to implement zero error
> records.  This means we need to save state for ERRSELR, but all
> values are out of range, so none of the indexed error record
> registers need be implemented.
>
> Add the EL2 registers required for injecting virtual SError.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> +/*
> + * Minimal RAS implementation with no Error Records.
> + * Which means that all of the Error Record registers:
> + *   ERXADDR_EL1
> + *   ERXCTLR_EL1
> + *   ERXFR_EL1
> + *   ERXMISC0_EL1
> + *   ERXMISC1_EL1
> + *   ERXMISC2_EL1
> + *   ERXMISC3_EL1
> + *   ERXPFGCDN_EL1  (RASv1p1)
> + *   ERXPFGCTL_EL1  (RASv1p1)
> + *   ERXPFGF_EL1    (RASv1p1)
> + *   ERXSTATUS_EL1
> + * may generate UNDEFINED, which is the effect we get by not
> + * listing them at all.
> + */
> +static const ARMCPRegInfo minimal_ras_reginfo_el1[] = {
> +    { .name = "DISR_EL1", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 0, .crn = 0xc, .crm = 1, .opc2 = 1,

".crn = 12", please -- no other reginfo struct uses hex here.
Similarly below.

> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.disr_el1),
> +      .readfn = disr_read, .writefn = disr_write, .raw_writefn = raw_write },
> +    { .name = "ERRIDR_EL1", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 0, .crn = 5, .crm = 3, .opc2 = 0,
> +      .access = PL1_R, .accessfn = access_terr,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "ERRSELR_EL1", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 0, .crn = 5, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_terr,
> +      .fieldoffset = offsetof(CPUARMState, cp15.errselr_el1) },

By my reading of the spec we could make ERRSELR_EL1 RAZ/WI, because
writing an over-large number has a number of behaviours including
that the value the guest can read back is UNKNOWN. That would save
having the CPU state struct field.

> +    REGINFO_SENTINEL
> +};
> +
> +static const ARMCPRegInfo minimal_ras_reginfo_el2[] = {
> +    { .name = "VDISR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 0xc, .crm = 1, .opc2 = 1,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.vdisr_el2) 
> },
> +    { .name = "VSESR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 3,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.vsesr_el2) 
> },
> +    REGINFO_SENTINEL
> +};
> +
> +static const ARMCPRegInfo minimal_ras_reginfo_no_el2[] = {
> +    { .name = "VDISR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 0xc, .crm = 1, .opc2 = 1,
> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "VSESR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 3,
> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    REGINFO_SENTINEL
> +};
> +
>  /* Return the exception level to which exceptions should be taken
>   * via SVEAccessTrap.  If an exception should be routed through
>   * AArch64.AdvSIMDFPAccessTrap, return 0; fp_exception_el should
> @@ -8452,6 +8550,15 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_one_arm_cp_reg(cpu, &ssbs_reginfo);
>      }
>
> +    if (cpu_isar_feature(any_ras, cpu)) {
> +        define_arm_cp_regs(cpu, minimal_ras_reginfo_el1);
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            define_arm_cp_regs(cpu, minimal_ras_reginfo_el2);
> +        } else {
> +            define_arm_cp_regs(cpu, minimal_ras_reginfo_no_el2);
> +        }
> +    }
> +
>      if (cpu_isar_feature(aa64_vh, cpu) ||
>          cpu_isar_feature(aa64_debugv8p2, cpu)) {
>          if (arm_feature(env, ARM_FEATURE_EL2)) {
> --
> 2.25.1

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM