[Qemu-devel] [PATCH 0/6] QEMU ARM64 Migration Fixes

[Qemu-devel] [PATCH 0/6] QEMU ARM64 Migration Fixes

[Alex Bennée]

In conjunction with the kernel patch series I've just sent:

  http://thread.gmane.org/gmane.comp.emulators.kvm.arm.devel/101/focus=100

These patches have been fairly heavily tested on the xgene systems
I've got access to and I'm pretty confident we've caught all the
corner cases.

>From QEMU's point of view the fixes are fairly simple. We need to take
a little care when restoring the GIC that config that affects the
later restoration is restored first. The rest was simply missing
serialisation code for SPSR and FP registers.

The pl011 patch was mainly to reduce noise on re-asserting level
triggered interrupt lines. And the cpu.h documentation was for my own
sanity.

Cheers,

Alex.


Alex Bennée (5):
  target-arm: kvm: save/restore mp state
  arm_gic_kvm.c: restore config before pending IRQs
  hw/char/pl011: don't keep setting the IRQ if nothing changed
  target-arm/kvm64.c: sync FP register state
  target-arm/cpu.h: document why env->spsr exists

Christoffer Dall (1):
  target-arm/kvm64: fix save/restore of SPSR regs

 hw/char/pl011.c       |  12 ++++--
 hw/intc/arm_gic_kvm.c |   7 +++-
 target-arm/cpu.h      |   5 +++
 target-arm/kvm.c      |   1 +
 target-arm/kvm64.c    | 104 +++++++++++++++++++++++++++++++++++++++++++++++---
 target-arm/machine.c  |  38 ++++++++++++++++++
 6 files changed, 156 insertions(+), 11 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Alex Bennée]

This adds the saving and restore of the current Multi-Processing state
of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
potential states for x86 we only use two for ARM. Either the process is
running or not.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 23cefe9..8732854 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -25,6 +25,7 @@
 #include "hw/arm/arm.h"
 
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
+    KVM_CAP_INFO(MP_STATE),
     KVM_CAP_LAST_INFO
 };
 
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 9446e5a..70b1bc4 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -161,6 +161,34 @@ static const VMStateInfo vmstate_cpsr = {
     .put = put_cpsr,
 };
 
