| Message ID | 1394134385-1727-17-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> > --- > 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, <Xt> ; */ /* 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 > >
On Mon, Mar 17, 2014 at 5:02 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> >> --- >> 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; >> + } Ergh, my bad. Missed your short return path here. >> + 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, <Xt> ; */ > /* 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. > Sorry for the noise. Regards, Peter >> + 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 >> >>
On 17 March 2014 07:02, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell <peter.maydell@linaro.org> 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". Fixed, thanks. >> 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 <peter.maydell@linaro.org> >> --- >> 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? I'm not really convinced, because the set of things that would need to change to make the maximum EL not 1 would be so huge that one array size more or less is not really very relevant. This seems to me like the kind of thing that will shake itself out when somebody gets round to adding EL2 or EL3 emulation support. >> 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? We have an entire 32 bit integer to work with for the ID numbers, so there's no reason to make patches harder to review by squashing together things that would be clearer handled separately. thanks -- PMM
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 */ /* 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]; + 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, .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;
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 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 <peter.maydell@linaro.org> --- 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(-)