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

[Peter Maydell]

On 17 March 2014 07:02, 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".

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 <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?

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