+#if defined CONFIG_KVM
+static int get_mpstate(QEMUFile *f, void *opaque, size_t size)
+{
+    ARMCPU *cpu = opaque;
+    struct kvm_mp_state mp_state = { .mp_state =  qemu_get_be32(f)};
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
+}
+
+static void put_mpstate(QEMUFile *f, void *opaque, size_t size)
+{
+    ARMCPU *cpu = opaque;
+    struct kvm_mp_state mp_state;
+    int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
+    if (ret) {
+        fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
+                __func__, ret, strerror(ret));
+        abort();
+    }
+    qemu_put_be32(f, mp_state.mp_state);
+}
+
+static const VMStateInfo vmstate_mpstate = {
+    .name = "mpstate",
+    .get = get_mpstate,
+    .put = put_mpstate,
+};
+#endif
+
 static void cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -244,6 +272,16 @@ const VMStateDescription vmstate_arm_cpu = {
         VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
         VMSTATE_UINT64_ARRAY(env.xregs, ARMCPU, 32),
         VMSTATE_UINT64(env.pc, ARMCPU),
+#if defined CONFIG_KVM
+        {
+            .name = "mp_state",
+            .version_id = 0,
+            .size = sizeof(uint32_t),
+            .info = &vmstate_mpstate,
+            .flags = VMS_SINGLE,
+            .offset = 0,
+        },
+#endif
         {
             .name = "cpsr",
             .version_id = 0,
-- 
2.3.0

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Peter Maydell]

On 26 February 2015 at 01:02, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds the saving and restore of the current Multi-Processing state
> of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
> potential states for x86 we only use two for ARM. Either the process is
> running or not.

By this you mean "is the CPU in the PSCI powered down state or not",
right?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 23cefe9..8732854 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -25,6 +25,7 @@
>  #include "hw/arm/arm.h"
>
>  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> +    KVM_CAP_INFO(MP_STATE),

Does this really want to be a required cap? I haven't checked,
but assuming 'required' means what it says this presumably
means we'll stop working on host kernels we previously ran
fine on (even if migration didn't work there).

>      KVM_CAP_LAST_INFO
>  };
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9446e5a..70b1bc4 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -161,6 +161,34 @@ static const VMStateInfo vmstate_cpsr = {
>      .put = put_cpsr,
>  };
>
> +#if defined CONFIG_KVM
> +static int get_mpstate(QEMUFile *f, void *opaque, size_t size)
> +{
> +    ARMCPU *cpu = opaque;
> +    struct kvm_mp_state mp_state = { .mp_state =  qemu_get_be32(f)};
> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
> +}

Won't this break if you're running a QEMU built with KVM
support in TCG mode on an aarch64 host?

In any case, for that configuration we should be migrating the
TCG cpu->powered_off flag, which is where we keep the PSCI
power state for TCG.

> +
> +static void put_mpstate(QEMUFile *f, void *opaque, size_t size)
> +{
> +    ARMCPU *cpu = opaque;
> +    struct kvm_mp_state mp_state;
> +    int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
> +    if (ret) {
> +        fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
> +                __func__, ret, strerror(ret));
> +        abort();
> +    }
> +    qemu_put_be32(f, mp_state.mp_state);
> +}
> +
> +static const VMStateInfo vmstate_mpstate = {
> +    .name = "mpstate",
> +    .get = get_mpstate,
> +    .put = put_mpstate,
> +};
> +#endif
> +
>  static void cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> @@ -244,6 +272,16 @@ const VMStateDescription vmstate_arm_cpu = {
>          VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
>          VMSTATE_UINT64_ARRAY(env.xregs, ARMCPU, 32),
>          VMSTATE_UINT64(env.pc, ARMCPU),
> +#if defined CONFIG_KVM
> +        {
> +            .name = "mp_state",
> +            .version_id = 0,
> +            .size = sizeof(uint32_t),
> +            .info = &vmstate_mpstate,
> +            .flags = VMS_SINGLE,
> +            .offset = 0,
> +        },
> +#endif

This means the migration format will be different on different
build configurations, which seems like a really bad idea.

Have you considered having the KVM state sync functions just
sync the kernel's MP state into cpu->powered_off, and then
migrating that flag here unconditionally?

>          {
>              .name = "cpsr",
>              .version_id = 0,
> --
> 2.3.0
>

-- PMM

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 February 2015 at 01:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This adds the saving and restore of the current Multi-Processing state
>> of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
>> potential states for x86 we only use two for ARM. Either the process is
>> running or not.
>
> By this you mean "is the CPU in the PSCI powered down state or not",
> right?

>From the vcpu's perspective it is either running or not. However it is
the same mechanism that is used when PSCI_0_2_FN_CPU_OFF is passed the
VM, internally setting vcpu->arch.paused.

>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 23cefe9..8732854 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -25,6 +25,7 @@
>>  #include "hw/arm/arm.h"
>>
>>  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>> +    KVM_CAP_INFO(MP_STATE),
>
> Does this really want to be a required cap? I haven't checked,
> but assuming 'required' means what it says this presumably
> means we'll stop working on host kernels we previously ran
> fine on (even if migration didn't work there).

You are right, I'll move the CAP check to the kvm_enabled() leg of
get/set functions.

>
>>      KVM_CAP_LAST_INFO
>>  };
>>
>> diff --git a/target-arm/machine.c b/target-arm/machine.c
>> index 9446e5a..70b1bc4 100644
>> --- a/target-arm/machine.c
>> +++ b/target-arm/machine.c
>> @@ -161,6 +161,34 @@ static const VMStateInfo vmstate_cpsr = {
>>      .put = put_cpsr,
>>  };
>>
>> +#if defined CONFIG_KVM
>> +static int get_mpstate(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    ARMCPU *cpu = opaque;
>> +    struct kvm_mp_state mp_state = { .mp_state =  qemu_get_be32(f)};
>> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
>> +}
>
> Won't this break if you're running a QEMU built with KVM
> support in TCG mode on an aarch64 host?
>
> In any case, for that configuration we should be migrating the
> TCG cpu->powered_off flag, which is where we keep the PSCI
> power state for TCG.

Yeah, I'll make the access functions aware of the mode. In itself it
won't enable KVM<->TCG migration though!

>
>> +
>> +static void put_mpstate(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    ARMCPU *cpu = opaque;
>> +    struct kvm_mp_state mp_state;
>> +    int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
>> +    if (ret) {
>> +        fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
>> +                __func__, ret, strerror(ret));
>> +        abort();
>> +    }
>> +    qemu_put_be32(f, mp_state.mp_state);
>> +}
>> +
>> +static const VMStateInfo vmstate_mpstate = {
>> +    .name = "mpstate",
>> +    .get = get_mpstate,
>> +    .put = put_mpstate,
>> +};
>> +#endif
>> +
>>  static void cpu_pre_save(void *opaque)
>>  {
>>      ARMCPU *cpu = opaque;
>> @@ -244,6 +272,16 @@ const VMStateDescription vmstate_arm_cpu = {
>>          VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
>>          VMSTATE_UINT64_ARRAY(env.xregs, ARMCPU, 32),
>>          VMSTATE_UINT64(env.pc, ARMCPU),
>> +#if defined CONFIG_KVM
>> +        {
>> +            .name = "mp_state",
>> +            .version_id = 0,
>> +            .size = sizeof(uint32_t),
>> +            .info = &vmstate_mpstate,
>> +            .flags = VMS_SINGLE,
>> +            .offset = 0,
>> +        },
>> +#endif
>
> This means the migration format will be different on different
> build configurations, which seems like a really bad idea.
>
> Have you considered having the KVM state sync functions just
> sync the kernel's MP state into cpu->powered_off, and then
> migrating that flag here unconditionally?

I was looking at using:

  .field_exists = mpstate_valid

for the field. If we have the field in the migration stream and the
kernel doesn't have the capability to set the state we need to fail hard
right?


>
>>          {
>>              .name = "cpsr",
>>              .version_id = 0,
>> --
>> 2.3.0
>>
>
> -- PMM

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Paolo Bonzini]

On 03/03/2015 11:56, Alex Bennée wrote:
> > > This adds the saving and restore of the current Multi-Processing state
> > > of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
> > > potential states for x86 we only use two for ARM. Either the process is
> > > running or not.
> >
> > By this you mean "is the CPU in the PSCI powered down state or not",
> > right?
> 
> From the vcpu's perspective it is either running or not. However it is
> the same mechanism that is used when PSCI_0_2_FN_CPU_OFF is passed the
> VM, internally setting vcpu->arch.paused.

I suggest that you define a new MP_STATE constant for this.  HALTED in
x86 and s390 is the state an ARM processor enters when you execute wfi.
 Right now this is not migrated on ARM if I remember correctly, but
perhaps you'll want to add it in the future.

Paolo

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Peter Maydell]

On 3 March 2015 at 20:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 03/03/2015 11:56, Alex Bennée wrote:
>> > > This adds the saving and restore of the current Multi-Processing state
>> > > of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
>> > > potential states for x86 we only use two for ARM. Either the process is
>> > > running or not.
>> >
>> > By this you mean "is the CPU in the PSCI powered down state or not",
>> > right?
>>
>> From the vcpu's perspective it is either running or not. However it is
>> the same mechanism that is used when PSCI_0_2_FN_CPU_OFF is passed the
>> VM, internally setting vcpu->arch.paused.

Well, it has to be (ABI defined to be) identical with being PSCI
powered down/up, because that's how userspace is going to be
treating it. If we might tell userspace we're in the "not running"
state for other cases than PSCI-powered-down then we probably need
to consider separating those out into separate states.

> I suggest that you define a new MP_STATE constant for this.  HALTED in
> x86 and s390 is the state an ARM processor enters when you execute wfi.

Architecturally the CPU doesn't have to enter any state at all
if you execute a WFI -- it might be implemented as "go to low
power state and wait for an interrupt" or "go low power but
maybe be unnecessarily woken up" or "nop, do nothing"...

>  Right now this is not migrated on ARM if I remember correctly, but
> perhaps you'll want to add it in the future.

...which is why we don't need to migrate this: it just means
that migration during WFI causes an unnecessary-wakeup, which
is architecturally fine.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 3 March 2015 at 20:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 03/03/2015 11:56, Alex Bennée wrote:
>>> > > This adds the saving and restore of the current Multi-Processing state
>>> > > of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
>>> > > potential states for x86 we only use two for ARM. Either the process is
>>> > > running or not.
>>> >
>>> > By this you mean "is the CPU in the PSCI powered down state or not",
>>> > right?
>>>
>>> From the vcpu's perspective it is either running or not. However it is
>>> the same mechanism that is used when PSCI_0_2_FN_CPU_OFF is passed the
>>> VM, internally setting vcpu->arch.paused.
>
> Well, it has to be (ABI defined to be) identical with being PSCI
> powered down/up, because that's how userspace is going to be
> treating it. If we might tell userspace we're in the "not running"
> state for other cases than PSCI-powered-down then we probably need
> to consider separating those out into separate states.
>
>> I suggest that you define a new MP_STATE constant for this.  HALTED in
>> x86 and s390 is the state an ARM processor enters when you execute wfi.
>
> Architecturally the CPU doesn't have to enter any state at all
> if you execute a WFI -- it might be implemented as "go to low
> power state and wait for an interrupt" or "go low power but
> maybe be unnecessarily woken up" or "nop, do nothing"...
>
>>  Right now this is not migrated on ARM if I remember correctly, but
>> perhaps you'll want to add it in the future.
>
> ...which is why we don't need to migrate this: it just means
> that migration during WFI causes an unnecessary-wakeup, which
> is architecturally fine.

What happens when you boot a SMP system but only ever power up one of the
CPUs? You can't just randomly start the second CPU if it's in the
powered off state, who knows what it would do?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Paolo Bonzini]

On 03/03/2015 17:30, Alex Bennée wrote:
>> >
>>> >>  Right now this is not migrated on ARM if I remember correctly, but
>>> >> perhaps you'll want to add it in the future.
>> >
>> > ...which is why we don't need to migrate this: it just means
>> > that migration during WFI causes an unnecessary-wakeup, which
>> > is architecturally fine.
> What happens when you boot a SMP system but only ever power up one of the
> CPUs? You can't just randomly start the second CPU if it's in the
> powered off state, who knows what it would do?

The second CPU would not be in the WFI state, which is what Peter is
talking about.

I agree that this state should be saved/restored.  I'm just saying that
HALTED is not the right constant to use.

Paolo

Re: [Qemu-devel] [PATCH 1/6] target-arm: kvm: save/restore mp state

[Paolo Bonzini]

On 25/02/2015 17:02, Alex Bennée wrote:
> +#if defined CONFIG_KVM
> +        {
> +            .name = "mp_state",
> +            .version_id = 0,
> +            .size = sizeof(uint32_t),
> +            .info = &vmstate_mpstate,
> +            .flags = VMS_SINGLE,
> +            .offset = 0,
> +        },
> +#endif

This makes TCG migration state incompatible depending on whether QEMU
was built on ARM or x86.

You can instead add a subsection, and mark it as needed only iff
kvm_enabled().

Paolo

>          {
>              .name = "cpsr",

[Qemu-devel] [PATCH 2/6] arm_gic_kvm.c: restore config before pending IRQs

[Alex Bennée]

As there is logic to deal with the difference between edge and level
triggered interrupts in the kernel we must ensure it knows the
configuration of the IRQs before we restore the pending state.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 1ad3eb0..2f21ae7 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -370,6 +370,11 @@ static void kvm_arm_gic_put(GICState *s)
      * the appropriate CPU interfaces in the kernel) */
     kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets);
 
