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

[Jan Kiszka]

On 2013-02-11 23:49, Marcelo Tosatti wrote:
> 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)


These states remain levels, i.e. are cumulative: RESET implies RUNTIME,
FULL implies RESET+RUNTIME. The encoding shall express this.

>>
>> 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 only do s390-specific stuff with the registers. At the point
generic code starts to access the state, you must read the FULL state as
that is what generic bits can assume today.

>>
>> 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.

Yes, until we have a generic and clean solution for all archs, such a
read split-up should remain s390-local.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux