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

[Marcelo Tosatti]

On Fri, Feb 01, 2013 at 10:47:37AM -0500, Jason J. Herne wrote:
> 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.

kvm_arch_get_registers reads the entire state, ie. FULL state. 

> 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!! :)

The idea behind s390_sync_partial_runtime_registers was that no generic
modifications have to be done. This (containment of the modifications 
to S/390) is possible if: 

1) The hot paths in question are vcpu local. That is, only executed in
VCPU context. Which seems to be the case because these hot paths are hot 
because they are VM-exits handled in userspace.

Can you confirm this?

Because frankly, i dislike splitting the register sets without adding accessors
(so thinking is, either go all the way and have an interface which is
difficult to make mistakes with or contain the register splitting 
changes to S/390).

But, the original author of this interface is Jan Kiskza, so he
should be consulted.