From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWnxo-0003bp-Lq for qemu-devel@nongnu.org; Fri, 19 Feb 2016 11:31:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWnxn-0001at-Fk for qemu-devel@nongnu.org; Fri, 19 Feb 2016 11:31:28 -0500 References: <1455892784-11328-1-git-send-email-peter.maydell@linaro.org> <1455892784-11328-2-git-send-email-peter.maydell@linaro.org> From: Sergey Fedorov Message-ID: <56C74356.7040604@gmail.com> Date: Fri, 19 Feb 2016 19:31:18 +0300 MIME-Version: 1.0 In-Reply-To: <1455892784-11328-2-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: "Edgar E. Iglesias" , qemu-arm@nongnu.org, patches@linaro.org 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 > --- > 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 > 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,