From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37378) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJl71-0006OT-BG for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:14:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJl6w-0004V4-7y for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:14:15 -0400 Received: from mail-la0-f47.google.com ([209.85.215.47]:58470) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJl6v-0004Uv-T5 for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:14:10 -0400 Received: by mail-la0-f47.google.com with SMTP id mc6so5972867lab.20 for ; Tue, 19 Aug 2014 08:14:08 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1408354830-1143-3-git-send-email-edgar.iglesias@gmail.com> References: <1408354830-1143-1-git-send-email-edgar.iglesias@gmail.com> <1408354830-1143-3-git-send-email-edgar.iglesias@gmail.com> From: Peter Maydell Date: Tue, 19 Aug 2014 16:13:48 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v5 02/10] target-arm: Add SCR_EL3 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Rob Herring , Peter Crosthwaite , Fabian Aggeler , QEMU Developers , Alexander Graf , Blue Swirl , Greg Bellows , Paolo Bonzini , =?UTF-8?B?QWxleCBCZW5uw6ll?= , Christoffer Dall , Richard Henderson On 18 August 2014 10:40, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Signed-off-by: Edgar E. Iglesias > --- > target-arm/cpu.h | 17 ++++++++++++++++- > target-arm/helper.c | 35 +++++++++++++++++++++++++++++++++-- > 2 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 8859b94..524eb90 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -172,7 +172,6 @@ typedef struct CPUARMState { > uint64_t c1_sys; /* System control register. */ > uint64_t c1_coproc; /* Coprocessor access register. */ > uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ > - uint32_t c1_scr; /* secure config register. */ > uint64_t ttbr0_el1; /* MMU translation table base 0. */ > uint64_t ttbr1_el1; /* MMU translation table base 1. */ > uint64_t c2_control; /* MMU translation table base control. */ > @@ -185,6 +184,7 @@ typedef struct CPUARMState { > uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */ > uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */ > uint64_t hcr_el2; /* Hypervisor configuration register */ > + uint32_t scr_el3; /* Secure configuration register. */ This is a uint32_t but you refer to it below with fieldoffset() in an AArch64 reginfo -- it has to be a uint64_t for that. > uint32_t ifsr_el2; /* Fault status registers. */ > uint64_t esr_el[4]; > uint32_t c6_region[8]; /* MPU base/size registers. */ > @@ -578,6 +578,21 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask) > #define HCR_ID (1ULL << 33) > #define HCR_MASK ((1ULL << 34) - 1) > > +#define SCR_NS (1U << 0) > +#define SCR_IRQ (1U << 1) > +#define SCR_FIQ (1U << 2) > +#define SCR_EA (1U << 3) > +#define SCR_SMD (1U << 7) > +#define SCR_HCE (1U << 8) > +#define SCR_SIF (1U << 9) > +#define SCR_RW (1U << 10) > +#define SCR_ST (1U << 11) > +#define SCR_TWI (1U << 12) > +#define SCR_TWE (1U << 13) > +#define SCR_AARCH64_RES1_MASK (3U << 4) > +#define SCR_AARCH32_MASK (0x3fff & ~(3U << 10)) > +#define SCR_AARCH64_MASK (0x3fff & ~(1U << 6)) I find these masks rather confusing to read... > + > /* Return the current FPSCR value. */ > uint32_t vfp_get_fpscr(CPUARMState *env); > void vfp_set_fpscr(CPUARMState *env, uint32_t val); > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 1021812..59144cd 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -684,6 +684,32 @@ static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, > raw_write(env, ri, value & ~0x1FULL); > } > > +static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > +{ > + uint32_t valid_mask = is_a64(env) ? SCR_AARCH64_MASK : SCR_AARCH32_MASK; > + uint32_t res1_mask = is_a64(env) ? SCR_AARCH64_RES1_MASK : 0; I don't think this is valid by the definition of RES0/RES1. We're basically implementing SCR and SCR_EL3 as aliased to each other. That means that bit 6 is RES0 for AArch64 but has a meaning in AArch32, which puts it into the "RES0 only in some contexts" category. That says "a direct write to the bit must update a storage location associated with the bit" -- we can't mask it out here. thanks -- PMM