[Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only pass
a single argument.  Create SyncStateArgs struct for this purpose and add
register bitmap data member to it.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/sysemu/kvm.h |    6 ++++++
 kvm-all.c            |   27 +++++++++++++++++----------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index e0738ba..193d1f4 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
+
+struct kvm_cpu_syncstate_args {
+    CPUState *cpu;
+    int regmap;
+};
+
 void kvm_cpu_synchronize_state(CPUArchState *env);
 void kvm_cpu_synchronize_post_reset(CPUArchState *env);
 void kvm_cpu_synchronize_post_init(CPUArchState *env);
diff --git a/kvm-all.c b/kvm-all.c
index 1aa61bb..77ab72a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
 
     cpu->kvm_fd = ret;
     cpu->kvm_state = s;
-    cpu->kvm_vcpu_dirty = true;
+    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
 
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
@@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
 
 static void do_kvm_cpu_synchronize_state(void *arg)
 {
-    CPUState *cpu = arg;
+    struct kvm_cpu_syncstate_args *args = arg;
 
-    if (!cpu->kvm_vcpu_dirty) {
-        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
-        cpu->kvm_vcpu_dirty = true;
+    /* Do not sync regs that are already dirty */
+    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
+
+    if (regs_to_get) {
+        kvm_arch_get_registers(args->cpu, regs_to_get);
+        args->cpu->kvm_vcpu_dirty |= regs_to_get;
     }
 }
 
 void kvm_cpu_synchronize_state(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+    struct kvm_cpu_syncstate_args args;
+
+    args.cpu = cpu;
+    args.regmap = KVM_REGSYNC_FULL_STATE;
 
-    if (!cpu->kvm_vcpu_dirty) {
-        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
+    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
+        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
     }
 }
 
@@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
     CPUState *cpu = ENV_GET_CPU(env);
 
     kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
-    cpu->kvm_vcpu_dirty = false;
+    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
 }
 
 void kvm_cpu_synchronize_post_init(CPUArchState *env)
@@ -1521,7 +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
     CPUState *cpu = ENV_GET_CPU(env);
 
     kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
-    cpu->kvm_vcpu_dirty = false;
+    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
 }
 
 int kvm_cpu_exec(CPUArchState *env)
@@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
     do {
         if (cpu->kvm_vcpu_dirty) {
             kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
-            cpu->kvm_vcpu_dirty = false;
+            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
         }
 
         kvm_arch_pre_run(cpu, run);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Blue Swirl]

