[Qemu-devel] [RFC 0/4] target-arm: KVM to TCG migration

[Qemu-devel] [RFC 0/4] target-arm: KVM to TCG migration

[Alvise Rigo]

QEMU at the moment doesn't allow the migration between KVM and TCG, in
both directions. This limitation is due first of all to the different set
of coprocessor registers supported by KVM and TCG, but also in the way
the coprocessors values are copied from the incoming data to the local QEMU
structures.
This set of patches is focused on the KVM to TCG migration using the
CortexA15 processor in both sides and tries to enhance the migration of
cp registers in order to prevent QEMU from failing migration in some
particular situations.
To illustrate the changes proposed with this patch series, we will call
"generator" a register that generates other registers in the TCG
coprocessor hash table, such a register can have one or more wildcarded
fields in its definition, or can have an CP_ARM_STATE_BOTH state (in
this case the generator is the AARCH64 view of the register). With the
term "child" a register generated by a generator will be indicated.
It turns out that KVM tries to migrate some child registers (that in TCG
are marked as CP_ARM_NO_MIGRATE) leading to a fault during the copy of
cp registers.

These patch series modify the coprocessor migration with the following steps:
- during the copy of the incoming cp registers inside cpu_post_load a list
  is kept containing the registers that haven't matched any of the registers
  inside the cpreg_list.
- only if the previous step has generated a not empty list, we try to
  handle the copy of these registers "by hand", verifying for every
  register in the list if it is:
    - already present in the hash table
    - flagged with CP_ARM_NO_MIGRATE
In this case if the register has a generator which is migratable we raw
write its value and then we remove it from the list, otherwise we simply
remove it from the list (because still a not migratable register for TCG).
When we raw_write the value of the register in the hash table we take
care of marking the generator register as already "updated" (modifying the
flag skip_cpreglist to true) because it has the same value of its children.
This will prevent the method write_list_to_cpustate from overwriting the value
of a child register with the default value of its parent: this is what
happens when KVM migrates a child register and not its generator.

Finally, it is also necessary to not update those registers with
constant values, because KVM and TCG in some cases don't agree with their
default values; this is done setting the skip_cpreglist flag to true for every
ARM_CP_CONST registers.
This is the case of constant registers like ID_PFR0 (the Jazelle bit for
instance is set in TCG and not set in KVM) and AUXCR (KVM read its
value directly form the processor while in TCG is not set); here a much
cleaner solution is needed.

All this "handling" is confined inside the method handle_cpreg_kvm2tcg_migration.
This method will be called only if is detected that we are migrating from
KVM to TCG (a new flag inside ARMCPU was added to address this purpose).
If we are performing a normal migration, be it TCG<->TCG or KVM<->KVM, nothing
will change and the usual copy of cp registers and aborting criteria will hold.

The other way of migration is not yet covered by this patch set. KVM lacks of
several coprocessor registers that in TCG are instead supported; solving this
open issue will be the main challenge of the TCG to KVM migration.

This set of patches has been tested by booting a guest running the
Linux 3.13 kernel in a host using KVM. Two types of hosts have been used:
ARM FastModels and the ARM Chromebook. Then, in the middle of the kernel
boot, the guest was migrated to a x86 host running TCG.
The machine model used is vexpress-a15 with cortex-a15 as CPU.

Alvise Rigo (4):
  Fix issue affecting get_int32_le() in vmstate.c
  Added flag in ARMCPU to track last execution mode
  Add l2ctlr cp register to CPUARMState
  Relevant changes to enable KVM to TCG migration

 target-arm/cpu-qom.h |   3 +
 target-arm/cpu.c     |  22 +++++--
 target-arm/cpu.h     |  43 +++++++++++++
 target-arm/helper.c  | 167 +++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/machine.c |  46 ++++++++++----
 vmstate.c            |   8 +--
 6 files changed, 263 insertions(+), 26 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [RFC 1/4] Fix issue affecting get_int32_le() in vmstate.c

[Alvise Rigo]

