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

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

[Alex Bennée]

This is an update to the series I posted last week addressing some of
the comments so far.

The main changes to this series are:

v2

  - Save/Restore MP STATE
    - no longer needs CAP_MP_STATE at start
    - re-uses cpu->powered_off for storing state (no stream ABI change)
    - kvm_enabled() runtime check (although ioctl still in #if defined)
  - Save/Restore SPSR
    - use the correct bank_number for aarch32
    - only tweak SPSR for elevated exception levels
  - arm_giv_kvm
    - add Christoffer's Acked-by:

The only question up in the air at the moment is defining a new
constant for the MP_STATE to represent powered off which I'm going to
look at when I re-spin the kernel series.

Branch: https://github.com/stsquad/qemu/tree/migration/fixes-v3

Alex Bennée (5):
  target-arm: kvm: save/restore mp state
  hw/intc: arm_gic_kvm.c restore config first
  hw/char: pl011 don't keep setting the IRQ if nothing changed
  target-arm: kvm64 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/kvm64.c    | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
 target-arm/machine.c  |  29 ++++++++++++++
 5 files changed, 151 insertions(+), 11 deletions(-)

-- 
2.3.1

[Qemu-devel] [PATCH v2 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. We then save this state into the cpu_powered TCG state
to avoid changing the serialisation format.

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

---
v2
  - make mpstate field runtime dependant (kvm_enabled())
  - drop initial KVM_CAP_MP_STATE requirement
  - re-use cpu_powered instead of new field

diff --git a/target-arm/machine.c b/target-arm/machine.c
index 9446e5a..185f9a2 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = {
     .put = put_cpsr,
 };
 
+
 static void cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque)
             /* This should never fail */
             abort();
         }
+#if defined CONFIG_KVM
+        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
+            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();
+            }
+            cpu->powered_off =
+                (mp_state.mp_state == KVM_MP_STATE_RUNNABLE)
+                ? false : true;
+        }
+#endif
     } else {
         if (!write_cpustate_to_list(cpu)) {
             /* This should never fail. */
@@ -222,6 +237,20 @@ static int cpu_post_load(void *opaque, int version_id)
          * we're using it.
          */
         write_list_to_cpustate(cpu);
+#if defined CONFIG_KVM
+        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
+            struct kvm_mp_state mp_state = {
+                .mp_state =
+                cpu->powered_off ? KVM_MP_STATE_HALTED : KVM_MP_STATE_RUNNABLE
+            };
+            int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
+            if (ret) {
+                fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
+                        __func__, ret, strerror(ret));
+                return -1;
+            }
+        }
+#endif
     } else {
         if (!write_list_to_cpustate(cpu)) {
             return -1;
-- 
2.3.1

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]

On Wed, Mar 4, 2015 at 8:35 AM, 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. We then save this state into the cpu_powered TCG state
> to avoid changing the serialisation format.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - make mpstate field runtime dependant (kvm_enabled())
>   - drop initial KVM_CAP_MP_STATE requirement
>   - re-use cpu_powered instead of new field
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9446e5a..185f9a2 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = {
>      .put = put_cpsr,
>  };
>
> +
>

remove ​extraneous space
​


>  static void cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque)
>              /* This should never fail */
>              abort();
>          }
> +#if defined CONFIG_KVM
>

​The convention for ifdefing KVMatures appears to be more around the
feature rather than KVM_CONFIG.  For instance ​loo fek at
kvm_check_extension in kvm-all.c.  I may be missing something but if you
follow this convention this should be

    #ifdef KVM_CAP_MP_STATE