On Thu, Jan 10, 2013 at 3:29 PM, Jason J. Herne <jjherne@us.ibm.com> wrote:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
>
> do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only pass
> a single argument.  Create SyncStateArgs struct for this purpose and add
> register bitmap data member to it.
>
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  include/sysemu/kvm.h |    6 ++++++
>  kvm-all.c            |   27 +++++++++++++++++----------
>  2 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index e0738ba..193d1f4 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
>
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
> +
> +struct kvm_cpu_syncstate_args {

KVMCPUSyncStateArgs, don't forget the typedef.

> +    CPUState *cpu;
> +    int regmap;
> +};
> +
>  void kvm_cpu_synchronize_state(CPUArchState *env);
>  void kvm_cpu_synchronize_post_reset(CPUArchState *env);
>  void kvm_cpu_synchronize_post_init(CPUArchState *env);
> diff --git a/kvm-all.c b/kvm-all.c
> index 1aa61bb..77ab72a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
>
>      cpu->kvm_fd = ret;
>      cpu->kvm_state = s;
> -    cpu->kvm_vcpu_dirty = true;
> +    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
>
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
> @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
>
>  static void do_kvm_cpu_synchronize_state(void *arg)
>  {
> -    CPUState *cpu = arg;
> +    struct kvm_cpu_syncstate_args *args = arg;
>
> -    if (!cpu->kvm_vcpu_dirty) {
> -        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -        cpu->kvm_vcpu_dirty = true;
> +    /* Do not sync regs that are already dirty */
> +    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> +
> +    if (regs_to_get) {
> +        kvm_arch_get_registers(args->cpu, regs_to_get);
> +        args->cpu->kvm_vcpu_dirty |= regs_to_get;
>      }
>  }
>
>  void kvm_cpu_synchronize_state(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> +    struct kvm_cpu_syncstate_args args;
> +
> +    args.cpu = cpu;
> +    args.regmap = KVM_REGSYNC_FULL_STATE;
>
> -    if (!cpu->kvm_vcpu_dirty) {
> -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> +    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
>      }
>  }
>
> @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
>
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
>  }
>
>  void kvm_cpu_synchronize_post_init(CPUArchState *env)
> @@ -1521,7 +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
>
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
>  }
>
>  int kvm_cpu_exec(CPUArchState *env)
> @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
>      do {
>          if (cpu->kvm_vcpu_dirty) {
>              kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
> -            cpu->kvm_vcpu_dirty = false;
> +            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
>          }
>
>          kvm_arch_pre_run(cpu, run);
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Jason J. Herne [mailto:jjherne@us.ibm.com]
> Sent: Thursday, January 10, 2013 8:59 PM
> To: agraf@suse.de; borntraeger@de.ibm.com; aliguori@us.ibm.com;
> mtosatti@redhat.com; qemu-devel@nongnu.org; Bhushan Bharat-R65777;
> jan.kiszka@siemens.com
> Cc: Jason J. Herne
> Subject: [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to
> do_kvm_cpu_synchronize_state
> 
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only pass
> a single argument.  Create SyncStateArgs struct for this purpose and add
> register bitmap data member to it.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  include/sysemu/kvm.h |    6 ++++++
>  kvm-all.c            |   27 +++++++++++++++++----------
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index e0738ba..193d1f4 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int
> extension);
> 
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
> +
> +struct kvm_cpu_syncstate_args {
> +    CPUState *cpu;
> +    int regmap;
> +};
> +
>  void kvm_cpu_synchronize_state(CPUArchState *env);
>  void kvm_cpu_synchronize_post_reset(CPUArchState *env);
>  void kvm_cpu_synchronize_post_init(CPUArchState *env);
> diff --git a/kvm-all.c b/kvm-all.c
> index 1aa61bb..77ab72a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
> 
>      cpu->kvm_fd = ret;
>      cpu->kvm_state = s;
> -    cpu->kvm_vcpu_dirty = true;
> +    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
> 
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
> @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
> 
>  static void do_kvm_cpu_synchronize_state(void *arg)
>  {
> -    CPUState *cpu = arg;
> +    struct kvm_cpu_syncstate_args *args = arg;
> 
> -    if (!cpu->kvm_vcpu_dirty) {
> -        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -        cpu->kvm_vcpu_dirty = true;
> +    /* Do not sync regs that are already dirty */
> +    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> +
> +    if (regs_to_get) {
> +        kvm_arch_get_registers(args->cpu, regs_to_get);
> +        args->cpu->kvm_vcpu_dirty |= regs_to_get;
>      }
>  }
> 
>  void kvm_cpu_synchronize_state(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> +    struct kvm_cpu_syncstate_args args;
> +
> +    args.cpu = cpu;
> +    args.regmap = KVM_REGSYNC_FULL_STATE;
> 
> -    if (!cpu->kvm_vcpu_dirty) {
> -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> +    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
>      }
>  }
> 
> @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
> 
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
>  }
> 
>  void kvm_cpu_synchronize_post_init(CPUArchState *env)
> @@ -1521,7 +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
> 
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
>  }
> 
>  int kvm_cpu_exec(CPUArchState *env)
> @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
>      do {
>          if (cpu->kvm_vcpu_dirty) {
>              kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);

s/KVM_REGSYNC_RUNTIME_STATE/cpu->kvm_vcpu_dirty


> -            cpu->kvm_vcpu_dirty = false;
> +            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;

cpu->kvm_vcpu_dirty = 0;

-Bharat

>          }
> 
>          kvm_arch_pre_run(cpu, run);
> --
> 1.7.9.5
> 

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Thu, Jan 10, 2013 at 10:29:04AM -0500, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only pass
> a single argument.  Create SyncStateArgs struct for this purpose and add
> register bitmap data member to it.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  include/sysemu/kvm.h |    6 ++++++
>  kvm-all.c            |   27 +++++++++++++++++----------
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index e0738ba..193d1f4 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
>  
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
> +
> +struct kvm_cpu_syncstate_args {
> +    CPUState *cpu;
> +    int regmap;
> +};
> +
>  void kvm_cpu_synchronize_state(CPUArchState *env);
>  void kvm_cpu_synchronize_post_reset(CPUArchState *env);
>  void kvm_cpu_synchronize_post_init(CPUArchState *env);
> diff --git a/kvm-all.c b/kvm-all.c
> index 1aa61bb..77ab72a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
>  
>      cpu->kvm_fd = ret;
>      cpu->kvm_state = s;
> -    cpu->kvm_vcpu_dirty = true;
> +    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
>  
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
> @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
>  
>  static void do_kvm_cpu_synchronize_state(void *arg)
>  {
> -    CPUState *cpu = arg;
> +    struct kvm_cpu_syncstate_args *args = arg;
>  
> -    if (!cpu->kvm_vcpu_dirty) {
> -        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -        cpu->kvm_vcpu_dirty = true;
> +    /* Do not sync regs that are already dirty */
> +    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> +
> +    if (regs_to_get) {
> +        kvm_arch_get_registers(args->cpu, regs_to_get);
> +        args->cpu->kvm_vcpu_dirty |= regs_to_get;
>      }
>  }
>  
>  void kvm_cpu_synchronize_state(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> +    struct kvm_cpu_syncstate_args args;
> +
> +    args.cpu = cpu;
> +    args.regmap = KVM_REGSYNC_FULL_STATE;
>  
> -    if (!cpu->kvm_vcpu_dirty) {
> -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> +    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
>      }
>  }
>  
> @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
>  
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
>  }
>  
>  void kvm_cpu_synchronize_post_init(CPUArchState *env)
> @@ -1521,7 +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
>  
>      kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> -    cpu->kvm_vcpu_dirty = false;
> +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
>  }
>  
>  int kvm_cpu_exec(CPUArchState *env)
> @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
>      do {
>          if (cpu->kvm_vcpu_dirty) {
>              kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
> -            cpu->kvm_vcpu_dirty = false;
> +            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
>          }

1) This implies a vcpu can enter guest mode with kvm_vcpu_dirty non-zero.

Unrelated:

2) Also, what is the reason for specifying sets of registers in
arch-specific code? Is that because it allows PPC to fix their sync-timer
register problem? 

When you are writing generic code, what does it mean to
use 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
Answer: it depends on the architecture.

3) On x86, kvm_arch_get_registers(GET_FULL) must not imply 
kvm_arch_put_registers(PUT_FULL).

The S/390 problem, from
http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:

">>> The kvm register sync needs to happen in the kvm register sync
>>> function :)
>> That would eliminate the whole purpose of sync regs and forces us to
>> have an
>> expensive ioctl on lots of exits (again). I would prefer to sync the 
>> registers
>> that we never need in qemu just here.
> 
> That's why the register sync has different stages.

