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