> +        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +            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();
> +            }
> +            cpu->powered_off =
> +                (mp_state.mp_state == KVM_MP_STATE_RUNNABLE)
> +                ? false : true;
> +        }
> +#endif
>      } else {
>          if (!write_cpustate_to_list(cpu)) {
>              /* This should never fail. */
> @@ -222,6 +237,20 @@ static int cpu_post_load(void *opaque, int version_id)
>           * we're using it.
>           */
>          write_list_to_cpustate(cpu);
> +#if defined CONFIG_KVM
> +        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +            struct kvm_mp_state mp_state = {
> +                .mp_state =
> +                cpu->powered_off ? KVM_MP_STATE_HALTED :
> KVM_MP_STATE_RUNNABLE
> +            };
> +            int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE,
> &mp_state);
> +            if (ret) {
> +                fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
> +                        __func__, ret, strerror(ret));
> +                return -1;
> +            }
> +        }
> +#endif
>      } else {
>          if (!write_list_to_cpustate(cpu)) {
>              return -1;
> --
> 2.3.1
>
>
> ​Besides these the above nits...

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

[-- Attachment #2: Type: text/html, Size: 5212 bytes --]

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

[Peter Maydell]

On 4 March 2015 at 14:35, 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. We then save this state into the cpu_powered TCG state
> to avoid changing the serialisation format.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - make mpstate field runtime dependant (kvm_enabled())
>   - drop initial KVM_CAP_MP_STATE requirement
>   - re-use cpu_powered instead of new field
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9446e5a..185f9a2 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = {
>      .put = put_cpsr,
>  };
>
> +
>  static void cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque)
>              /* This should never fail */
>              abort();
>          }
> +#if defined CONFIG_KVM
> +        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +            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();
> +            }
> +            cpu->powered_off =
> +                (mp_state.mp_state == KVM_MP_STATE_RUNNABLE)
> +                ? false : true;

Ternary operator to produce a true-or-false result is a bit
redundant...

> +        }
> +#endif

Why is this in pre-save/post-load rather than in the
kvm_arch_get/put_registers functions like all the other
syncing code?

-- PMM

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

[Alex Bennée]

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

> On 4 March 2015 at 14:35, 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. We then save this state into the cpu_powered TCG state
>> to avoid changing the serialisation format.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - make mpstate field runtime dependant (kvm_enabled())
>>   - drop initial KVM_CAP_MP_STATE requirement
>>   - re-use cpu_powered instead of new field
>>
>> diff --git a/target-arm/machine.c b/target-arm/machine.c
>> index 9446e5a..185f9a2 100644
>> --- a/target-arm/machine.c
>> +++ b/target-arm/machine.c
>> @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = {
>>      .put = put_cpsr,
>>  };
>>
>> +
>>  static void cpu_pre_save(void *opaque)
>>  {
>>      ARMCPU *cpu = opaque;
>> @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque)
>>              /* This should never fail */
>>              abort();
>>          }
>> +#if defined CONFIG_KVM
>> +        if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
>> +            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();
>> +            }
>> +            cpu->powered_off =
>> +                (mp_state.mp_state == KVM_MP_STATE_RUNNABLE)
>> +                ? false : true;
>
> Ternary operator to produce a true-or-false result is a bit
> redundant...
>
>> +        }
>> +#endif
>
> Why is this in pre-save/post-load rather than in the
> kvm_arch_get/put_registers functions like all the other
> syncing code?

Yeah the #ifdefs should have waved the red flag - I'll move it ;-)

>
> -- PMM

-- 
Alex Bennée

[Qemu-devel] [PATCH v2 2/6] hw/intc: arm_gic_kvm.c restore config first

[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>
Acked-by: Christoffer Dall <christoffer.dall@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.1

Re: [Qemu-devel] [PATCH v2 2/6] hw/intc: arm_gic_kvm.c restore config first

[Greg Bellows]

On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée <alex.bennee@linaro.org> 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>
>
> 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.1
>
>

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

[Qemu-devel] [PATCH v2 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.1

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

[Greg Bellows]

On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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.1
>
>

Did you find the source of the additional IRQs? This seems like it
could be potentially be hiding an issue.  From looking at the code, it
appears that one source of the additional updates is in pl011_read,
where we possibly do an update without a change in int_level.  I
wonder if finding and fixing the updates is a better approach.

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

[Peter Maydell]

On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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);
> +    }
>  }

