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

[Marcelo Tosatti]

On Mon, Jan 28, 2013 at 12:49:26PM -0500, Jason J. Herne wrote:
> 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?  

Because only registers on the runtime set should be written
(userspace->kernel direction) during runtime.

All registers must be read (kernel->userspace direction) on
cpu_synchronize_state(void).

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

This makes this 

1. cpu_synchronize_state()
2. read env->env which is part of RESET or FULL set but not RUNTIME set

sequence invalid (such as 'info regs'). Perhaps on S/390 its not a
problem.

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

This is fine.

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

Since cpu_synchronize_state() does not take a parameter, it is
expected to synchronize all registers (FULL/RESET/RUNTIME sets).

> 
> Also Alex, what did you mean by get_xxx() and set_xxx()?
> 
> -- 
> -- Jason J. Herne (address@hidden)