+    /* irq_state[n].trigger -> GICD_ICFGRn
+     * (restore targets before pending IRQs so we treat level/edge
+     * correctly */
+    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
+
     /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */
     kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear);
     kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending);
@@ -378,8 +383,6 @@ static void kvm_arm_gic_put(GICState *s)
     kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear);
     kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active);
 
-    /* irq_state[n].trigger -> GICD_ICFRn */
-    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
 
     /* s->priorityX[irq] -> ICD_IPRIORITYRn */
     kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority);
-- 
2.3.0

Re: [Qemu-devel] [PATCH 2/6] arm_gic_kvm.c: restore config before pending IRQs

[Christoffer Dall]

On Wed, Feb 25, 2015 at 04:02:34PM +0000, Alex Bennée wrote:
> As there is logic to deal with the difference between edge and level
> triggered interrupts in the kernel we must ensure it knows the
> configuration of the IRQs before we restore the pending state.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[Qemu-devel] [PATCH 3/6] hw/char/pl011: don't keep setting the IRQ if nothing changed

[Alex Bennée]

While observing KVM traces I can see additional IRQ calls on pretty much
every MMIO access which is just plain inefficient. Only update the QEMU
IRQ level if something has actually changed from last time. Otherwise we
may be papering over other failure modes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 0a45115..bb554bc 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -36,6 +36,9 @@ typedef struct PL011State {
     CharDriverState *chr;
     qemu_irq irq;
     const unsigned char *id;
+
+    /* not serialised, prevents pl011_update doing extra set_irqs */
+    uint32_t current_irq;
 } PL011State;
 
 #define PL011_INT_TX 0x20