Consider this sequence of events:

 * the guest does something causing the interrupt to
   be asserted; int_level and int_enabled are 1, and
   current_irq is also now 1. We call qemu_set_irq()
   to raise the interrupt with the GIC
 * we migrate the guest to another host
 * on the receiving end, QEMU is in a cleanly reset
   state, and so current_irq, int_level and int_enabled
   are all zero before incoming data arrives
 * int_level and int_enabled are both set to 1 from
   the incoming data stream
 * the GIC itself is set to the "interrupt is
   asserted" state by its own incoming data
 * current_irq remains zero, because it's not migrated
 * the guest is resumed, and does something to deassert
   the interrupt. the new 'flags' value is zero
 * because flags == s->current_irq, we don't call
   qemu_set_irq, and so we've just dropped the deassert
   of this interrupt on the floor.

-- PMM

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

[Peter Maydell]

On 12 March 2015 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 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.

> Consider this sequence of events:

Incidentally, the code review rule of thumb that led me to
construct that scenario is:
 * state inside the device struct which doesn't correspond
   to real hardware state is suspicious
 * state which doesn't have any handling on migration
   save/restore is doubly suspicious
...and then it was just a matter of "find the situation
where this is broken" :-)

If we want to avoid making syscalls back into KVM it might
be better to attack the problem in the GIC object rather
than in all the devices that might be connected to it.
In general QEMU devices tend to assume they can just
always call qemu_set_irq() and it's the other end's
job to avoid doing anything too expensive in that situation.

-- PMM

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

[Alex Bennée]

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

> On 12 March 2015 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> 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.
>
>> Consider this sequence of events:
>
> Incidentally, the code review rule of thumb that led me to
> construct that scenario is:
>  * state inside the device struct which doesn't correspond
>    to real hardware state is suspicious
>  * state which doesn't have any handling on migration
>    save/restore is doubly suspicious
> ...and then it was just a matter of "find the situation
> where this is broken" :-)
>
> If we want to avoid making syscalls back into KVM it might
> be better to attack the problem in the GIC object rather
> than in all the devices that might be connected to it.
> In general QEMU devices tend to assume they can just
> always call qemu_set_irq() and it's the other end's
> job to avoid doing anything too expensive in that situation.

Fair enough - I'll drop this patch on the re-spin

>
> -- PMM

-- 
Alex Bennée

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

[Alex Bennée]

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.1

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

[Greg Bellows]

On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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++;

What does this increment do?  It appears that it just gets thrown away
on the next iteration of the loop unless ioctl(SET) return something,
but maybe I am missing something.

> +    }
> +
> +    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.1
>
>

