From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48139) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPRZM-0003FY-Hy for qemu-devel@nongnu.org; Mon, 17 Mar 2014 03:02:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPRZE-0006Zw-Rz for qemu-devel@nongnu.org; Mon, 17 Mar 2014 03:02:44 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:61254) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPRZE-0006Zm-JG for qemu-devel@nongnu.org; Mon, 17 Mar 2014 03:02:36 -0400 Received: by mail-wi0-f181.google.com with SMTP id hm4so1683988wib.2 for ; Mon, 17 Mar 2014 00:02:35 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <1394134385-1727-17-git-send-email-peter.maydell@linaro.org> References: <1394134385-1727-1-git-send-email-peter.maydell@linaro.org> <1394134385-1727-17-git-send-email-peter.maydell@linaro.org> Date: Mon, 17 Mar 2014 17:02:35 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH v4 16/21] target-arm: Implement SP_EL0, SP_EL1 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Rob Herring , Patch Tracking , Michael Matz , Claudio Fontana , Alexander Graf , "qemu-devel@nongnu.org Developers" , Laurent Desnogues , Dirk Mueller , Will Newton , =?ISO-8859-1?Q?Alex_Benn=E9e?= , "kvmarm@lists.cs.columbia.edu" , Christoffer Dall , Richard Henderson On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell wrote: > Implement handling for the AArch64 SP_EL0 system register. > This holds the EL0 stack pointer, and is only accessible when > it's not being used as the stack pointer, ie when we're in EL1 > and EL1 is using its own stack pointer. We also provide a > definition of the SP_EL1 register; this isn't guest visible > as a system register for an implementation like QEMU which > doesn't provide EL2 or EL3; however it is useful for ensuring > the underlying state is migrated. > > We need to update the state fields in the CPU state whenever "whenever we". > switch stack pointers; this happens when we take an exception > and also when SPSEL is used to change the bit in PSTATE which > indicates which stack pointer EL1 should use. > > Signed-off-by: Peter Maydell > --- > target-arm/cpu.h | 2 ++ > target-arm/helper.c | 34 ++++++++++++++++++++++++++++++++++ > target-arm/internals.h | 25 +++++++++++++++++++++++++ > target-arm/kvm64.c | 37 ++++++++++++++++++++++++++++++++++--- > target-arm/machine.c | 7 ++++--- > target-arm/op_helper.c | 2 +- > 6 files changed, 100 insertions(+), 7 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 7ef2c71..338edc3 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -163,6 +163,7 @@ typedef struct CPUARMState { > uint64_t daif; /* exception masks, in the bits they are in in PSTATE */ > > uint64_t elr_el1; /* AArch64 ELR_EL1 */ > + uint64_t sp_el[2]; /* AArch64 banked stack pointers */ > Should the macro AARCH64_MAX_EL_LEVELS exist for this? > /* System control coprocessor (cp15) */ > struct { > @@ -431,6 +432,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address, int rw, > * Only these are valid when in AArch64 mode; in > * AArch32 mode SPSRs are basically CPSR-format. > */ > +#define PSTATE_SP (1U) > #define PSTATE_M (0xFU) > #define PSTATE_nRW (1U << 4) > #define PSTATE_F (1U << 6) > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 812fc73..6ee4135 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1682,6 +1682,27 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri) > return cpu->dcz_blocksize | dzp_bit; > } > > +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + if (!env->pstate & PSTATE_SP) { > + /* Access to SP_EL0 is undefined if it's being used as > + * the stack pointer. > + */ > + return CP_ACCESS_TRAP_UNCATEGORIZED; > + } > + return CP_ACCESS_OK; > +} > + > +static uint64_t spsel_read(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + return env->pstate & PSTATE_SP; > +} > + > +static void spsel_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t val) > +{ > + update_spsel(env, val); > +} > + > 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. > @@ -1814,6 +1835,19 @@ static const ARMCPRegInfo v8_cp_reginfo[] = { > .type = ARM_CP_NO_MIGRATE, > .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 1, > .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, elr_el1) }, > + /* We rely on the access checks not allowing the guest to write to the > + * state field when SPSel indicates that it's being used as the stack > + * pointer. > + */ > + { .name = "SP_EL0", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 1, .opc2 = 0, > + .access = PL1_RW, .accessfn = sp_el0_access, > + .type = ARM_CP_NO_MIGRATE, > + .fieldoffset = offsetof(CPUARMState, sp_el[0]) }, > + { .name = "SPSel", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 0, > + .type = ARM_CP_NO_MIGRATE, > + .access = PL1_RW, .readfn = spsel_read, .writefn = spsel_write }, > REGINFO_SENTINEL > }; > > diff --git a/target-arm/internals.h b/target-arm/internals.h > index 11a7040..97a76c2 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -60,6 +60,31 @@ enum arm_fprounding { > > int arm_rmode_to_sf(int rmode); > > +static inline void update_spsel(CPUARMState *env, uint32_t imm) > +{ > + /* Update PSTATE SPSel bit; this requires us to update the > + * working stack pointer in xregs[31]. > + */ > + if (!((imm ^ env->pstate) & PSTATE_SP)) { > + return; > + } > + env->pstate = deposit32(env->pstate, 0, 1, imm); > + > + /* EL0 has no access rights to update SPSel, and this code > + * assumes we are updating SP for EL1 while running as EL1. > + */ > + assert(arm_current_pl(env) == 1); > + if (env->pstate & PSTATE_SP) { > + /* Switch from using SP_EL0 to using SP_ELx */ > + env->sp_el[0] = env->xregs[31]; Does this break on repeated writes to spsel bit of the same value? E.g. Lets say I have the sequence (with spsel = 0 initially): spsel = 1; /* Via MSR SPSel, ; */ /* do some stuff that changes current SP */ spsel = 1; /* again */ The first write will sync sp_el[0] fine. But the second one will take the new version xregs[31] and save to sp_el[0] again. A subsequent return to use of sp_el[0] expecting the original saved value would then break. Can probably be solved by doing to save offs before ->pstate update (based on the old value) and the loads after. > + env->xregs[31] = env->sp_el[1]; > + } else { > + /* Switch from SP_EL0 to SP_ELx */ > + env->sp_el[1] = env->xregs[31]; > + env->xregs[31] = env->sp_el[0]; > + } > +} > + > /* Valid Syndrome Register EC field values */ > enum arm_exception_class { > EC_UNCATEGORIZED = 0, > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index ee72748..39c4364 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -121,8 +121,24 @@ int kvm_arch_put_registers(CPUState *cs, int level) > } > } > > + /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the > + * QEMU side we keep the current SP in xregs[31] as well. > + */ > + if (env->pstate & PSTATE_SP) { > + env->sp_el[1] = env->xregs[31]; > + } else { > + env->sp_el[0] = env->xregs[31]; > + } > + > reg.id = AARCH64_CORE_REG(regs.sp); > - reg.addr = (uintptr_t) &env->xregs[31]; > + reg.addr = (uintptr_t) &env->sp_el[0]; > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > + reg.id = AARCH64_CORE_REG(sp_el1); > + reg.addr = (uintptr_t) &env->sp_el[1]; > ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > if (ret) { > return ret; > @@ -152,7 +168,6 @@ int kvm_arch_put_registers(CPUState *cs, int level) > } > > /* TODO: > - * SP_EL1 > * SPSR[] > * FP state > * system registers > @@ -180,7 +195,14 @@ int kvm_arch_get_registers(CPUState *cs) > } > > reg.id = AARCH64_CORE_REG(regs.sp); > - reg.addr = (uintptr_t) &env->xregs[31]; > + reg.addr = (uintptr_t) &env->sp_el[0]; > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > + reg.id = AARCH64_CORE_REG(sp_el1); > + reg.addr = (uintptr_t) &env->sp_el[1]; > ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > if (ret) { > return ret; > @@ -194,6 +216,15 @@ int kvm_arch_get_registers(CPUState *cs) > } > pstate_write(env, val); > > + /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the > + * QEMU side we keep the current SP in xregs[31] as well. > + */ > + if (env->pstate & PSTATE_SP) { > + env->xregs[31] = env->sp_el[1]; > + } else { > + env->xregs[31] = env->sp_el[0]; > + } > + > reg.id = AARCH64_CORE_REG(regs.pc); > reg.addr = (uintptr_t) &env->pc; > ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > diff --git a/target-arm/machine.c b/target-arm/machine.c > index 01d8f83..af49662 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -222,9 +222,9 @@ static int cpu_post_load(void *opaque, int version_id) > > const VMStateDescription vmstate_arm_cpu = { > .name = "cpu", > - .version_id = 15, > - .minimum_version_id = 15, > - .minimum_version_id_old = 15, > + .version_id = 16, > + .minimum_version_id = 16, > + .minimum_version_id_old = 16, So in the past, I have squashed unrelated patches together to minimise VMSD versions bumps. Whats more important - these versions numbers of git patch separation? Regards, Peter > .pre_save = cpu_pre_save, > .post_load = cpu_post_load, > .fields = (VMStateField[]) { > @@ -244,6 +244,7 @@ const VMStateDescription vmstate_arm_cpu = { > VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5), > VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5), > VMSTATE_UINT64(env.elr_el1, ARMCPU), > + VMSTATE_UINT64_ARRAY(env.sp_el, ARMCPU, 2), > /* The length-check must come before the arrays to avoid > * incoming data possibly overflowing the array. > */ > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index b1db672..aeb2538 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -349,7 +349,7 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) > > switch (op) { > case 0x05: /* SPSel */ > - env->pstate = deposit32(env->pstate, 0, 1, imm); > + update_spsel(env, imm); > break; > case 0x1e: /* DAIFSet */ > env->daif |= (imm << 6) & PSTATE_DAIF; > -- > 1.9.0 > >