| Message ID | 1412113785-21525-13-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > Prepare for cp register banking by inserting every cp register twice, > once for secure world and once for non-secure world. > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > ---------- > v4 -> v5 > - Added use of ARM CP secure/non-secure bank flags during register processing > in define_one_arm_cp_reg_with_opaque(). We now only register the specified > bank if only one flag is specified, otherwise we register both a secure and > non-secure instance. > --- > target-arm/cpu.h | 14 +++++++++++--- > target-arm/helper.c | 30 ++++++++++++++++++++++++++---- > target-arm/translate.c | 14 +++++++++----- > 3 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 9681d45..220571c 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -864,6 +864,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq); > * Crn, Crm, opc1, opc2 fields > * 32 or 64 bit register (ie is it accessed via MRC/MCR > * or via MRRC/MCRR?) > + * non-secure/secure bank (Aarch32 only) ...so if this is an AArch64 register is the bit in the hash table key set or clear? > * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field. > * (In this case crn and opc2 should be zero.) > * For AArch64, there is no 32/64 bit size distinction; > @@ -881,9 +882,16 @@ void armv7m_nvic_complete_irq(void *opaque, int irq); > #define CP_REG_AA64_SHIFT 28 > #define CP_REG_AA64_MASK (1 << CP_REG_AA64_SHIFT) > > -#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2) \ > - (((cp) << 16) | ((is64) << 15) | ((crn) << 11) | \ > - ((crm) << 7) | ((opc1) << 3) | (opc2)) > +/* To enable banking of coprocessor registers depending on ns-bit we > + * add a bit to distinguish between secure and non-secure cpregs in the > + * hashtable. > + */ > +#define CP_REG_NS_SHIFT 27 > +#define CP_REG_NS_MASK(nsbit) (nsbit << CP_REG_NS_SHIFT) Bit 27 is already used, as part of the COPROC field. There's a reason the AA64 bit is 28... > + > +#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2, ns) \ > + (CP_REG_NS_MASK(ns) | ((cp) << 16) | ((is64) << 15) | \ > + ((crn) << 11) | ((crm) << 7) | ((opc1) << 3) | (opc2)) Doesn't this break KVM's accessing of the hashtable? You probably need to make kvm_to_cpreg_id OR in the NS bit as appropriate, and cpreg_to_kvm_id mask it out, since if we're using KVM then we're always non-secure. thanks -- PMM
On 6 October 2014 11:25, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > Prepare for cp register banking by inserting every cp register twice, > > once for secure world and once for non-secure world. > > > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > ---------- > > v4 -> v5 > > - Added use of ARM CP secure/non-secure bank flags during register > processing > > in define_one_arm_cp_reg_with_opaque(). We now only register the > specified > > bank if only one flag is specified, otherwise we register both a > secure and > > non-secure instance. > > --- > > target-arm/cpu.h | 14 +++++++++++--- > > target-arm/helper.c | 30 ++++++++++++++++++++++++++---- > > target-arm/translate.c | 14 +++++++++----- > > 3 files changed, 46 insertions(+), 12 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 9681d45..220571c 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -864,6 +864,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq); > > * Crn, Crm, opc1, opc2 fields > > * 32 or 64 bit register (ie is it accessed via MRC/MCR > > * or via MRRC/MCRR?) > > + * non-secure/secure bank (Aarch32 only) > > ...so if this is an AArch64 register is the bit in the hash > table key set or clear? > The 64-bit hash does not include a S/NS bit, so it defaults to 0. I'll add some additional comments to v6. > > > * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field. > > * (In this case crn and opc2 should be zero.) > > * For AArch64, there is no 32/64 bit size distinction; > > @@ -881,9 +882,16 @@ void armv7m_nvic_complete_irq(void *opaque, int > irq); > > #define CP_REG_AA64_SHIFT 28 > > #define CP_REG_AA64_MASK (1 << CP_REG_AA64_SHIFT) > > > > -#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2) \ > > - (((cp) << 16) | ((is64) << 15) | ((crn) << 11) | \ > > - ((crm) << 7) | ((opc1) << 3) | (opc2)) > > +/* To enable banking of coprocessor registers depending on ns-bit we > > + * add a bit to distinguish between secure and non-secure cpregs in the > > + * hashtable. > > + */ > > +#define CP_REG_NS_SHIFT 27 > > +#define CP_REG_NS_MASK(nsbit) (nsbit << CP_REG_NS_SHIFT) > > Bit 27 is already used, as part of the COPROC field. > There's a reason the AA64 bit is 28... > Yes, good catch. I think I originally had it as bit 31. but went with what Fabian had. I will bump it up for v6. > > > + > > +#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2, ns) \ > > + (CP_REG_NS_MASK(ns) | ((cp) << 16) | ((is64) << 15) | \ > > + ((crn) << 11) | ((crm) << 7) | ((opc1) << 3) | (opc2)) > > Doesn't this break KVM's accessing of the hashtable? > You probably need to make kvm_to_cpreg_id OR in the NS > bit as appropriate, and cpreg_to_kvm_id mask it out, since > if we're using KVM then we're always non-secure. > > Not sure if it breaks KVM, I'll have to look into it. > thanks > -- PMM >
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 9681d45..220571c 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -864,6 +864,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq); * Crn, Crm, opc1, opc2 fields * 32 or 64 bit register (ie is it accessed via MRC/MCR * or via MRRC/MCRR?) + * non-secure/secure bank (Aarch32 only) * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field. * (In this case crn and opc2 should be zero.) * For AArch64, there is no 32/64 bit size distinction; @@ -881,9 +882,16 @@ void armv7m_nvic_complete_irq(void *opaque, int irq); #define CP_REG_AA64_SHIFT 28 #define CP_REG_AA64_MASK (1 << CP_REG_AA64_SHIFT) -#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2) \ - (((cp) << 16) | ((is64) << 15) | ((crn) << 11) | \ - ((crm) << 7) | ((opc1) << 3) | (opc2)) +/* To enable banking of coprocessor registers depending on ns-bit we + * add a bit to distinguish between secure and non-secure cpregs in the + * hashtable. + */ +#define CP_REG_NS_SHIFT 27 +#define CP_REG_NS_MASK(nsbit) (nsbit << CP_REG_NS_SHIFT) + +#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2, ns) \ + (CP_REG_NS_MASK(ns) | ((cp) << 16) | ((is64) << 15) | \ + ((crn) << 11) | ((crm) << 7) | ((opc1) << 3) | (opc2)) #define ENCODE_AA64_CP_REG(cp, crn, crm, op0, op1, op2) \ (CP_REG_AA64_MASK | \ diff --git a/target-arm/helper.c b/target-arm/helper.c index ab38b68..a0f53dd 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3288,7 +3288,7 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp) static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, void *opaque, int state, - int crm, int opc1, int opc2) + int crm, int opc1, int opc2, int nsbit) { /* Private utility function for define_one_arm_cp_reg_with_opaque(): * add a single reginfo struct to the hash table. @@ -3361,7 +3361,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, *key = ENCODE_AA64_CP_REG(r2->cp, r2->crn, crm, r2->opc0, opc1, opc2); } else { - *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2); + *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2, nsbit); } if (opaque) { r2->opaque = opaque; @@ -3510,8 +3510,30 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, if (r->state != state && r->state != ARM_CP_STATE_BOTH) { continue; } - add_cpreg_to_hashtable(cpu, r, opaque, state, - crm, opc1, opc2); + if (state == ARM_CP_STATE_AA32) { + /* Under Aarch32 CP registers can be common + * (same for secure and non-secure world) or banked. + */ + uint32_t s = + r->type & (ARM_CP_BANK_S | ARM_CP_BANK_NS); + if (ARM_CP_BANK_S == s) { + add_cpreg_to_hashtable(cpu, r, opaque, state, + crm, opc1, opc2, !SCR_NS); + } else if (ARM_CP_BANK_NS == s) { + add_cpreg_to_hashtable(cpu, r, opaque, state, + crm, opc1, opc2, SCR_NS); + } else { + add_cpreg_to_hashtable(cpu, r, opaque, state, + crm, opc1, opc2, !SCR_NS); + add_cpreg_to_hashtable(cpu, r, opaque, state, + crm, opc1, opc2, SCR_NS); + } + } else { + /* Aarch64 registers get mapped to non-secure instance + * of Aarch32 */ + add_cpreg_to_hashtable(cpu, r, opaque, state, + crm, opc1, opc2, SCR_NS); + } } } } diff --git a/target-arm/translate.c b/target-arm/translate.c index 5e1d677..7720ed8 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -7038,7 +7038,7 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn) rt = (insn >> 12) & 0xf; ri = get_arm_cp_reginfo(s->cp_regs, - ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2)); + ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2, s->ns)); if (ri) { /* Check access permissions */ if (!cp_access_ok(s->current_pl, ri, isread)) { @@ -7228,12 +7228,16 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn) */ if (is64) { qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 " - "64 bit system register cp:%d opc1: %d crm:%d\n", - isread ? "read" : "write", cpnum, opc1, crm); + "64 bit system register cp:%d opc1: %d crm:%d " + "(%s)\n", + isread ? "read" : "write", cpnum, opc1, crm, + s->ns ? "non-secure" : "secure"); } else { qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 " - "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d\n", - isread ? "read" : "write", cpnum, opc1, crn, crm, opc2); + "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d " + "(%s)\n", + isread ? "read" : "write", cpnum, opc1, crn, crm, opc2, + s->ns ? "non-secure" : "secure"); } return 1;