| Message ID | 1455892784-11328-2-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <peter.maydell@linaro.org> > --- > 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 <serge.fdrv@gmai.com> > 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,
On 19 February 2016 at 16:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 19.02.2016 17:39, Peter Maydell wrote: >> +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. Yes, as you say we could do either (and we have examples of both in QEMU currently, as well as examples of "we don't do either and if the guest writes in a bit it should not it will get a feature it shouldn't in theory have"). Masking on write seemed simpler here. thanks -- PMM
On Fri, Feb 19, 2016 at 6:39 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> Acked-by: Alistair Francis <alistair.francis@xilinx.com> Thanks, Alistair > --- > 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; +} + 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,
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 <peter.maydell@linaro.org> --- target-arm/cpu.h | 4 ++++ target-arm/helper.c | 23 +++++++++++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-)