Not the get_register. Which is called on every synchronize_state. Which
happen 
quite often
on s390."

But wait: on these S/390 codepaths, you do GET_REGS already, via
cpu_synchronize_state.

So on S/390

- cpu_synchronize_state(env)
- read any register from env

Is not valid? This is what generic code assumes.


Bhushan Bharat, the PPC problem, can you describe it clearly: from what
i understood, an in-kernel register cannot be read/written back because
that register value can change in the meantime. When is it necessary 
to write it back? (there is a similar problem with TSC on x86, which 
is "fixed" by only writing TSC on FULL_STATE arch_put_registers).

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 02:05:33PM -0200, Marcelo Tosatti wrote:
> On Thu, Jan 10, 2013 at 10:29:04AM -0500, Jason J. Herne wrote:
> > From: "Jason J. Herne" <jjherne@us.ibm.com>
> > 
> > do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only pass
> > a single argument.  Create SyncStateArgs struct for this purpose and add
> > register bitmap data member to it.
> > 
> > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  include/sysemu/kvm.h |    6 ++++++
> >  kvm-all.c            |   27 +++++++++++++++++----------
> >  2 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index e0738ba..193d1f4 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
> >  
> >  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> >                                        uint32_t index, int reg);
> > +
> > +struct kvm_cpu_syncstate_args {
> > +    CPUState *cpu;
> > +    int regmap;
> > +};
> > +
> >  void kvm_cpu_synchronize_state(CPUArchState *env);
> >  void kvm_cpu_synchronize_post_reset(CPUArchState *env);
> >  void kvm_cpu_synchronize_post_init(CPUArchState *env);
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 1aa61bb..77ab72a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
> >  
> >      cpu->kvm_fd = ret;
> >      cpu->kvm_state = s;
> > -    cpu->kvm_vcpu_dirty = true;
> > +    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
> >  
> >      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> >      if (mmap_size < 0) {
> > @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
> >  
> >  static void do_kvm_cpu_synchronize_state(void *arg)
> >  {
> > -    CPUState *cpu = arg;
> > +    struct kvm_cpu_syncstate_args *args = arg;
> >  
> > -    if (!cpu->kvm_vcpu_dirty) {
> > -        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > -        cpu->kvm_vcpu_dirty = true;
> > +    /* Do not sync regs that are already dirty */
> > +    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> > +
> > +    if (regs_to_get) {
> > +        kvm_arch_get_registers(args->cpu, regs_to_get);
> > +        args->cpu->kvm_vcpu_dirty |= regs_to_get;
> >      }
> >  }
> >  
> >  void kvm_cpu_synchronize_state(CPUArchState *env)
> >  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> > +    struct kvm_cpu_syncstate_args args;
> > +
> > +    args.cpu = cpu;
> > +    args.regmap = KVM_REGSYNC_FULL_STATE;
> >  
> > -    if (!cpu->kvm_vcpu_dirty) {
> > -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> > +    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> > +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
> >      }
> >  }
> >  
> > @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
> >      CPUState *cpu = ENV_GET_CPU(env);
> >  
> >      kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> > -    cpu->kvm_vcpu_dirty = false;
> > +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
> >  }
> >  
> >  void kvm_cpu_synchronize_post_init(CPUArchState *env)
> > @@ -1521,7 +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
> >      CPUState *cpu = ENV_GET_CPU(env);
> >  
> >      kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > -    cpu->kvm_vcpu_dirty = false;
> > +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
> >  }
> >  
> >  int kvm_cpu_exec(CPUArchState *env)
> > @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
> >      do {
> >          if (cpu->kvm_vcpu_dirty) {
> >              kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
> > -            cpu->kvm_vcpu_dirty = false;
> > +            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
> >          }
> 
> 1) This implies a vcpu can enter guest mode with kvm_vcpu_dirty non-zero.
> 
> Unrelated:
> 
> 2) Also, what is the reason for specifying sets of registers in
> arch-specific code? Is that because it allows PPC to fix their sync-timer
> register problem? 
> 
> When you are writing generic code, what does it mean to
> use 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
> Answer: it depends on the architecture.
> 
> 3) On x86, kvm_arch_get_registers(GET_FULL) must not imply 
> kvm_arch_put_registers(PUT_FULL).
> 
> The S/390 problem, from
> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> 
> ">>> The kvm register sync needs to happen in the kvm register sync
> >>> function :)
> >> That would eliminate the whole purpose of sync regs and forces us to
> >> have an
> >> expensive ioctl on lots of exits (again). I would prefer to sync the 
> >> registers
> >> that we never need in qemu just here.
> > 
> > That's why the register sync has different stages.
> 
> Not the get_register. Which is called on every synchronize_state. Which
> happen 
> quite often
> on s390."
> 
> But wait: on these S/390 codepaths, you do GET_REGS already, via
> cpu_synchronize_state.
> 
> So on S/390
> 
> - cpu_synchronize_state(env)
> - read any register from env
> 
> Is not valid? This is what generic code assumes.

One possible alternative would be:

- Full synchronization on cpu_synchronize_state(env).
- Use of custom GET_REGS on these special codepaths which apparently
  are:
	- always vcpu local (no need for cpu_synchronize_state).
	- need a particular register read.
	- writeback nothing?