[Qemu-devel] [PATCH v2 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
v3
  - Use the correct bank index function for setting/getting env->spsr
  - only deal with spsrs in elevated exception levels

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index c60e989..45e5c3f 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,27 @@ 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 (el > 0) {
+        if (is_a64(env)) {
+            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];
+        } else {
+            i = bank_number(env->uncached_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 +272,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 +345,35 @@ 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 (el > 0) {
+        if (is_a64(env)) {
+            g_assert(el == 1);
+            /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+            env->banked_spsr[0] = env->banked_spsr[1];
+            i = aarch64_banked_spsr_index(el);
+        } else {
+            i = bank_number(env->uncached_cpsr & CPSR_M);
+        }
+        env->spsr = env->banked_spsr[i];
+    }
+
     /* Advanced SIMD and FP registers */
     for (i = 0; i < 32; i++) {
         reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-- 
2.3.1

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

[Christoffer Dall]

On Wed, Mar 04, 2015 at 02:35:52PM +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
> v3
>   - Use the correct bank index function for setting/getting env->spsr
>   - only deal with spsrs in elevated exception levels
> 
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index c60e989..45e5c3f 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,27 @@ 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 (el > 0) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */

not sure about the 'for aarch64' comment; I would say that it's for
aarch32 support.  Also, you can drop the ATM, since this is user space
ABI that we don't change easily.


don't you need to do env->banked_spsr[0] = env->spsr first?

> +            env->banked_spsr[1] = env->banked_spsr[0];


> +        } else {
> +            i = bank_number(env->uncached_cpsr & CPSR_M);
> +            env->banked_spsr[i] = env->spsr;

so here we don't need to worry about banked_spsr[1] = banked_spsr[0]
because banked_spsr[0] is meaningless for 32-bit state and we only sync
banked_spsr[1] and up to KVM, correct?  I think this is what may deserve
a comment.

> +        }
> +    }
> +
>      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 +272,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 +345,35 @@ 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

privilege

> +     * there are some transitions possible which can access old state
> +     * so it is worth keeping them all.
> +     */

dubious comment overall

>      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 (el > 0) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */

same as above

> +            env->banked_spsr[0] = env->banked_spsr[1];
> +            i = aarch64_banked_spsr_index(el);
> +        } else {
> +            i = bank_number(env->uncached_cpsr & CPSR_M);

same potential place for comment as above.

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

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

[Greg Bellows]

On Mon, Mar 9, 2015 at 8:26 AM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Wed, Mar 04, 2015 at 02:35:52PM +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
>> v3
>>   - Use the correct bank index function for setting/getting env->spsr
>>   - only deal with spsrs in elevated exception levels
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index c60e989..45e5c3f 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,27 @@ 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 (el > 0) {
>> +        if (is_a64(env)) {
>> +            g_assert(el == 1);
>> +            /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
>
> not sure about the 'for aarch64' comment; I would say that it's for
> aarch32 support.  Also, you can drop the ATM, since this is user space
> ABI that we don't change easily.
>
>
> don't you need to do env->banked_spsr[0] = env->spsr first?

I agree with Christoffer, env->spsr actually has the most current
value so you need to sync up with it before sending it out.

>
>> +            env->banked_spsr[1] = env->banked_spsr[0];
>
>
>> +        } else {
>> +            i = bank_number(env->uncached_cpsr & CPSR_M);
>> +            env->banked_spsr[i] = env->spsr;
>
> so here we don't need to worry about banked_spsr[1] = banked_spsr[0]
> because banked_spsr[0] is meaningless for 32-bit state and we only sync
> banked_spsr[1] and up to KVM, correct?  I think this is what may deserve
> a comment.
>
>> +        }
>> +    }
>> +
>>      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 +272,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 +345,35 @@ 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
>
> privilege
>
>> +     * there are some transitions possible which can access old state
>> +     * so it is worth keeping them all.
>> +     */
>
> dubious comment overall
>
>>      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 (el > 0) {
>> +        if (is_a64(env)) {
>> +            g_assert(el == 1);
>> +            /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
>
> same as above

If Christoffer's comment is referring to updating env->spsr, it occurs
below based on 'i'.

>
>> +            env->banked_spsr[0] = env->banked_spsr[1];
>> +            i = aarch64_banked_spsr_index(el);
>> +        } else {
>> +            i = bank_number(env->uncached_cpsr & CPSR_M);
>
> same potential place for comment as above.
>
>> +        }
>> +        env->spsr = env->banked_spsr[i];
>> +    }
>> +
>>      /* Advanced SIMD and FP registers */
>>      for (i = 0; i < 32; i++) {
>>          reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
>> --
>> 2.3.1
>>
>

[Qemu-devel] [PATCH v2 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.1

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

[Peter Maydell]

On 4 March 2015 at 23:35, 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?

Doesn't seem worth changing the current working code to something
else that's strictly less efficient... spsr is by no means the
only banked-by-mode register, and it works just like all the others.

-- PMM

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

[Alex Bennée]

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

> On 4 March 2015 at 23:35, 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?
>
> Doesn't seem worth changing the current working code to something
> else that's strictly less efficient... spsr is by no means the
> only banked-by-mode register, and it works just like all the others.

Fair enough. I just wanted to make it clear it was a cached value for
the benefit of TCG.

>
> -- PMM

-- 
Alex Bennée