Re: [Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code

[Sergey Fedorov]

On 19.02.2016 17:39, Peter Maydell wrote:
> Fix two issues with our implementation of the SDCR:
>  * it is only present from ARMv8 onwards
>  * it does not contain several of the trap bits present in its 64-bit
>    counterpart the MDCR_EL3
>
> Put the register description in the right place so that it does not
> get enabled for ARMv7 and earlier, and give it a write function so that
> we can mask out the bits which should not be allowed to have an effect
> if EL3 is 32-bit.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/cpu.h    |  4 ++++
>  target-arm/helper.c | 23 +++++++++++++++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index e72e33b..a729ae0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -599,6 +599,7 @@ void pmccntr_sync(CPUARMState *env);
>  #define MDCR_EDAD     (1U << 20)
>  #define MDCR_SPME     (1U << 17)
>  #define MDCR_SDD      (1U << 16)
> +#define MDCR_SPD      (3U << 14)
>  #define MDCR_TDRA     (1U << 11)
>  #define MDCR_TDOSA    (1U << 10)
>  #define MDCR_TDA      (1U << 9)
> @@ -607,6 +608,9 @@ void pmccntr_sync(CPUARMState *env);
>  #define MDCR_TPM      (1U << 6)
>  #define MDCR_TPMCR    (1U << 5)
>  
> +/* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */
> +#define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
> +
>  #define CPSR_M (0x1fU)
>  #define CPSR_T (1U << 5)
>  #define CPSR_F (1U << 6)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3d7fda1..e9b89e6 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3037,6 +3037,12 @@ static CPAccessResult fpexc32_access(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> +static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                       uint64_t value)
> +{
> +    env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
> +}
> +

Just one comment. As soon as we cannot have both of MDCR_EL3 in SDCR in
a specific CPU configuration (EL3 is either AArch64 or AArch32), the
RES0 bitfields of SDCR are "RES0 in all contexts". Thus we can choose
"The bit is hardwired to 0" behaviour as we do here. We could also
choose another behaviour "The bit can be written" and check for "EL3 is
AArch64" case before trying to interpret those bits. Anyway:

Reviewed-by: Sergey Fedorov <address@hidden>

>  static const ARMCPRegInfo v8_cp_reginfo[] = {
>      /* Minimal set of EL0-visible registers. This will need to be expanded
>       * significantly for system emulation of AArch64 CPUs.
> @@ -3331,6 +3337,15 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 3, .opc2 = 3,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_FIQ]) },
> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> +      .resetvalue = 0,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) 
> },
> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .writefn = sdcr_write,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      REGINFO_SENTINEL
>  };
>  
> @@ -3688,14 +3703,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
> -    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> -      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> -      .resetvalue = 0,
> -      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) 
> },
> -    { .name = "SDCR", .type = ARM_CP_ALIAS,
> -      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> -      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> -      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,