@@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] =
 
 static void pl011_update(PL011State *s)
 {
-    uint32_t flags;
-
-    flags = s->int_level & s->int_enabled;
-    qemu_set_irq(s->irq, flags != 0);
+    uint32_t flags = s->int_level & s->int_enabled;
+    if (flags != s->current_irq) {
+        s->current_irq = flags;
+        qemu_set_irq(s->irq, s->current_irq != 0);
+    }
 }
 
 static uint64_t pl011_read(void *opaque, hwaddr offset,
-- 
2.3.0

[Qemu-devel] [PATCH 4/6] target-arm/kvm64.c: sync FP register state

[Alex Bennée]

From: Alex Bennée <alex@bennee.com>

For migration to work we need to sync all of the register state. This is
especially noticeable when GCC starts using FP registers as spill
registers even with integer programs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 8cf3a62..c60e989 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -126,9 +126,17 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
 #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
                  KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
 
+/* The linux headers don't define a 128 bit wide SIMD macro for us */
+#define AARCH64_SIMD_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | \
+                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
+
+#define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
+                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     struct kvm_one_reg reg;
+    uint32_t fpr;
     uint64_t val;
     int i;
     int ret;
@@ -207,13 +215,36 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
+    /* Advanced SIMD and FP registers */
+    for (i = 0; i < 32; i++) {
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&env->vfp.regs[i]);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+        reg.id++;
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    fpr = vfp_get_fpsr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    fpr = vfp_get_fpcr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
     if (!write_list_to_kvmstate(cpu)) {
         return EINVAL;
     }
 
-    /* TODO:
-     * FP state
-     */
     return ret;
 }
 