Is to read/write registers directly.

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Marcelo Tosatti [mailto:mtosatti@redhat.com]
> Sent: Wednesday, January 16, 2013 9:36 PM
> To: Jason J. Herne
> Cc: agraf@suse.de; borntraeger@de.ibm.com; aliguori@us.ibm.com; qemu-
> devel@nongnu.org; Bhushan Bharat-R65777; jan.kiszka@siemens.com
> Subject: Re: [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to
> do_kvm_cpu_synchronize_state
> 
> On Thu, Jan 10, 2013 at 10:29:04AM -0500, Jason J. Herne wrote:
> > From: "Jason J. Herne" <jjherne@us.ibm.com>
> >
> > do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only
> > pass a single argument.  Create SyncStateArgs struct for this purpose
> > and add register bitmap data member to it.
> >
> > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  include/sysemu/kvm.h |    6 ++++++
> >  kvm-all.c            |   27 +++++++++++++++++----------
> >  2 files changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
> > e0738ba..193d1f4 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int
> > extension);
> >
> >  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> >                                        uint32_t index, int reg);
> > +
> > +struct kvm_cpu_syncstate_args {
> > +    CPUState *cpu;
> > +    int regmap;
> > +};
> > +
> >  void kvm_cpu_synchronize_state(CPUArchState *env);  void
> > kvm_cpu_synchronize_post_reset(CPUArchState *env);  void
> > kvm_cpu_synchronize_post_init(CPUArchState *env); diff --git
> > a/kvm-all.c b/kvm-all.c index 1aa61bb..77ab72a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
> >
> >      cpu->kvm_fd = ret;
> >      cpu->kvm_state = s;
> > -    cpu->kvm_vcpu_dirty = true;
> > +    cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
> >
> >      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> >      if (mmap_size < 0) {
> > @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
> >
> >  static void do_kvm_cpu_synchronize_state(void *arg)  {
> > -    CPUState *cpu = arg;
> > +    struct kvm_cpu_syncstate_args *args = arg;
> >
> > -    if (!cpu->kvm_vcpu_dirty) {
> > -        kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > -        cpu->kvm_vcpu_dirty = true;
> > +    /* Do not sync regs that are already dirty */
> > +    int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> > +
> > +    if (regs_to_get) {
> > +        kvm_arch_get_registers(args->cpu, regs_to_get);
> > +        args->cpu->kvm_vcpu_dirty |= regs_to_get;
> >      }
> >  }
> >
> >  void kvm_cpu_synchronize_state(CPUArchState *env)  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> > +    struct kvm_cpu_syncstate_args args;
> > +
> > +    args.cpu = cpu;
> > +    args.regmap = KVM_REGSYNC_FULL_STATE;
> >
> > -    if (!cpu->kvm_vcpu_dirty) {
> > -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> > +    if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> > +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
> >      }
> >  }
> >
> > @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
> >      CPUState *cpu = ENV_GET_CPU(env);
> >
> >      kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> > -    cpu->kvm_vcpu_dirty = false;
> > +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
> >  }
> >
> >  void kvm_cpu_synchronize_post_init(CPUArchState *env) @@ -1521,7
> > +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
> >      CPUState *cpu = ENV_GET_CPU(env);
> >
> >      kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > -    cpu->kvm_vcpu_dirty = false;
> > +    cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
> >  }
> >
> >  int kvm_cpu_exec(CPUArchState *env)
> > @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
> >      do {
> >          if (cpu->kvm_vcpu_dirty) {
> >              kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
> > -            cpu->kvm_vcpu_dirty = false;
> > +            cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
> >          }
> 
> 1) This implies a vcpu can enter guest mode with kvm_vcpu_dirty non-zero.

I think above code should be:
            kvm_arch_put_registers(cpu, cpu->kvm_vcpu_dirty);
            cpu->kvm_vcpu_dirty = false;

so vcpu will not enter guest state with dirty registers in qemu.

> 
> Unrelated:
> 
> 2) Also, what is the reason for specifying sets of registers in arch-specific
> code? Is that because it allows PPC to fix their sync-timer register problem?
> 
> When you are writing generic code, what does it mean to use
> 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
> Answer: it depends on the architecture.
> 
> 3) On x86, kvm_arch_get_registers(GET_FULL) must not imply
> kvm_arch_put_registers(PUT_FULL).
> 
> The S/390 problem, from
> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> 
> ">>> The kvm register sync needs to happen in the kvm register sync
> >>> function :)
> >> That would eliminate the whole purpose of sync regs and forces us to
> >> have an expensive ioctl on lots of exits (again). I would prefer to
> >> sync the registers that we never need in qemu just here.
> >
> > That's why the register sync has different stages.
> 
> Not the get_register. Which is called on every synchronize_state. Which happen
> quite often on s390."
> 
> But wait: on these S/390 codepaths, you do GET_REGS already, via
> cpu_synchronize_state.
> 
> So on S/390
> 
> - cpu_synchronize_state(env)
> - read any register from env
> 
> Is not valid? This is what generic code assumes.
> 
> 
> Bhushan Bharat, the PPC problem, can you describe it clearly: from what i
> understood, an in-kernel register cannot be read/written back because that
> register value can change in the meantime. When is it necessary to write it
> back? (there is a similar problem with TSC on x86, which is "fixed" by only
> writing TSC on FULL_STATE arch_put_registers).

There are two things:

First-)
For timer related changes on PowerPC, some registers needed to be changed from QEMU, so we have to get the registers via KVM_GET_SREGS and then set those registers back to KVM via KVM_SET_SREGS. cpu_synchronize_state() will get registers but kvm_arch_put_registers() works on level based mechanism and does not provide a good way of setting a register-set. So we wrote a separate function that will push these registers back to KVM and this also uses KVM_SET_SREGS ioctl. This solves what is needed for PPC.