The method is not behaving in the way it's supposed to. It should return
the new value only if it's less than the actual one.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 vmstate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vmstate.c b/vmstate.c
index 284b080..038b274 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = {
 
 static int get_int32_le(QEMUFile *f, void *pv, size_t size)
 {
-    int32_t *old = pv;
-    int32_t new;
-    qemu_get_sbe32s(f, &new);
+    int32_t old = *(int32_t *)pv;
+    int32_t *new = pv;
+    qemu_get_sbe32s(f, new);
 
-    if (*old <= new) {
+    if (*new <= old) {
         return 0;
     }
     return -EINVAL;
-- 
1.8.3.2

Re: [Qemu-devel] [RFC 1/4] Fix issue affecting get_int32_le() in vmstate.c

[Eduardo Habkost]

On Tue, Feb 25, 2014 at 05:52:47PM +0100, Alvise Rigo wrote:
> The method is not behaving in the way it's supposed to. It should return
> the new value only if it's less than the actual one.
> 
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  vmstate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/vmstate.c b/vmstate.c
> index 284b080..038b274 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = {
>  
>  static int get_int32_le(QEMUFile *f, void *pv, size_t size)
>  {
> -    int32_t *old = pv;
> -    int32_t new;
> -    qemu_get_sbe32s(f, &new);
> +    int32_t old = *(int32_t *)pv;
> +    int32_t *new = pv;
> +    qemu_get_sbe32s(f, new);

You are now changing the value in *(int32_t*)pv on every call, instead
of simply ensuring the value is less than the current value. This
doesn't seem to be the intended behavior of
vmstate_info_int32_le/VMSTATE_INT32_LE.


>  
> -    if (*old <= new) {
> +    if (*new <= old) {
>          return 0;
>      }
>      return -EINVAL;
> -- 
> 1.8.3.2
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 1/4] Fix issue affecting get_int32_le() in vmstate.c

[Peter Maydell]

On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> The method is not behaving in the way it's supposed to. It should return
> the new value only if it's less than the actual one.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>

This clashes with David Gilbert's patch to this function:

http://patchwork.ozlabs.org/patch/319717/

which I suspect is fixing the same bug you're trying
to address here.

thanks
-- PMM

Re: [Qemu-devel] [RFC 1/4] Fix issue affecting get_int32_le() in vmstate.c

[Juan Quintela]

Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> The method is not behaving in the way it's supposed to. It should return
> the new value only if it's less than the actual one.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>

See David patch of this function.  There were a bug, we were doing the
wrong comparison.  But we expect not to chang the local value.  We just
want the the one that cames is less or equal that the current value
(think of an array size, it is a bad idea to try to read a bigger array
into a smaller one).

BTW, did you find this bug by testing or by code inspection?

thanks, Juan.

> ---
>  vmstate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/vmstate.c b/vmstate.c
> index 284b080..038b274 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = {
>  
>  static int get_int32_le(QEMUFile *f, void *pv, size_t size)
>  {
> -    int32_t *old = pv;
> -    int32_t new;
> -    qemu_get_sbe32s(f, &new);
> +    int32_t old = *(int32_t *)pv;
> +    int32_t *new = pv;
> +    qemu_get_sbe32s(f, new);
>  
> -    if (*old <= new) {
> +    if (*new <= old) {
>          return 0;
>      }
>      return -EINVAL;

Re: [Qemu-devel] [RFC 1/4] Fix issue affecting get_int32_le() in vmstate.c

[Peter Maydell]

On 25 February 2014 18:52, Juan Quintela <quintela@redhat.com> wrote:
> BTW, did you find this bug by testing or by code inspection?

Trying a KVM<->TCG migration of an ARM guest will likely
hit the bug (because the number of coprocessor registers
on either end differs).

thanks
-- PMM

[Qemu-devel] [RFC 2/4] Added flag in ARMCPU to track last execution mode

[Alvise Rigo]

The value of this flag indicates the execution mode of the CPU prior the
migration. It is used to handle the KVM <-> TCG migration.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/cpu-qom.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index afbd422..6819bfc 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -102,6 +102,9 @@ typedef struct ARMCPU {
      */
     uint32_t kvm_target;
 
+    /* true if this cpu is using KVM. Read and set in cpu_pre/post_load */
+    bool running_kvm;
+
     /* The instance init functions for implementation-specific subclasses
      * set these fields to specify the implementation-dependent values of
      * various constant registers and reset values of non-constant
-- 
1.8.3.2

Re: [Qemu-devel] [RFC 2/4] Added flag in ARMCPU to track last execution mode

[Peter Maydell]

On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> The value of this flag indicates the execution mode of the CPU prior the
> migration. It is used to handle the KVM <-> TCG migration.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  target-arm/cpu-qom.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index afbd422..6819bfc 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -102,6 +102,9 @@ typedef struct ARMCPU {
>       */
>      uint32_t kvm_target;
>
> +    /* true if this cpu is using KVM. Read and set in cpu_pre/post_load */
> +    bool running_kvm;

This is definitely wrong. We should not care whether either
end of the migration connection is using KVM.

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/4] Added flag in ARMCPU to track last execution mode

[alvise rigo]

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

I see, but how can we know the type of migration (KVM to TCG or TCG to
KVM) which is taking place only looking at the incoming data? Of course,
I'm supposing that the two cases will require different handling and so
that distinguish the migration type (at the destination) has to be part
of the process.

thanks,
alvise



On Tue, Feb 25, 2014 at 7:19 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com>
> wrote:
> > The value of this flag indicates the execution mode of the CPU prior the
> > migration. It is used to handle the KVM <-> TCG migration.
> >
> > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> > ---
> >  target-arm/cpu-qom.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> > index afbd422..6819bfc 100644
> > --- a/target-arm/cpu-qom.h
> > +++ b/target-arm/cpu-qom.h
> > @@ -102,6 +102,9 @@ typedef struct ARMCPU {
> >       */
> >      uint32_t kvm_target;
> >
> > +    /* true if this cpu is using KVM. Read and set in cpu_pre/post_load
> */
> > +    bool running_kvm;
>
> This is definitely wrong. We should not care whether either
> end of the migration connection is using KVM.
>
> thanks
> -- PMM
>

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

Re: [Qemu-devel] [RFC 2/4] Added flag in ARMCPU to track last execution mode

[Peter Maydell]

On 26 February 2014 09:16, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> I see, but how can we know the type of migration (KVM to TCG or TCG to
> KVM) which is taking place only looking at the incoming data? Of course,
> I'm supposing that the two cases will require different handling and so
> that distinguish the migration type (at the destination) has to be part
> of the process.

That's my point -- the two cases should not require different handling:
you should just handle the migration and succeed if possible or
fail if not, based on the incnoming data alone.

thanks
-- PMM

[Qemu-devel] [RFC 3/4] Add l2ctlr cp register to CPUARMState

[Alvise Rigo]

Since the irq bit seems to not be updated, exclude it from the check done
while copying data during migration.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 6e7ce89..e8db00e 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
 }
 
 #ifndef CONFIG_USER_ONLY
-static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
 {
     /* Linux wants the number of processors from here.
      * Might as well set the interrupt-controller bit too.
      */
-    return ((smp_cpus - 1) << 24) | (1 << 23);
+    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
+}
+
+static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    int smp_cpus_new = ((value >> 24) & 3);
+    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
+
+    if (smp_cpus_new != smp_cpus_old) {
+        return;
+    }
+
+    env->cp15.c9_l2ctlr = value;
 }
 #endif
 
 static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
 #ifndef CONFIG_USER_ONLY
     { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 2,
-      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
-      .writefn = arm_cp_write_ignore, },
+      .access = PL1_RW, .resetvalue = 0,
+      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
 #endif
     { .name = "L2ECTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 3,
       .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
-- 
1.8.3.2

Re: [Qemu-devel] [RFC 3/4] Add l2ctlr cp register to CPUARMState

[Peter Maydell]

On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> Since the irq bit seems to not be updated, exclude it from the check done
> while copying data during migration.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  target-arm/cpu.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 6e7ce89..e8db00e 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
>  }
>
>  #ifndef CONFIG_USER_ONLY
> -static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
>  {
>      /* Linux wants the number of processors from here.
>       * Might as well set the interrupt-controller bit too.
>       */
> -    return ((smp_cpus - 1) << 24) | (1 << 23);
> +    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
> +}
> +
> +static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    int smp_cpus_new = ((value >> 24) & 3);
> +    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
> +
> +    if (smp_cpus_new != smp_cpus_old) {
> +        return;
> +    }
> +
> +    env->cp15.c9_l2ctlr = value;
>  }
>  #endif
>
>  static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
>  #ifndef CONFIG_USER_ONLY
>      { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 2,
> -      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
> -      .writefn = arm_cp_write_ignore, },
> +      .access = PL1_RW, .resetvalue = 0,
> +      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
>  #endif

Why are you changing the behaviour of this register from
read-only/writes-ignored to permitting writes? This looks
wrong.

thanks
-- PMM

Re: [Qemu-devel] [RFC 3/4] Add l2ctlr cp register to CPUARMState

[alvise rigo]

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

This was a polite way to alter the value of the register in order to not
fail the
migration when KVM migrates a value different from the TCG one.
KVM in particular does not set the irq bit while TCG does.

alvise


On Tue, Feb 25, 2014 at 7:22 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com>
> wrote:
> > Since the irq bit seems to not be updated, exclude it from the check done
> > while copying data during migration.
> >
> > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> > ---
> >  target-arm/cpu.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 6e7ce89..e8db00e 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
> >  }
> >
> >  #ifndef CONFIG_USER_ONLY
> > -static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo
> *ri)
> > +static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo
> *opaque)
> >  {
> >      /* Linux wants the number of processors from here.
> >       * Might as well set the interrupt-controller bit too.
> >       */
> > -    return ((smp_cpus - 1) << 24) | (1 << 23);
> > +    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
> > +}
> > +
> > +static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > +                           uint64_t value)
> > +{
> > +    int smp_cpus_new = ((value >> 24) & 3);
> > +    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
> > +
> > +    if (smp_cpus_new != smp_cpus_old) {
> > +        return;
> > +    }
> > +
> > +    env->cp15.c9_l2ctlr = value;
> >  }
> >  #endif
> >
> >  static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
> >  #ifndef CONFIG_USER_ONLY
> >      { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2
> = 2,
> > -      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
> > -      .writefn = arm_cp_write_ignore, },
> > +      .access = PL1_RW, .resetvalue = 0,
> > +      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
> >  #endif
>
> Why are you changing the behaviour of this register from
> read-only/writes-ignored to permitting writes? This looks
> wrong.
>
> thanks
> -- PMM
>

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

Re: [Qemu-devel] [RFC 3/4] Add l2ctlr cp register to CPUARMState

[Peter Maydell]

On 26 February 2014 09:17, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> This was a polite way to alter the value of the register in order to not
> fail the
> migration when KVM migrates a value different from the TCG one.
> KVM in particular does not set the irq bit while TCG does.

You mean the "interrupt controller" bit 23? That seems like a bug
in the KVM emulation of this register which should be fixed in KVM.
Alternatively if it's a documentation error and the actual hardware
A15 doesn't set this bit (ie if it's not set when you're running outside
KVM) then we should fix the TCG emulation not to set the bit.

thanks
-- PMM

[Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Alvise Rigo]

CPUARMState:
* added adfsr cp register.
* added aifsr cp register.
These registers have been added because they are migrated by KVM. This prevents
the migration from failing when trying to copy their values.

ARMCPRegInfo:
* added a pointer to the parent that generated the register (if any).
* a flag to inform that we have already copied the value of the register:
  this prevents the register from being overwritten by the parent register
  value, which could be not set.

helper.c:
* added mechanism to track the cp register "parent".
* compare_cpreg_array(): compare the incoming cp registers with the
  cpreg_list keeping a list of the registers that do not succeeded the
  match.
* handle_cpreg_kvm2tcg_migration(): try to solve the mismatch of
  cp registers coming from KVM; without this additional step the
  migration would fail even if it's feasible.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/cpu.h     |  43 +++++++++++++
 target-arm/helper.c  | 167 +++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/machine.c |  46 ++++++++++----
 3 files changed, 238 insertions(+), 18 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3c8a2db..a97246d 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -184,6 +184,8 @@ typedef struct CPUARMState {
                         MPU write buffer control.  */
         uint32_t c5_insn; /* Fault status registers.  */
         uint32_t c5_data;
+        uint32_t c5_adfsr;
+        uint32_t c5_aifsr;
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;
@@ -197,6 +199,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_l2ctlr; /* L2 Control Register */
         uint32_t c12_vbar; /* vector base address register */
         uint32_t c13_fcse; /* FCSE PID.  */
         uint32_t c13_context; /* Context ID.  */
@@ -867,6 +870,15 @@ struct ARMCPRegInfo {
     uint8_t opc0;
     uint8_t opc1;
     uint8_t opc2;
+    /* When migrating this flag tells if the register's value has already
+     * been copied. This is used in some unlikely cases where KVM migrates
+     * a register that in TCG is generated by a wildcarded or
+     * ARM_CP_STATE_BOTH parent (hence a not migratable register);
+     * in those cases, we will set the field "parent" to point to the
+     * generating register. Most of the time this field can stay false.
+     * */
+    bool skip_cpreglist;
+    ARMCPRegInfo *parent;
     /* Execution state in which this register is visible: ARM_CP_STATE_* */
     int state;
     /* Register type: ARM_CP_* bits/values */
@@ -995,8 +1007,39 @@ bool write_list_to_cpustate(ARMCPU *cpu);
  * Note that we do not stop early on failure -- we will attempt
  * reading all registers in the list.
  */
+
 bool write_cpustate_to_list(ARMCPU *cpu);
 
+typedef struct coproc_pair {
+    uint64_t id;
+    uint64_t val;
+} cp_pair;
+
+#ifndef KVM_REG_ARM_CORE
+#define KVM_REG_ARM_CORE                (0x0010 << CP_REG_ARM_COPROC_SHIFT)
+#endif
+#ifndef KVM_REG_ARM_DEMUX
+#define KVM_REG_ARM_DEMUX               (0x0011 << CP_REG_ARM_COPROC_SHIFT)
+#endif
+#ifndef KVM_REG_ARM_VFP
+#define KVM_REG_ARM_VFP                 (0x0012 << CP_REG_ARM_COPROC_SHIFT)
+#endif
+/* This method compares two arrays of coprocessor registers; in case of
+ * migration, the array b contains the incoming values. If a not NULL
+ * pointer to an empty list is provided, the method will fill it with
+ * pairs <id, value> of the registers that are in b but not in a.
+ * */
+int compare_cpreg_array(uint32_t len_a, uint64_t *idx_a, uint64_t *val_a,
+                        uint32_t len_b, uint64_t *idx_b, uint64_t *val_b,
+                        GList **mis_list);
+
+/* This method is supposed to take in input those registers which failed the
+ * copy in compare_cpreg_array and tries to handle them in such a way to allow
+ * migration from KVM to the TCG processor.
+ * It returns the number of registers not handled, -1 in case of error.
+ * */
+int handle_cpreg_kvm2tcg_migration(ARMCPU *cpu, GList *mis_list, uint32_t len);
+
 /* Does the core conform to the the "MicroController" profile. e.g. Cortex-M3.
    Note the M in older cores (eg. ARM7TDMI) stands for Multiply. These are
    conventional cores (ie. Application or Realtime profile).  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1b111b6..2abe169 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -197,7 +197,7 @@ bool write_list_to_cpustate(ARMCPU *cpu)
             ok = false;
             continue;
         }
-        if (ri->type & ARM_CP_NO_MIGRATE) {
+        if (ri->type & ARM_CP_NO_MIGRATE || (ri->skip_cpreglist)) {
             continue;
         }
         /* Write value and confirm it reads back as written
@@ -1213,6 +1213,10 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
     { .name = "DFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c6_data),
       .resetvalue = 0, },
+    { .name = "ADFSR", .cp = 15, .crn = 5, .crm = 1, .opc1 = 0, .opc2 = 0,
+      .access = PL1_R, .fieldoffset = offsetof(CPUARMState, cp15.c5_adfsr) },
+    { .name = "AIFSR", .cp = 15, .crn = 5, .crm = 1, .opc1 = 0, .opc2 = 1,
+      .access = PL1_R, .fieldoffset = offsetof(CPUARMState, cp15.c5_aifsr) },
     REGINFO_SENTINEL
 };
 
@@ -1922,8 +1926,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 }
 
 static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
-                                   void *opaque, int state,
-                                   int crm, int opc1, int opc2)
+                                   void *opaque, int state, int crm, int opc1,
+                                   int opc2, ARMCPRegInfo **parent)
 {
     /* Private utility function for define_one_arm_cp_reg_with_opaque():
      * add a single reginfo struct to the hash table.
@@ -1931,6 +1935,12 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
     uint32_t *key = g_new(uint32_t, 1);
     ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
     int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
+
+    /* these fields are used when migrating from TCG to KVM or the other way
+     * around*/
+    r2->skip_cpreglist = false;
+    r2->parent = NULL;
+
     if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA32) {
         /* The AArch32 view of a shared register sees the lower 32 bits
          * of a 64 bit backing field. It is not migratable as the AArch64
@@ -1945,6 +1955,9 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
             r2->fieldoffset += sizeof(uint32_t);
         }
 #endif
+        if (*parent != NULL) {
+            r2->parent = *parent;
+        }
     }
     if (state == ARM_CP_STATE_AA64) {
         /* To allow abbreviation of ARMCPRegInfo
@@ -1979,6 +1992,21 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         ((r->opc1 == CP_ANY) && opc1 != 0) ||
         ((r->opc2 == CP_ANY) && opc2 != 0)) {
         r2->type |= ARM_CP_NO_MIGRATE;
+        if (*parent != NULL) {
+            r2->parent = *parent;
+        }
+    }
+
+    /* Check if this register is going to generate other "child"
+     * registers, in this case set the parent pointer.
+     * */
+    bool crm_cond   = (r->crm != CP_ANY)  ? true : (crm == 0);
+    bool opc1_cond  = (r->opc1 != CP_ANY) ? true : (opc1 == 0);
+    bool opc2_cond  = (r->opc2 != CP_ANY) ? true : (opc2 == 0);
+    bool state_cond = (state == ARM_CP_STATE_AA64 &&
+                      r->state == ARM_CP_STATE_BOTH);
+    if ((crm_cond && opc1_cond && opc2_cond) || state_cond) {
+        *parent = r2;
     }
 
     /* Overriding of an existing definition must be explicitly
@@ -2094,22 +2122,149 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
     }
     /* Bad type field probably means missing sentinel at end of reg list */
     assert(cptype_valid(r->type));
+    ARMCPRegInfo *parent = NULL;
     for (crm = crmmin; crm <= crmmax; crm++) {
         for (opc1 = opc1min; opc1 <= opc1max; opc1++) {
             for (opc2 = opc2min; opc2 <= opc2max; opc2++) {
-                for (state = ARM_CP_STATE_AA32;
-                     state <= ARM_CP_STATE_AA64; state++) {
+                for (state = ARM_CP_STATE_AA64;
+                     state >= ARM_CP_STATE_AA32; state--) {
                     if (r->state != state && r->state != ARM_CP_STATE_BOTH) {
                         continue;
                     }
                     add_cpreg_to_hashtable(cpu, r, opaque, state,
-                                           crm, opc1, opc2);
+                                           crm, opc1, opc2, &parent);
                 }
             }
         }
     }
 }
 
+static void add_pair_to_list(uint64_t id, uint64_t val, GList **list)
+{
+    cp_pair *new = g_malloc(sizeof(cp_pair));
+    new->id = id;
+    new->val = val;
+    *list = g_list_prepend(*list, new);
+}
+
+static void set_skip_cpreglist(gpointer key, gpointer value, gpointer type)
+{
+    ARMCPRegInfo *reg = (ARMCPRegInfo *)value;
+
+    if ((type != NULL && reg->type & *(int *)type) || type == NULL) {
+        reg->skip_cpreglist = true;
+    }
+}
+
+static void unset_skip_cpreglist(gpointer key, gpointer value, gpointer type)
+{
+    ARMCPRegInfo *reg = (ARMCPRegInfo *)value;
+
+    if ((type != NULL && reg->type & *(int *)type) || type == NULL) {
+        reg->skip_cpreglist = false;
+    }
+}
+
+int compare_cpreg_array(uint32_t len_a, uint64_t *idx_a, uint64_t *val_a,
+                        uint32_t len_b, uint64_t *idx_b, uint64_t *val_b,
+                        GList **mis_list)
+{
+    int i = 0, j = 0, ret = 0;
+
+    while (i < len_a && j < len_b) {
+        if (idx_b[j] > idx_a[i]) {
+            i++;
+            continue;
+        }
+        if (idx_b[j] < idx_a[i]) {
+            if (mis_list != NULL) {
+                add_pair_to_list(idx_b[j], val_b[j], mis_list);
+            }
+            j++;
+
+            ret = -1;
+            continue;
+        }
+        val_a[i] = val_b[j];
+        j++;
+        i++;
+    }
+
+    if ((i == len_a) && (j != len_b)) {
+        if (mis_list != NULL) {
+            for (i = j; i < len_b; i++) {
+                add_pair_to_list(idx_b[i], val_b[i], mis_list);
+            }
+        }
+
+        ret = -1;
+    }
+
+    return ret;
+}
+
+int handle_cpreg_kvm2tcg_migration(ARMCPU *cpu, GList *mis_list, uint32_t len)
+{
+    GList *ptr_l = mis_list;
+
+    /* restore skip_cpreglist flag from last migration */
+    g_hash_table_foreach(cpu->cp_regs, unset_skip_cpreglist, NULL);
+
+    while (ptr_l != NULL) {
+        GList *n = ptr_l->next;
+        cp_pair *el = (cp_pair *)ptr_l->data;
+        const ARMCPRegInfo *ri;
+        /*
+         * KVM migrates all the CSSIDR cp registers while QEMU doesn't.
+         * They don't follow the usual nomenclature.
+         **/
+        switch (el->id & CP_REG_ARM_COPROC_MASK) {
+        case KVM_REG_ARM_CORE:
+        case KVM_REG_ARM_DEMUX:
+        case KVM_REG_ARM_VFP:
+            g_free(el);
+            mis_list = g_list_delete_link(mis_list, ptr_l);
+
+            break;
+        default:
+            /* probably its value has already been copied because
+             * the register is covered by a wilcarded case or it's
+             * a register not migrated by TCG. In any case, the
+             * register type has to be ARM_CP_NO_MIGRATE.
+             */
+            ri = get_arm_cp_reginfo(cpu->cp_regs, kvm_to_cpreg_id(el->id));
+            if (ri) {
+                if ((ri->type & ARM_CP_NO_MIGRATE) == 0) {
+                    /* error */
+                    return -1;
+                }
+                /* If it has a migratable parent, we raw write its value,
+                 * otherwise we just remove it from the list since not
+                 * migratable.
+                 * */
+                if (ri->parent != NULL && !((*(ri->parent)).type &
+                                            ARM_CP_NO_MIGRATE)) {
+                    write_raw_cp_reg(&cpu->env, ri, el->val);
+                    (*(ri->parent)).skip_cpreglist = true;
+                }
+
+                g_free(el);
+                mis_list = g_list_delete_link(mis_list, ptr_l);
+            } else {
+                return -1;
+            }
+        }
+        ptr_l = n;
+    }
+
+    /* set all ARM_CP_CONST as already migrated, this will prevent the migration
+     * from failing when TCG and KVM constant registers do not match */
+    int type = ARM_CP_CONST;
+    g_hash_table_foreach(cpu->cp_regs, set_skip_cpreglist, &type);
+
+    return g_list_length(mis_list);
+}
+
 void define_arm_cp_regs_with_opaque(ARMCPU *cpu,
                                     const ARMCPRegInfo *regs, void *opaque)
 {
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 8f9e7d4..244ba41 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -155,11 +155,13 @@ static void cpu_pre_save(void *opaque)
     ARMCPU *cpu = opaque;
 
     if (kvm_enabled()) {
+        cpu->running_kvm = true;
         if (!write_kvmstate_to_list(cpu)) {
             /* This should never fail */
             abort();
         }
     } else {
+        cpu->running_kvm = false;
         if (!write_cpustate_to_list(cpu)) {
             /* This should never fail. */
             abort();
@@ -176,7 +178,6 @@ static void cpu_pre_save(void *opaque)
 static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
-    int i, v;
 
     /* Update the values list from the incoming migration data.
      * Anything in the incoming data which we don't know about is
@@ -186,22 +187,42 @@ static int cpu_post_load(void *opaque, int version_id)
      * incoming migration index list so we can match the values array
      * entries with the right slots in our own values array.
      */
+    GList *missing_regs = NULL;
+    if (compare_cpreg_array(cpu->cpreg_array_len, cpu->cpreg_indexes,
+                cpu->cpreg_values, cpu->cpreg_vmstate_array_len,
+                cpu->cpreg_vmstate_indexes, cpu->cpreg_vmstate_values,
+                                                    &missing_regs)) {
+        /* three different cases:
+         *          1. KVM to TCG migration
+         *          2. TCG to KVM migration
+         *          3. KVM to KVM / TCG to TCG migration*/
+        if (cpu->running_kvm && !kvm_enabled()) {
+            /* case 1. */
+            if (handle_cpreg_kvm2tcg_migration(cpu, missing_regs,
+                                 g_list_length(missing_regs))) {
+                g_list_free(missing_regs);
+
+                return -1;
+            }
+            /* update the value for future migrations */
+            cpu->running_kvm = false;
+        } else if (!cpu->running_kvm && kvm_enabled()) {
+            /* case 2. TODO */
+            cpu->running_kvm = true;
+            g_list_free(missing_regs);
+
+            return -1;
+        } else {
+            /* case 3. - regular migration, an error occurred */
+            g_list_free(missing_regs);
 
-    for (i = 0, v = 0; i < cpu->cpreg_array_len
-             && v < cpu->cpreg_vmstate_array_len; i++) {
-        if (cpu->cpreg_vmstate_indexes[v] > cpu->cpreg_indexes[i]) {
-            /* register in our list but not incoming : skip it */
-            continue;
-        }
-        if (cpu->cpreg_vmstate_indexes[v] < cpu->cpreg_indexes[i]) {
-            /* register in their list but not ours: fail migration */
             return -1;
         }
-        /* matching register, copy the value over */
-        cpu->cpreg_values[i] = cpu->cpreg_vmstate_values[v];
-        v++;
     }
 
+    /* free the list */
+    g_list_free(missing_regs);
+
     if (kvm_enabled()) {
         if (!write_list_to_kvmstate(cpu)) {
             return -1;
@@ -238,6 +259,7 @@ const VMStateDescription vmstate_arm_cpu = {
             .offset = 0,
         },
         VMSTATE_UINT32(env.spsr, ARMCPU),
+        VMSTATE_BOOL(running_kvm, ARMCPU),
         VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6),
         VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6),
         VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6),
-- 
1.8.3.2

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Peter Maydell]

On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> CPUARMState:
> * added adfsr cp register.
> * added aifsr cp register.
> These registers have been added because they are migrated by KVM. This prevents
> the migration from failing when trying to copy their values.

This should be done in a separate patch.

> ARMCPRegInfo:
> * added a pointer to the parent that generated the register (if any).
> * a flag to inform that we have already copied the value of the register:
>   this prevents the register from being overwritten by the parent register
>   value, which could be not set.
>
> helper.c:
> * added mechanism to track the cp register "parent".
> * compare_cpreg_array(): compare the incoming cp registers with the
>   cpreg_list keeping a list of the registers that do not succeeded the
>   match.
> * handle_cpreg_kvm2tcg_migration(): try to solve the mismatch of
>   cp registers coming from KVM; without this additional step the
>   migration would fail even if it's feasible.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  target-arm/cpu.h     |  43 +++++++++++++
>  target-arm/helper.c  | 167 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  target-arm/machine.c |  46 ++++++++++----
>  3 files changed, 238 insertions(+), 18 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3c8a2db..a97246d 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -184,6 +184,8 @@ typedef struct CPUARMState {
>                          MPU write buffer control.  */
>          uint32_t c5_insn; /* Fault status registers.  */
>          uint32_t c5_data;
> +        uint32_t c5_adfsr;
> +        uint32_t c5_aifsr;
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
>          uint32_t c6_insn; /* Fault address registers.  */
>          uint32_t c6_data;
> @@ -197,6 +199,7 @@ typedef struct CPUARMState {
>          uint32_t c9_pmxevtyper; /* perf monitor event type */
>          uint32_t c9_pmuserenr; /* perf monitor user enable */
>          uint32_t c9_pminten; /* perf monitor interrupt enables */
> +        uint32_t c9_l2ctlr; /* L2 Control Register */
>          uint32_t c12_vbar; /* vector base address register */
>          uint32_t c13_fcse; /* FCSE PID.  */
>          uint32_t c13_context; /* Context ID.  */
> @@ -867,6 +870,15 @@ struct ARMCPRegInfo {
>      uint8_t opc0;
>      uint8_t opc1;
>      uint8_t opc2;
> +    /* When migrating this flag tells if the register's value has already
> +     * been copied. This is used in some unlikely cases where KVM migrates
> +     * a register that in TCG is generated by a wildcarded or
> +     * ARM_CP_STATE_BOTH parent (hence a not migratable register);
> +     * in those cases, we will set the field "parent" to point to the
> +     * generating register. Most of the time this field can stay false.
> +     * */
> +    bool skip_cpreglist;

This reads to me like it's papering over a problem rather
than fixing it.

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[alvise rigo]

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

I agree that this is a sort of workaround, but it seems to me that a
proper solution is not possible without changing the ideas contemplated
now in the migration code.
Are we willing to accept some major changes in the code to embrace
this type of migration?

Thanks,
alvise


On Tue, Feb 25, 2014 at 7:25 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com>
> wrote:
> > CPUARMState:
> > * added adfsr cp register.
> > * added aifsr cp register.
> > These registers have been added because they are migrated by KVM. This
> prevents
> > the migration from failing when trying to copy their values.
>
> This should be done in a separate patch.
>
> > ARMCPRegInfo:
> > * added a pointer to the parent that generated the register (if any).
> > * a flag to inform that we have already copied the value of the register:
> >   this prevents the register from being overwritten by the parent
> register
> >   value, which could be not set.
> >
> > helper.c:
> > * added mechanism to track the cp register "parent".
> > * compare_cpreg_array(): compare the incoming cp registers with the
> >   cpreg_list keeping a list of the registers that do not succeeded the
> >   match.
> > * handle_cpreg_kvm2tcg_migration(): try to solve the mismatch of
> >   cp registers coming from KVM; without this additional step the
> >   migration would fail even if it's feasible.
> >
> > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> > ---
> >  target-arm/cpu.h     |  43 +++++++++++++
> >  target-arm/helper.c  | 167
> +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  target-arm/machine.c |  46 ++++++++++----
> >  3 files changed, 238 insertions(+), 18 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 3c8a2db..a97246d 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -184,6 +184,8 @@ typedef struct CPUARMState {
> >                          MPU write buffer control.  */
> >          uint32_t c5_insn; /* Fault status registers.  */
> >          uint32_t c5_data;
> > +        uint32_t c5_adfsr;
> > +        uint32_t c5_aifsr;
> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> >          uint32_t c6_insn; /* Fault address registers.  */
> >          uint32_t c6_data;
> > @@ -197,6 +199,7 @@ typedef struct CPUARMState {
> >          uint32_t c9_pmxevtyper; /* perf monitor event type */
> >          uint32_t c9_pmuserenr; /* perf monitor user enable */
> >          uint32_t c9_pminten; /* perf monitor interrupt enables */
> > +        uint32_t c9_l2ctlr; /* L2 Control Register */
> >          uint32_t c12_vbar; /* vector base address register */
> >          uint32_t c13_fcse; /* FCSE PID.  */
> >          uint32_t c13_context; /* Context ID.  */
> > @@ -867,6 +870,15 @@ struct ARMCPRegInfo {
> >      uint8_t opc0;
> >      uint8_t opc1;
> >      uint8_t opc2;
> > +    /* When migrating this flag tells if the register's value has
> already
> > +     * been copied. This is used in some unlikely cases where KVM
> migrates
> > +     * a register that in TCG is generated by a wildcarded or
> > +     * ARM_CP_STATE_BOTH parent (hence a not migratable register);
> > +     * in those cases, we will set the field "parent" to point to the
> > +     * generating register. Most of the time this field can stay false.
> > +     * */
> > +    bool skip_cpreglist;
>
> This reads to me like it's papering over a problem rather
> than fixing it.
>
> thanks
> -- PMM
>

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

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Peter Maydell]

On 26 February 2014 10:02, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> I agree that this is a sort of workaround, but it seems to me that a
> proper solution is not possible without changing the ideas contemplated
> now in the migration code.
> Are we willing to accept some major changes in the code to embrace
> this type of migration?

Well, the migration code as it stands is in theory supposed to cope
with KVM<->TCG migration. I would expect that we'd need to improve
a number of places where our TCG cpreg emulation is wrong.

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[alvise rigo]

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

Improvements of cpreg emulation apart, we will still need to tackle
with the great amount of registers not migrated because generated
by a wildcarded register; some of these registers are in fact migrated
by KVM. I think that the idea of keeping a reference
to the generator register (i.e. the only register which is migrated in a
wildcarded case) could be considered to handle the migration.

alvise


On Wed, Feb 26, 2014 at 11:04 AM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 26 February 2014 10:02, alvise rigo <a.rigo@virtualopensystems.com>
> wrote:
> > I agree that this is a sort of workaround, but it seems to me that a
> > proper solution is not possible without changing the ideas contemplated
> > now in the migration code.
> > Are we willing to accept some major changes in the code to embrace
> > this type of migration?
>
> Well, the migration code as it stands is in theory supposed to cope
> with KVM<->TCG migration. I would expect that we'd need to improve
> a number of places where our TCG cpreg emulation is wrong.
>
> thanks
> -- PMM
>

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

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Peter Maydell]

On 26 February 2014 10:27, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> Improvements of cpreg emulation apart, we will still need to tackle
> with the great amount of registers not migrated because generated
> by a wildcarded register; some of these registers are in fact migrated
> by KVM. I think that the idea of keeping a reference
> to the generator register (i.e. the only register which is migrated in a
> wildcarded case) could be considered to handle the migration.

These probably need to be fixed by being actually implemented in
TCG. If there's a register which we wildcard on TCG that means
we should usually fail the incoming migration, because the guest
might be actually using that register for something and we would
just be dropping the state in migration if we allowed the migration
to succeed.

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Peter Maydell]

On 26 February 2014 10:02, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> I agree that this is a sort of workaround, but it seems to me that a
> proper solution is not possible without changing the ideas contemplated
> now in the migration code.
> Are we willing to accept some major changes in the code to embrace
> this type of migration?

Incidentally it occurred to me recently that the design we've gone for
for AArch64 support (canonical copy of system register info is the AArch64
view, AArch32 register encoding is a non-migratable view onto the same
underlying data) clashes rather with the idea of being able to migrate
AArch32 TCG<->KVM. (Migrating AArch32 TCG to/from a KVM which
is configured to run the VM in AArch32 on an AArch64 host doesn't run
into the same problems, because in that case what KVM exposes to
userspace will also be the AArch64 views of registers.)
That was accidental, not an intentional tradeoff, but I'm not entirely
sure how to reconcile things...

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[alvise rigo]

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

Indeed, this is one of the problem I workaround with the patch.
One not much intrusive solution that I see now is to decouple the AArch64
definitions from AArch32. For example we can make add_cpreg_to_hashtable()
aware of the processor architecture with something like:

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 90f85f1..7fd8dc8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2232,6 +2232,13 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
const ARMCPRegInfo *r,
     /* Private utility function for define_one_arm_cp_reg_with_opaque():
      * add a single reginfo struct to the hash table.
      */
+    int is_a64 = (&cpu->env)->aarch64;
+    if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA64
+
&& !is_a64) {
+        /* no need of the AArch64 cp definition */
+        return;
+    }
+
     uint32_t *key = g_new(uint32_t, 1);
     ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
     int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
@@ -2241,9 +2248,14 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
const ARMCPRegInfo *r,
          * view handles that. AArch64 also handles reset.
          * We assume it is a cp15 register.
          */
+        if (is_a64) {
+            /* this is an AArch32 view */
+            r2->type |= ARM_CP_NO_MIGRATE;
+            r2->resetfn = arm_cp_reset_ignore;
+        } else {
+            /* this is the AArch32 register itself */
+        }
         r2->cp = 15;
-        r2->type |= ARM_CP_NO_MIGRATE;
-        r2->resetfn = arm_cp_reset_ignore;
 #ifdef HOST_WORDS_BIGENDIAN
         if (r2->fieldoffset) {
             r2->fieldoffset += sizeof(uint32_t);

So, if the processor does not start in AArch64 mode, we only add the AArch32
version of the ARM_CP_STATE_BOTH register to the hashtable, otherwise
nothing
changes and we add the two views of the register. This approach works only
if all the 64bit CPUs will always start in 64 bit mode.

In my opinion, other solutions would require to revisit the idea of
ARM_CP_STATE_BOTH in favour of distinct definitions of the registers with
multiple views (like VBAR for AArch32 and VBAR_EL1 for AArch64).

Regards,
alvise


On Mon, Mar 3, 2014 at 10:39 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 26 February 2014 10:02, alvise rigo <a.rigo@virtualopensystems.com>
> wrote:
> > I agree that this is a sort of workaround, but it seems to me that a
> > proper solution is not possible without changing the ideas contemplated
> > now in the migration code.
> > Are we willing to accept some major changes in the code to embrace
> > this type of migration?
>
> Incidentally it occurred to me recently that the design we've gone for
> for AArch64 support (canonical copy of system register info is the AArch64
> view, AArch32 register encoding is a non-migratable view onto the same
> underlying data) clashes rather with the idea of being able to migrate
> AArch32 TCG<->KVM. (Migrating AArch32 TCG to/from a KVM which
> is configured to run the VM in AArch32 on an AArch64 host doesn't run
> into the same problems, because in that case what KVM exposes to
> userspace will also be the AArch64 views of registers.)
> That was accidental, not an intentional tradeoff, but I'm not entirely
> sure how to reconcile things...
>
> thanks
> -- PMM
>

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

Re: [Qemu-devel] [RFC 4/4] Relevant changes to enable KVM to TCG migration

[Peter Maydell]

On 5 March 2014 15:01, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> So, if the processor does not start in AArch64 mode, we only add the AArch32
> version of the ARM_CP_STATE_BOTH register to the hashtable, otherwise
> nothing changes and we add the two views of the register.

Yes, I think if we want TCG<->KVM migration we're going to have to
do something like this. It introduces complication for reset handling,
though -- at the moment the aarch64 view handles reset and the
aarch32 view does not. We'd need to add (back) reset information
in the aarch32 views, and also raw read/write functions if needed.

> This approach works only
> if all the 64bit CPUs will always start in 64 bit mode.

You should be checking for whether the CPU has the AARCH64 feature,
which would fix that nit.

> In my opinion, other solutions would require to revisit the idea of
> ARM_CP_STATE_BOTH in favour of distinct definitions of the registers with
> multiple views (like VBAR for AArch32 and VBAR_EL1 for AArch64).

ARM_CP_STATE_BOTH is purely a shorthand to avoid having
to write out two definitions when they'd be virtually the same.
I don't think it adds any extra complexity that isn't already
present for registers that have split definitions. In fact the
split-definition registers are trickier because you end up needing
all of (1) aarch64 view (2) aarch32-in-aarch64-cpu view (3)
aarch32 view, which is pretty ugly.

thanks
-- PMM