@@ -221,6 +252,7 @@ int kvm_arch_get_registers(CPUState *cs)
 {
     struct kvm_one_reg reg;
     uint64_t val;
+    uint32_t fpr;
     int i;
     int ret;
 
@@ -302,9 +334,36 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    /* Advanced SIMD and FP registers */
+    for (i = 0; i < 32; i++) {
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&env->vfp.regs[i]);
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+        reg.id++;
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpsr(env, fpr);
+
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpcr(env, fpr);
+
     if (!write_kvmstate_to_list(cpu)) {
         return EINVAL;
     }
+
     /* Note that it's OK to have registers which aren't in CPUState,
      * so we can ignore a failure return here.
      */
-- 
2.3.0

[Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR registers

[Alex Bennée]

From: Christoffer Dall <christoffer.dall@linaro.org>

The current code was negatively indexing the cpu state array and not
synchronizing banked spsr register state with the current mode's spsr
state, causing occasional failures with migration.

Some munging is done to take care of the aarch64 mapping and also to
ensure the most current value of the spsr is updated to the banked
registers (relevant for KVM<->TCG migration).

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2 (ajb)
  - minor tweaks and clarifications

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index c60e989..6d30e10 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     uint64_t val;
     int i;
     int ret;
+    unsigned int el;
 
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
-    for (i = 0; i < KVM_NR_SPSR; i++) {
+    /* Saved Program State Registers
+     *
+     * Before we restore from the banked_spsr[] array we need to
+     * ensure that any modifications to env->spsr are correctly
+     * reflected and map aarch64 exception levels if required.
+     */
+    el = arm_current_el(env);
+    if (is_a64(env) && el > 0) {
+        g_assert(el == 1);
+        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+        env->banked_spsr[1] = env->banked_spsr[0];
+        env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr;
+    } else {
+        env->banked_spsr[el] = env->spsr;
+    }
+
+    for (i=0; i<KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
         ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
         if (ret) {
             return ret;
@@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
     struct kvm_one_reg reg;
     uint64_t val;
     uint32_t fpr;
+    unsigned int el;
     int i;
     int ret;
 
@@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
-    for (i = 0; i < KVM_NR_SPSR; i++) {
+    /* Fetch the SPSR registers
+     *
+     * KVM has an array of state indexed for all the possible aarch32
+     * privilage levels. Although not all are valid at all points
+     * there are some transitions possible which can access old state
+     * so it is worth keeping them all.
+     */
+    for (i=0; i<KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
         ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
         if (ret) {
             return ret;
         }
     }
 
+    el = arm_current_el(env);
+    if (is_a64(env) && el > 0) {
+        g_assert(el == 1);
+        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+        env->banked_spsr[0] = env->banked_spsr[1];
+        env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)];
+    } else {
+        env->spsr = env->banked_spsr[el];
+    }
+
     /* Advanced SIMD and FP registers */
     for (i = 0; i < 32; i++) {
         reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-- 
2.3.0

[Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Alex Bennée]

From: Christoffer Dall <christoffer.dall@linaro.org>

The current code was negatively indexing the cpu state array and not
synchronizing banked spsr register state with the current mode's spsr
state, causing occasional failures with migration.

Some munging is done to take care of the aarch64 mapping and also to
ensure the most current value of the spsr is updated to the banked
registers (relevant for KVM<->TCG migration).

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2 (ajb)
  - minor tweaks and clarifications

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index c60e989..1e36b0a 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     uint64_t val;
     int i;
     int ret;
+    unsigned int el;
 
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
+    /* Saved Program State Registers
+     *
+     * Before we restore from the banked_spsr[] array we need to
+     * ensure that any modifications to env->spsr are correctly
+     * reflected and map aarch64 exception levels if required.
+     */
+    el = arm_current_el(env);
+    if (is_a64(env) && el > 0) {
+        g_assert(el == 1);
+        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+        env->banked_spsr[1] = env->banked_spsr[0];
+        env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr;
+    } else {
+        env->banked_spsr[el] = env->spsr;
+    }
+
     for (i = 0; i < KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
         ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
         if (ret) {
             return ret;
@@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
     struct kvm_one_reg reg;
     uint64_t val;
     uint32_t fpr;
+    unsigned int el;
     int i;
     int ret;
 
@@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
+    /* Fetch the SPSR registers
+     *
+     * KVM has an array of state indexed for all the possible aarch32
+     * privilage levels. Although not all are valid at all points
+     * there are some transitions possible which can access old state
+     * so it is worth keeping them all.
+     */
     for (i = 0; i < KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
         ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
         if (ret) {
             return ret;
         }
     }
 
+    el = arm_current_el(env);
+    if (is_a64(env) && el > 0) {
+        g_assert(el == 1);
+        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+        env->banked_spsr[0] = env->banked_spsr[1];
+        env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)];
+    } else {
+        env->spsr = env->banked_spsr[el];
+    }
+
     /* Advanced SIMD and FP registers */
     for (i = 0; i < 32; i++) {
         reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-- 
2.3.0

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Christoffer Dall]

Hi Alex,

Seems like you accidentally sent out two copies of this patch, hopefully
I'm reviewing the right one...

On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Bennée wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> The current code was negatively indexing the cpu state array and not
> synchronizing banked spsr register state with the current mode's spsr
> state, causing occasional failures with migration.
> 
> Some munging is done to take care of the aarch64 mapping and also to
> ensure the most current value of the spsr is updated to the banked
> registers (relevant for KVM<->TCG migration).
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2 (ajb)
>   - minor tweaks and clarifications
> 
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index c60e989..1e36b0a 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      uint64_t val;
>      int i;
>      int ret;
> +    unsigned int el;
>  
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          return ret;
>      }
>  
> +    /* Saved Program State Registers
> +     *
> +     * Before we restore from the banked_spsr[] array we need to
> +     * ensure that any modifications to env->spsr are correctly
> +     * reflected and map aarch64 exception levels if required.
> +     */
> +    el = arm_current_el(env);
> +    if (is_a64(env) && el > 0) {
> +        g_assert(el == 1);
> +        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
> +        env->banked_spsr[1] = env->banked_spsr[0];
> +        env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr;
> +    } else {
> +        env->banked_spsr[el] = env->spsr;

is this valid if (is_a64(env) && el == 0)) ?  I thought that if you're
in el == 0, then env->banked_spsr[x] is the most up-to-date one, not
env->spsr ?

for !is_a64(env) this looks wrong, because of the same as above if el ==
0, but also because I think you need
bank_number(env->uncached_cpsr & CPSR_M) to index into the array.

> +    }
> +
>      for (i = 0; i < KVM_NR_SPSR; i++) {
>          reg.id = AARCH64_CORE_REG(spsr[i]);
> -        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
> +        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>          if (ret) {
>              return ret;
> @@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      struct kvm_one_reg reg;
>      uint64_t val;
>      uint32_t fpr;
> +    unsigned int el;
>      int i;
>      int ret;
>  
> @@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
>          return ret;
>      }
>  
> +    /* Fetch the SPSR registers
> +     *
> +     * KVM has an array of state indexed for all the possible aarch32
> +     * privilage levels. Although not all are valid at all points
> +     * there are some transitions possible which can access old state
> +     * so it is worth keeping them all.
> +     */
>      for (i = 0; i < KVM_NR_SPSR; i++) {
>          reg.id = AARCH64_CORE_REG(spsr[i]);
> -        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
> +        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>          ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
>          if (ret) {
>              return ret;
>          }
>      }
>  
> +    el = arm_current_el(env);
> +    if (is_a64(env) && el > 0) {
> +        g_assert(el == 1);
> +        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
> +        env->banked_spsr[0] = env->banked_spsr[1];
> +        env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)];
> +    } else {
> +        env->spsr = env->banked_spsr[el];

same concern with bank_number as above.

> +    }
> +
>      /* Advanced SIMD and FP registers */
>      for (i = 0; i < 32; i++) {
>          reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> -- 
> 2.3.0
> 

Thanks,
-Christoffer

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> Hi Alex,
>
> Seems like you accidentally sent out two copies of this patch, hopefully
> I'm reviewing the right one...

Oops, perils of not wiping your output directory. I think it was just a
title tweak!

> On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Bennée wrote:
>> From: Christoffer Dall <christoffer.dall@linaro.org>
>> 
>> The current code was negatively indexing the cpu state array and not
>> synchronizing banked spsr register state with the current mode's spsr
>> state, causing occasional failures with migration.
>> 
>> Some munging is done to take care of the aarch64 mapping and also to
>> ensure the most current value of the spsr is updated to the banked
>> registers (relevant for KVM<->TCG migration).
>> 
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v2 (ajb)
>>   - minor tweaks and clarifications
>> 
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index c60e989..1e36b0a 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>      uint64_t val;
>>      int i;
>>      int ret;
>> +    unsigned int el;
>>  
>>      ARMCPU *cpu = ARM_CPU(cs);
>>      CPUARMState *env = &cpu->env;
>> @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>          return ret;
>>      }
>>  
>> +    /* Saved Program State Registers
>> +     *
>> +     * Before we restore from the banked_spsr[] array we need to
>> +     * ensure that any modifications to env->spsr are correctly
>> +     * reflected and map aarch64 exception levels if required.
>> +     */
>> +    el = arm_current_el(env);
>> +    if (is_a64(env) && el > 0) {
>> +        g_assert(el == 1);
>> +        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
>> +        env->banked_spsr[1] = env->banked_spsr[0];
>> +        env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr;
>> +    } else {
>> +        env->banked_spsr[el] = env->spsr;
>
> is this valid if (is_a64(env) && el == 0)) ?  I thought that if you're
> in el == 0, then env->banked_spsr[x] is the most up-to-date one, not
> env->spsr ?

Actually they will both be the same (at least for KVM). In the end both:

        VMSTATE_UINT32(env.spsr, ARMCPU),
        VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8),

get serialised in the stream and when we save the stream out we make
sure everything is in sync (i.e. env->spsr is correct). In reality this
makes more sense for TCG than KVM which is the only reason env->spsr
exists.

>
> for !is_a64(env) this looks wrong, because of the same as above if el ==
> 0, but also because I think you need
> bank_number(env->uncached_cpsr & CPSR_M) to index into the array.
>

Good catch. For some reason I was treating the number from
arm_current_el() as equivalent. How about:

    el = arm_current_el(env);
    if (is_a64(env) && el > 0) {
        g_assert(el == 1);
        /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
        env->banked_spsr[1] = env->banked_spsr[0];
    }
    i = is_a64(env) ?
        aarch64_banked_spsr_index(el) : bank_number(env->unached_cpsr & CPSR_M);
    env->banked_spsr[i] = env->spsr;


>> +    }
>> +
>>      for (i = 0; i < KVM_NR_SPSR; i++) {
>>          reg.id = AARCH64_CORE_REG(spsr[i]);
>> -        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
>> +        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>>          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>>          if (ret) {
>>              return ret;
>> @@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
>>      struct kvm_one_reg reg;
>>      uint64_t val;
>>      uint32_t fpr;
>> +    unsigned int el;
>>      int i;
>>      int ret;
>>  
>> @@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
>>          return ret;
>>      }
>>  
>> +    /* Fetch the SPSR registers
>> +     *
>> +     * KVM has an array of state indexed for all the possible aarch32
>> +     * privilage levels. Although not all are valid at all points
>> +     * there are some transitions possible which can access old state
>> +     * so it is worth keeping them all.
>> +     */
>>      for (i = 0; i < KVM_NR_SPSR; i++) {
>>          reg.id = AARCH64_CORE_REG(spsr[i]);
>> -        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
>> +        reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>>          ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
>>          if (ret) {
>>              return ret;
>>          }
>>      }
>>  
>> +    el = arm_current_el(env);
>> +    if (is_a64(env) && el > 0) {
>> +        g_assert(el == 1);
>> +        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
>> +        env->banked_spsr[0] = env->banked_spsr[1];
>> +        env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)];
>> +    } else {
>> +        env->spsr = env->banked_spsr[el];
>
> same concern with bank_number as above.
>
>> +    }
>> +
>>      /* Advanced SIMD and FP registers */
>>      for (i = 0; i < 32; i++) {
>>          reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
>> -- 
>> 2.3.0
>> 
>
> Thanks,
> -Christoffer

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Christoffer Dall]

