On Mon, Mar 17, 2014 at 5:02 PM, Peter Crosthwaite
<peter.crosthwaite@xxxxxxxxxx> wrote:
> On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell <peter.maydell@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>>
>>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