Second-)
Currently kvm_arch_get_registers() is not optimized in two sense; one, it always get all registers from KVM; two, in kvm_arch_get_registers() it copies all registers to env->. This patch-set handles the second issue of optimization, copy only the requested registers to env-> in kvm_arch_get_registers(), plus when kvm_arch_put_registers() is called then it copies only the modified registers for KVM_SET_SREGS.

This optimization is looking good to me and allows sync of registers via one common kvm_arch_get/set_registers() and no separate function definition for setting is needed for timer related changes.

Thanks
-Bharat
 

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 05:00:52PM +0000, Bhushan Bharat-R65777 wrote:
> I think above code should be:
>             kvm_arch_put_registers(cpu, cpu->kvm_vcpu_dirty);
>             cpu->kvm_vcpu_dirty = false;
> 
> so vcpu will not enter guest state with dirty registers in qemu.

Not so clear - currently PUT_FULL/PUT_RESET are performed on
pre-defined points.

> > Unrelated:
> > 
> > 2) Also, what is the reason for specifying sets of registers in arch-specific
> > code? Is that because it allows PPC to fix their sync-timer register problem?
> > 
> > When you are writing generic code, what does it mean to use
> > 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
> > Answer: it depends on the architecture.
> > 
> > 3) On x86, kvm_arch_get_registers(GET_FULL) must not imply
> > kvm_arch_put_registers(PUT_FULL).
> > 
> > The S/390 problem, from
> > http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> > 
> > ">>> The kvm register sync needs to happen in the kvm register sync
> > >>> function :)
> > >> That would eliminate the whole purpose of sync regs and forces us to
> > >> have an expensive ioctl on lots of exits (again). I would prefer to
> > >> sync the registers that we never need in qemu just here.
> > >
> > > That's why the register sync has different stages.
> > 
> > Not the get_register. Which is called on every synchronize_state. Which happen
> > quite often on s390."
> > 
> > But wait: on these S/390 codepaths, you do GET_REGS already, via
> > cpu_synchronize_state.
> > 
> > So on S/390
> > 
> > - cpu_synchronize_state(env)
> > - read any register from env
> > 
> > Is not valid? This is what generic code assumes.
> > 
> > 
> > Bhushan Bharat, the PPC problem, can you describe it clearly: from what i
> > understood, an in-kernel register cannot be read/written back because that
> > register value can change in the meantime. When is it necessary to write it
> > back? (there is a similar problem with TSC on x86, which is "fixed" by only
> > writing TSC on FULL_STATE arch_put_registers).
> 
> There are two things:
> 
> First-)
> For timer related changes on PowerPC, some registers needed to be changed from QEMU, so we have to get the registers via KVM_GET_SREGS and then set those registers back to KVM via KVM_SET_SREGS. cpu_synchronize_state() will get registers but kvm_arch_put_registers() works on level based mechanism and does not provide a good way of setting a register-set. So we wrote a separate function that will push these registers back to KVM and this also uses KVM_SET_SREGS ioctl. This solves what is needed for PPC.

Can you describe the problem in detail? You must sync a particular
timer register only on special conditions, not during normal
cpu_synchronize_state() runs?

What register is that and why it cannot be synced normally? When is it
necessary to sync it?

> Second-)
> Currently kvm_arch_get_registers() is not optimized in two sense; one, it always get all registers from KVM; two, in kvm_arch_get_registers() it copies all registers to env->. This patch-set handles the second issue of optimization, copy only the requested registers to env-> in kvm_arch_get_registers(), plus when kvm_arch_put_registers() is called then it copies only the modified registers for KVM_SET_SREGS.
> 
> This optimization is looking good to me and allows sync of registers via one common kvm_arch_get/set_registers() and no separate function definition for setting is needed for timer related changes.

The problem with S390 is not clear to me (besides
cpu_synchronize_state must sync all state as mentioned).

> 
> Thanks
> -Bharat
>  

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 05:00:52PM +0000, Bhushan Bharat-R65777 wrote:
> Second-)
> Currently kvm_arch_get_registers() is not optimized in two sense; one, it always get all registers from KVM; two, in kvm_arch_get_registers() it copies all registers to env->. This patch-set handles the second issue of optimization, copy only the requested registers to env-> in kvm_arch_get_registers(), plus when kvm_arch_put_registers() is called then it copies only the modified registers for KVM_SET_SREGS.
> 
> This optimization is looking good to me and allows sync of registers via one common kvm_arch_get/set_registers() and no separate function definition for setting is needed for timer related changes.

To be clearer. For example, executing:

- kvm_get_regs(FULL_STATE)
- kvm_put_regs(FULL_STATE)

Is reading and writing the full register set while
in runtime state.

/* full state set, modified during initialization or on vmload */
#define KVM_PUT_FULL_STATE      3

Again, its necessary to read the full state on "cpu_synchronize_state" 
because its not specified which registers can be modified
during ioctl(KVM_RUN) (therefore separating get registers operation
in full/reset/runtime sets is awkward).

But direct read/write of registers is also bad: if a given register is
read and modified (but not written back), then reading through ioctl()
returns a wrong value.

Apparently accessors are the best option. 

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Christian Borntraeger]

On 16/01/13 17:05, Marcelo Tosatti wrote:

> The S/390 problem, from
> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> 
> ">>> The kvm register sync needs to happen in the kvm register sync
>>>> function :)
>>> That would eliminate the whole purpose of sync regs and forces us to
>>> have an
>>> expensive ioctl on lots of exits (again). I would prefer to sync the 
>>> registers
>>> that we never need in qemu just here.
>>
>> That's why the register sync has different stages.
> 
> Not the get_register. Which is called on every synchronize_state. Which
> happen 
> quite often
> on s390."
> 
> But wait: on these S/390 codepaths, you do GET_REGS already, via
> cpu_synchronize_state.
> 
> So on S/390
> 
> - cpu_synchronize_state(env)
> - read any register from env
> 
> Is not valid? This is what generic code assumes.