On Tue, Mar 03, 2015 at 11:28:21AM +0000, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > Hi Alex,
> >
> > Seems like you accidentally sent out two copies of this patch, hopefully
> > I'm reviewing the right one...
> 
> Oops, perils of not wiping your output directory. I think it was just a
> title tweak!
> 
> > On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Bennée wrote:
> >> From: Christoffer Dall <christoffer.dall@linaro.org>
> >> 
> >> The current code was negatively indexing the cpu state array and not
> >> synchronizing banked spsr register state with the current mode's spsr
> >> state, causing occasional failures with migration.
> >> 
> >> Some munging is done to take care of the aarch64 mapping and also to
> >> ensure the most current value of the spsr is updated to the banked
> >> registers (relevant for KVM<->TCG migration).
> >> 
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> 
> >> ---
> >> v2 (ajb)
> >>   - minor tweaks and clarifications
> >> 
> >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> >> index c60e989..1e36b0a 100644
> >> --- a/target-arm/kvm64.c
> >> +++ b/target-arm/kvm64.c
> >> @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >>      uint64_t val;
> >>      int i;
> >>      int ret;
> >> +    unsigned int el;
> >>  
> >>      ARMCPU *cpu = ARM_CPU(cs);
> >>      CPUARMState *env = &cpu->env;
> >> @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >>          return ret;
> >>      }
> >>  
> >> +    /* Saved Program State Registers
> >> +     *
> >> +     * Before we restore from the banked_spsr[] array we need to
> >> +     * ensure that any modifications to env->spsr are correctly
> >> +     * reflected and map aarch64 exception levels if required.
> >> +     */
> >> +    el = arm_current_el(env);
> >> +    if (is_a64(env) && el > 0) {
> >> +        g_assert(el == 1);
> >> +        /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
> >> +        env->banked_spsr[1] = env->banked_spsr[0];
> >> +        env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr;
> >> +    } else {
> >> +        env->banked_spsr[el] = env->spsr;
> >
> > is this valid if (is_a64(env) && el == 0)) ?  I thought that if you're
> > in el == 0, then env->banked_spsr[x] is the most up-to-date one, not
> > env->spsr ?
> 
> Actually they will both be the same (at least for KVM). In the end both:
> 
>         VMSTATE_UINT32(env.spsr, ARMCPU),
>         VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8),
> 
> get serialised in the stream and when we save the stream out we make
> sure everything is in sync (i.e. env->spsr is correct). In reality this
> makes more sense for TCG than KVM which is the only reason env->spsr
> exists.
> 

