Re: [PATCH 09/10] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs

[Gleb Natapov <gleb@redhat.com>]

On Thu, Apr 18, 2013 at 09:24:20AM +0300, Abel Gordon wrote:
> 
> 
> Paolo Bonzini <paolo.bonzini@gmail.com> wrote on 18/04/2013 12:59:48 AM:
> 
> 
> > Il 17/04/2013 18:03, Abel Gordon ha scritto:
> > >> > Right, not a big deal if this is the only case when it happens. When
> we
> > >> > discussed accessors vs sync_shadow_vmcs flag approach I said that
> flag
> > >> > will work only if no vmcs12 fields are changed not as part of vmexit
> or
> > >> > vmwrite emulations. This one is such a field unfortunately. Hope it
> is
> > >> > the only one.
> > > Yep, remember that. I answered that L0 should NOT change VMCS12 fields
> > > if L1 is running and L1 didn't execute any vmlaunch, vmresume,
> vmwrite...
> > > (any vmx instruction. Sorry if I wasn't clear).
> > > nested_vmx_failValid is called ONLY when L1 executes vmx instructions
> > > which L0 traps and emulate.
> > >
> > > So, can we keep this part of the code as is ?
> >
> > I think so.  Not shadowing the field is just as good a solution as
> > forcing the copy.
> 
> Ok, then I'll keep the code as is (not shadowing).
> 
Paolo is right that forcing the copy is not less error pron since each
case where vmcs is changed outside of vmwrite emulation or vmexit should
be tracked manually. Accessors is the only way to make the code more or
less error free without reviewing each case manually. But I am OK with
current approach for now. To get rid of unnecessary copying we will have
to move to accessors some day anyway.

> > Perhaps at the top of the field lists you can replace the comment about
> > VM_INSTRUCTION_ERROR with one that is more generic, and mentions that
> > fields that are changed as part of vmexit or vmwrite emulation must not
> > be shadowed, or alternatively *insert explanation here*...
> 
> Good idea, I will change the comment to be more generic.
I like the comment since it provides example that can be looked at to
understand the problem, so if you make the comment more generic leave
the example please :)

> 
> Note I already sent v3 which targets all the other suggestions Gleb
> wrote. Let me know if I should wait for another review or just
> re-send v4 so you can apply the patches.
> Right now, the only pending change for v4 is generalizing the
> VM_INSTRUCTION_ERROR comment.

--
			Gleb.