TO recap the motiviation:

cpu_synchronize_state on s390 currently updates any register in env that is
used by qemu (general purpose, prefix, psw, control and access) in the normal
runtime. it turns out we have all of these regs in kvm_run, so we can do 
synchronize states without doing an additional ioctl call.
Now, for life migration and dump we need some additional registers (which are
only accessable via onereg interface). So synchronize_state would need to
do 3 or 4 additional system calls on the hot path, only to take care of 
something that is not on the hot path at all.
For historic reasons, we have one exit code for almost all exits. Therefore,
we need to call synchronize_states almost always.
We could now start to have a poor mans synchronize_state in arch code, but
that would collide with common code synchronize_state if done at the wrong
time. Thus we want to make common code capable of having only a subset of
the register synched - by making it possible to sync the other regs later
on if needed without wiping the former sync.

Makes sense?

Christian

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 09:03:20PM +0100, Christian Borntraeger wrote:
> On 16/01/13 17:05, Marcelo Tosatti wrote:
> 
> > The S/390 problem, from
> > http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> > 
> > ">>> The kvm register sync needs to happen in the kvm register sync
> >>>> function :)
> >>> That would eliminate the whole purpose of sync regs and forces us to
> >>> have an
> >>> expensive ioctl on lots of exits (again). I would prefer to sync the 
> >>> registers
> >>> that we never need in qemu just here.
> >>
> >> That's why the register sync has different stages.
> > 
> > Not the get_register. Which is called on every synchronize_state. Which
> > happen 
> > quite often
> > on s390."
> > 
> > But wait: on these S/390 codepaths, you do GET_REGS already, via
> > cpu_synchronize_state.
> > 
> > So on S/390
> > 
> > - cpu_synchronize_state(env)
> > - read any register from env
> > 
> > Is not valid? This is what generic code assumes.
> 
> TO recap the motiviation:
> 
> cpu_synchronize_state on s390 currently updates any register in env that is
> used by qemu (general purpose, prefix, psw, control and access) in the normal
> runtime. it turns out we have all of these regs in kvm_run, so we can do 
> synchronize states without doing an additional ioctl call.
> Now, for life migration and dump we need some additional registers (which are
> only accessable via onereg interface). So synchronize_state would need to
> do 3 or 4 additional system calls on the hot path, only to take care of 
> something that is not on the hot path at all.
> For historic reasons, we have one exit code for almost all exits. Therefore,
> we need to call synchronize_states almost always.
> We could now start to have a poor mans synchronize_state in arch code, but
> that would collide with common code synchronize_state if done at the wrong
> time. Thus we want to make common code capable of having only a subset of
> the register synched - by making it possible to sync the other regs later
> on if needed without wiping the former sync.
> 
> Makes sense?
> 
> Christian

Yes. As noted in the last email on the thread, runtime/reset/full are to
serapate sets of registers when writing _to_ kernel. When reading _from_
kernel, reset and full distinctions are not appropriate (any register
can change, as far as knowledge goes).

Accessors for reading/writing shared (between userspace and kernel)
registers in CPUState is one option.

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Christian Borntraeger]

On 16/01/13 21:21, Marcelo Tosatti wrote:
> On Wed, Jan 16, 2013 at 09:03:20PM +0100, Christian Borntraeger wrote:
>> On 16/01/13 17:05, Marcelo Tosatti wrote:
>>
>>> The S/390 problem, from
>>> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
>>>
>>> ">>> The kvm register sync needs to happen in the kvm register sync
>>>>>> function :)
>>>>> That would eliminate the whole purpose of sync regs and forces us to
>>>>> have an
>>>>> expensive ioctl on lots of exits (again). I would prefer to sync the 
>>>>> registers
>>>>> that we never need in qemu just here.
>>>>
>>>> That's why the register sync has different stages.
>>>
>>> Not the get_register. Which is called on every synchronize_state. Which
>>> happen 
>>> quite often
>>> on s390."
>>>
>>> But wait: on these S/390 codepaths, you do GET_REGS already, via
>>> cpu_synchronize_state.
>>>
>>> So on S/390
>>>
>>> - cpu_synchronize_state(env)
>>> - read any register from env
>>>
>>> Is not valid? This is what generic code assumes.
>>
>> TO recap the motiviation:
>>
>> cpu_synchronize_state on s390 currently updates any register in env that is
>> used by qemu (general purpose, prefix, psw, control and access) in the normal
>> runtime. it turns out we have all of these regs in kvm_run, so we can do 
>> synchronize states without doing an additional ioctl call.
>> Now, for life migration and dump we need some additional registers (which are
>> only accessable via onereg interface). So synchronize_state would need to
>> do 3 or 4 additional system calls on the hot path, only to take care of 
>> something that is not on the hot path at all.
>> For historic reasons, we have one exit code for almost all exits. Therefore,
>> we need to call synchronize_states almost always.
>> We could now start to have a poor mans synchronize_state in arch code, but
>> that would collide with common code synchronize_state if done at the wrong
>> time. Thus we want to make common code capable of having only a subset of
>> the register synched - by making it possible to sync the other regs later
>> on if needed without wiping the former sync.
>>
>> Makes sense?
>>
>> Christian
> 
> Yes. As noted in the last email on the thread, runtime/reset/full are to
> serapate sets of registers when writing _to_ kernel. When reading _from_
> kernel, reset and full distinctions are not appropriate (any register
> can change, as far as knowledge goes).

