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" <address@hidden>
> > 
> > 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 <address@hidden>
> > Reviewed-by: Christian Borntraeger <address@hidden>
> > ---
> >  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.