this function, however, is not used only when migration, but should
generally cover the case where you want to synchronize QEMU's state into
KVM's state, right?  So while it may not be harmful in currently
supported use cases, is there ever a situation where (is_a64(env) && el
== 0) and env->spsr != banked_spsr[el], and where env->spsr is
out-of-date?

If that's the case, I think it would be better to have an assert that
says "don't call the code in this situation" than relying on limited use
cases for callers of this function.

> >
> > for !is_a64(env) this looks wrong, because of the same as above if el ==
> > 0, but also because I think you need
> > bank_number(env->uncached_cpsr & CPSR_M) to index into the array.
> >
> 
> Good catch. For some reason I was treating the number from
> arm_current_el() as equivalent. How about:
> 
>     el = arm_current_el(env);
>     if (is_a64(env) && el > 0) {
>         g_assert(el == 1);
>         /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
>         env->banked_spsr[1] = env->banked_spsr[0];
>     }
>     i = is_a64(env) ?
>         aarch64_banked_spsr_index(el) : bank_number(env->unached_cpsr & CPSR_M);

I think that will fail due to the assert in aarch64_banked_spsr_index()
for el == 0.


-Christoffer

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Peter Maydell]

On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> this function, however, is not used only when migration, but should
> generally cover the case where you want to synchronize QEMU's state into
> KVM's state, right?  So while it may not be harmful in currently
> supported use cases, is there ever a situation where (is_a64(env) && el
> == 0) and env->spsr != banked_spsr[el], and where env->spsr is
> out-of-date?

