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:01 PM, Marcelo Tosatti wrote:
> On Thu, Jan 24, 2013 at 06:44:50PM -0200, Marcelo Tosatti wrote:
>
> What 'subtle errors' are you thinking of?
>
> It should be easy to convert as its greppable.
>
> > S/390 not synchronizing the env-> copy of the FULL register set is still
> > a bug, though (because the FULL set is what "cpu_synchronize_state" with
> > no parameter implies).

There seems to be a lot of good discussion going on regarding these 
changes.  Have we reached some kind of consensus on how we want to proceed?

If I understand the arguments being made... this patch set, as 
submitted, is wrong because KVM_ARCH_GET_REGS should always retrieve 
runtime regs only?  Or must it sync everything?  If there is not a clear 
answer here we'll need to decide this before going anywhere with this 
patch, yes?

Alex 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?
"""

This is the most comprehensive suggestion I've been able to pick out of 
the discussion.  However, Marcelo has expressed concern over S390 not 
updating everything in env:

"""
S/390 not synchronizing the env-> copy of the FULL register set is still
a bug, though (because the FULL set is what "cpu_synchronize_state" with
no parameter implies).
"""

Also Alex, what did you mean by get_xxx() and set_xxx()?

--
-- Jason J. Herne (address@hidden)