Re: [Qemu-devel] [PATCH v4 16/21] target-arm: Implement SP_EL0, SP_EL1

[Peter Crosthwaite]

On Mon, Mar 17, 2014 at 5:02 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell <address@hidden> 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 <address@hidden>
>> ---
>>  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, &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, &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, &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, &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, &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
>>
>>