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 (address@hidden)