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

[Alexander Graf]

On 25.01.2013, at 01:01, Marcelo Tosatti wrote:

> On Thu, Jan 24, 2013 at 06:44:50PM -0200, Marcelo Tosatti wrote:
>> On Thu, Jan 24, 2013 at 01:40:49PM +0100, Alexander Graf wrote:
>>>> read_reg(x)
>>>>    if x not cached
>>>>            arch_get_regs(RUNTIME_STATE) (*)
>>>> 
>>>> write_reg(x, val)
>>>>    read_reg(x)
>>>>    cpustate->x = val;
>>>>    mark_dirty(x)
>>>> 
>>>> Which is basically the pattern used in KVM x86 (but instead of
>>>> ioctl(KVM_RUN) there is VMENTRY).
>>> 
>>> But that would mean that any code in QEMU that accesses registers can't 
>>> access env-> ,but instead needs to go through an accessor function. That's 
>>> a lot of potential for subtile error, no?
>> 
>> I do not see why. It has the potential to catch users of
>> env->reg which do not call cpu_synchronize_state().
>> 
>>> 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.
>> 
>> Well the interface "kvm_arch_get_regs" is supposed to synchronize the
>> entire state ATM. So for example, "info registers" has
>> 
>> - cpu_synchronize_state()
>> - proceed assuming env-> is an uptodate copy of VCPU registers.
>> 
>>> 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.
>> 
>> There is no parameter to cpu_synchronize_registers().
>> 
>>> 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?
>> 
>> Yes, creating a new subset GPR which is part of RUNTIME is valid. 
> 
> Accessors though are more organized, the information of which code
> accesses CPUState->env is then explicit. Whether to synchronize a given
> register using eg. GET_SREGS (say GPR set) or GET_ONE_REG (individual
> register) is up to architecture.
> 
> What 'subtle errors' are you thinking of?

I'm thinking of vmport for example, which lives in hw/ but still would need 
conversion to an accessor.

> It should be easy to convert as its greppable.

Only if the code in question makes it greppable. It could be using fancy 
env->spr[spr] = xxx; semantics instead.

But accessor functions are the second step to this. First we need 
infrastructure that lets us actually define which bits need to be synchronized. 
Then the next step could be to create accessor functions to remove use of 
cpu_synchronize_registers() or cpu_synchronize(REGS_GPRS).


Alex