Hmm, I probably did not understood your point, so I will try to explain mine
and see what you respond :-)

The point of the patch set, is to allow this distinction when reading. 
In other words it allows code to state: I am only interested in regxy and dont
care if the other regs in env are out of sync.
If a full sync is necessary later on the other regs are synched as well.
If a full sync was already done before a partial get becomes a no-op. 
Why should that be not possible.

Christian

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 09:41:54PM +0100, Christian Borntraeger wrote:
> On 16/01/13 21:21, Marcelo Tosatti wrote:
> > On Wed, Jan 16, 2013 at 09:03:20PM +0100, Christian Borntraeger wrote:
> >> On 16/01/13 17:05, Marcelo Tosatti wrote:
> >>
> >>> The S/390 problem, from
> >>> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> >>>
> >>> ">>> The kvm register sync needs to happen in the kvm register sync
> >>>>>> function :)
> >>>>> That would eliminate the whole purpose of sync regs and forces us to
> >>>>> have an
> >>>>> expensive ioctl on lots of exits (again). I would prefer to sync the 
> >>>>> registers
> >>>>> that we never need in qemu just here.
> >>>>
> >>>> That's why the register sync has different stages.
> >>>
> >>> Not the get_register. Which is called on every synchronize_state. Which
> >>> happen 
> >>> quite often
> >>> on s390."
> >>>
> >>> But wait: on these S/390 codepaths, you do GET_REGS already, via
> >>> cpu_synchronize_state.
> >>>
> >>> So on S/390
> >>>
> >>> - cpu_synchronize_state(env)
> >>> - read any register from env
> >>>
> >>> Is not valid? This is what generic code assumes.
> >>
> >> TO recap the motiviation:
> >>
> >> cpu_synchronize_state on s390 currently updates any register in env that is
> >> used by qemu (general purpose, prefix, psw, control and access) in the normal
> >> runtime. it turns out we have all of these regs in kvm_run, so we can do 
> >> synchronize states without doing an additional ioctl call.
> >> Now, for life migration and dump we need some additional registers (which are
> >> only accessable via onereg interface). So synchronize_state would need to
> >> do 3 or 4 additional system calls on the hot path, only to take care of 
> >> something that is not on the hot path at all.
> >> For historic reasons, we have one exit code for almost all exits. Therefore,
> >> we need to call synchronize_states almost always.
> >> We could now start to have a poor mans synchronize_state in arch code, but
> >> that would collide with common code synchronize_state if done at the wrong
> >> time. Thus we want to make common code capable of having only a subset of
> >> the register synched - by making it possible to sync the other regs later
> >> on if needed without wiping the former sync.
> >>
> >> Makes sense?
> >>
> >> Christian
> > 
> > Yes. As noted in the last email on the thread, runtime/reset/full are to
> > serapate sets of registers when writing _to_ kernel. When reading _from_
> > kernel, reset and full distinctions are not appropriate (any register
> > can change, as far as knowledge goes).
> 
> Hmm, I probably did not understood your point, so I will try to explain mine
> and see what you respond :-)
> 
> The point of the patch set, is to allow this distinction when reading. 
> In other words it allows code to state: I am only interested in regxy and dont
> care if the other regs in env are out of sync.

Fine.

> If a full sync is necessary later on the other regs are synched as well.
> If a full sync was already done before a partial get becomes a no-op.

-> FULL is the set of registers written when loadvm/initialization is
performed.
-> RESET, a subset of full, is a set of registers written on SYSTEM
RESET.
-> RUNTIME, a subset of RESET, is a set of registers written during
RUNTIME.

To write both the RESET and FULL set of registers during runtime,
contradicts the description above for both RESET and FULL.

Two examples from i386:

    if (level == KVM_PUT_FULL_STATE) {
        /*
         * KVM is yet unable to synchronize TSC values of multiple VCPUs
         * on
         * writeback. Until this is fixed, we only write the offset to
         * SMP
         * guests after migration, desynchronizing the VCPUs, but
         * avoiding
         * huge jump-backs that would occur without any writeback at
         * all.
         */
	...
    }

And:

    /*
     * The following paravirtual MSRs have side effects on the guest or
     * are
     * too heavy for normal writeback. Limit them to reset or full state
     * updates.
     */

> Why should that be not possible.

It should, but separately from FULL/RESET/RUNTIME distinction.
This sequence

	get_regs(FULLSTATE)
	put_regs(FULLSTATE)

During runtime is not allowed. And only syncing the RUNTIME set of
registers during and leaving the FULL set of registers marked as
dirty is confusing also. 

So perhaps what you'd want is selective read/write of RUNTIME registers
as suggested.


Date: Fri, 4 Jan 2013 23:49:42 -0200
From: Marcelo Tosatti <mtosatti@redhat.com>
To: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
Cc: Alexander Graf <agraf@suse.de>,
	Bhushan Bharat-R65777 <R65777@freescale.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Anthony Liguori <aliguori@us.ibm.com>,
	"qemu-devel@nongnu.org qemu-devel" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix
 do_kvm_cpu_synchronize_state data integrity issue

