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 (jjhe...@linux.vnet.ibm.com)