If EL == 0 then you can't access any SPSR, so env->spsr is by
definition out of date.

-- PMM

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> this function, however, is not used only when migration, but should
>> generally cover the case where you want to synchronize QEMU's state into
>> KVM's state, right?  So while it may not be harmful in currently
>> supported use cases, is there ever a situation where (is_a64(env) && el
>> == 0) and env->spsr != banked_spsr[el], and where env->spsr is
>> out-of-date?
>
> If EL == 0 then you can't access any SPSR, so env->spsr is by
> definition out of date.

Indeed and in v2 the whole thing is wrapped in if (el > 0)

>
> -- PMM

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

[Christoffer Dall]

On Mon, Mar 09, 2015 at 10:31:19PM +0900, Peter Maydell wrote:
> On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > this function, however, is not used only when migration, but should
> > generally cover the case where you want to synchronize QEMU's state into
> > KVM's state, right?  So while it may not be harmful in currently
> > supported use cases, is there ever a situation where (is_a64(env) && el
> > == 0) and env->spsr != banked_spsr[el], and where env->spsr is
> > out-of-date?
> 
> If EL == 0 then you can't access any SPSR, so env->spsr is by
> definition out of date.
> 
ok, never mind then.

-Christoffer

[Qemu-devel] [PATCH 6/6] target-arm/cpu.h: document why env->spsr exists

[Alex Bennée]

I was getting very confused about the duplication of state. Perhaps we
should just get rid of env->spsr and use helpers that understand the
banking?

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 11845a6..d7fd13f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -155,6 +155,11 @@ typedef struct CPUARMState {
        This contains all the other bits.  Use cpsr_{read,write} to access
        the whole CPSR.  */
     uint32_t uncached_cpsr;
+    /* The spsr is a alias for spsr_elN where N is the current
+     * exception level. It is provided for here so the TCG msr/mrs
+     * implementation can access one register. Care needs to be taken
+     * to ensure the banked_spsr[] is also updated.
+     */
     uint32_t spsr;
 
     /* Banked registers.  */
-- 
2.3.0

Re: [Qemu-devel] [PATCH 6/6] target-arm/cpu.h: document why env->spsr exists

[Greg Bellows]

On Wed, Feb 25, 2015 at 10:02 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> I was getting very confused about the duplication of state. Perhaps we
> should just get rid of env->spsr and use helpers that understand the
> banking?
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 11845a6..d7fd13f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -155,6 +155,11 @@ typedef struct CPUARMState {
>         This contains all the other bits.  Use cpsr_{read,write} to access
>         the whole CPSR.  */
>      uint32_t uncached_cpsr;
> +    /* The spsr is a alias for spsr_elN where N is the current

"an alias"

> +     * exception level. It is provided for here so the TCG msr/mrs

remove "for here"

> +     * implementation can access one register. Care needs to be taken
> +     * to ensure the banked_spsr[] is also updated.
> +     */
>      uint32_t spsr;
>
>      /* Banked registers.  */
> --
> 2.3.0
>
>

Otherwise...

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Re: [Qemu-devel] [PATCH 6/6] target-arm/cpu.h: document why env->spsr exists

[Peter Maydell]

On 25 February 2015 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
> I was getting very confused about the duplication of state. Perhaps we
> should just get rid of env->spsr and use helpers that understand the
> banking?

I've already disagreed with this. I would suggest putting
tentative questions about future direction of the codebase
below the '---' rather than in the commit log :-)


> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 11845a6..d7fd13f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -155,6 +155,11 @@ typedef struct CPUARMState {
>         This contains all the other bits.  Use cpsr_{read,write} to access
>         the whole CPSR.  */
>      uint32_t uncached_cpsr;
> +    /* The spsr is a alias for spsr_elN where N is the current
> +     * exception level.

This isn't true for AArch32 (which has multiple different SPSRs
any of which might be the one in env->spsr when we're at EL1).

-- PMM