On Fri, Jan 04, 2013 at 10:25:45AM -0500, Jason J. Herne wrote:
> If I've followed the conversation correctly this is what needs to be done:
> 
> 1. Remove the level parameters from kvm_arch_get_registers and
> kvm_arch_put_registers.
> 
> 2. Add a new bitmap parameter to kvm_arch_get_registers and
> kvm_arch_put_registers.
> 
> 3. Define a bit that correlates to our current notion of "all
> runtime registers".  This bit, and all bits in this bitmap, would be
> architecture specific.
> 
> 4. Remove the cpustate->kvm_sync_dirty field.  Replace it with a
> bitmap that tracks which bits are dirty and need to be synced back
> to KVM-land.
> 
> 5. As we do today, we'll assume registers are dirty and turn on
> their corresponding bit in this new bitmap whenever we "get" the
> registers from KVM.
> 
> 6. Add other bits as needed on a case by case basis.
> 
> Does this seem to match what was discussed, and what we want to do?
> 
> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)

The s390 change to use runtime_state is self-contained and could be
integrated now from my pov.

The suggestion was based on the observation that the ppc guys are trying 
to fix their timer problem and that should be a generic solution,
per-register granularity.

Perhaps something along the lines of 

init
all registers marked as cached (read/write accessors don't 
synchronize state)

Around ioctl(KVM_RUN) have:

if regs dirty
	writeback
r = ioctl(KVM_RUN)
mark runstate registers as not cached

Then

read_reg(x)
	if x not cached
		arch_get_regs(RUNTIME_STATE) (*)

write_reg(x, val)
	read_reg(x)
	cpustate->x = val;
	mark_dirty(x)

Which is basically the pattern used in KVM x86 (but instead of
ioctl(KVM_RUN) there is VMENTRY).

But talk is cheap, how that would work with RESET/FULL states
is unclear. Jan?

For those archs that need, of course, (*) would use ONE_REG or whatever
other interface. For x86, or those archs that are not converted, keep
using kvm_arch_get_regs/kvm_arch_put_regs.

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Jason J. Herne]

On 01/24/2013 07:40 AM, Alexander Graf wrote:
> I think for now the best choice for get_regs() would be to ignore the FULL/RESET bits and always keep the syncing as it happens today under the RUNTIME umbrella only. So all of get_regs() only checks for RUNTIME.
>
> Whenever get_xxx() happens, a bit gets set for set_xxx(). Up to this point, only the RUNTIME bit is ever set, because that's what cpu_synchronize_registers() sets.
>
> Then s390 can add special separate bits for "sync GPRs" and "sync CRs". SYNC_RUNTIME would include those bits. The kvm hypercall exit calls a new synchronize_registers() function with a parameter telling it to only sync GPRs. This marks GPRs dirty, but not RUNTIME. The set_registers() function in s390 specific code could handle this particular case specially.
>
> That way everything's solved and scalable, no?
>
> Alex
>

Ok, based on the discussions we've had I think I have a plan of attack 
based on Alex's above suggestion.  I believe it also satisfies the 
concerns Marcelo pointed out.  Please correct me if I'm wrong.

kvm_arch_get_registers() stays exactly as is for all architectures 
(reads RUNTIME state only). No new parameters.

Each architecture defines arch specific bits for runtime/reset/full states:
     #define KVM_REGSYNC_I386_RUNTIME_BIT  (1 << 1)
     #define KVM_REGSYNC_I386_RESET_BIT    (1 << 2)
     #define KVM_REGSYNC_I386_FULL_BIT     (1 << 3)

Each architecture defines generic bits (for use in platform agnostic 
code: kvm-all.c) for runtime/reset/full states:
     #define KVM_REGSYNC_RUNTIME_STATE    KVM_REGSYNC_I386_RUNTIME_BIT
     #define KVM_REGSYNC_RESET_STATE      KVM_REGSYNC_I386_RESET_BIT
     #define KVM_REGSYNC_FULL_STATE       KVM_REGSYNC_I386_FULL_BIT

S390: replace KVM_REGSYNC_S390_RUNTIME_BIT with two new bits so the S390 
arch specific bits look like this:
     #define KVM_REGSYNC_S390_RUNTIME_SOME_BIT  (1 << 1)
     #define KVM_REGSYNC_S390_RUNTIME_REST_BIT  (1 << 2)
     #define KVM_REGSYNC_S390_RESET_BIT         (1 << 3)
     #define KVM_REGSYNC_S390_FULL_BIT          (1 << 4)
The idea being that SOME represents the set of RUNTIME registers we 
always want to read when we exit from KVM. And REST represents the set 
of RUNTIME registers we want to read for migration/dump and potentially 
other special cases.  My understanding is that SOME and REST should be 
mutually exclusive.  I think they need better names as well :).

S390 defines it's generic bits like this:
     #define KVM_REGSYNC_RUNTIME_STATE
         (KVM_REGSYNC_S390_RUNTIME_SOME_BIT |
             KVM_REGSYNC_S390_RUNTIME_REST_BIT)
     #define KVM_REGSYNC_RESET_STATE      KVM_REGSYNC_S390_RESET_BIT
     #define KVM_REGSYNC_FULL_STATE        KVM_REGSYNC_S390_FULL_BIT

S390: A new function is created: s390_sync_partial_runtime_registers(int 
bitmap).  The bitmap argument indicates which of the SOME/REST register 
sets to read.  Either this new function or perhaps the caller will 
update the cpu->kvm_vcpu_dirty bitmap to indicate which regs are now dirty.

S390: On the hot paths we call 
s390_sync_partial_runtime_registers(KVM_REGSYNC_S390_RUNTIME_SOME_BIT) 
instead of cpu_synchronize_state() to read only the set of runtime 
registers we need on the hot path.  If at some later point 
cpu_synchronize_state() happens to be called then the S390 version of 
kvm_arch_get_registers() needs to be smart enough to avoid data loss. 
So we make it write back all dirty registers (env->kvm_vcpu_dirty) 
before getting anything.

I think this works.  Comments please and thank you!! :)

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)