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

[Gleb Natapov <gleb@redhat.com>]

On Sun, Apr 14, 2013 at 01:49:44PM +0300, Abel Gordon wrote:
> 
> 
> Gleb Natapov <gleb@redhat.com> wrote on 14/04/2013 01:34:52 PM:
> 
> > On Sun, Apr 14, 2013 at 12:27:10PM +0200, Jan Kiszka wrote:
> > > On 2013-04-14 12:07, Gleb Natapov wrote:
> > > > On Sun, Apr 14, 2013 at 01:00:10PM +0300, Gleb Natapov wrote:
> > > >> On Sun, Apr 14, 2013 at 12:51:34PM +0300, Abel Gordon wrote:
> > > >>>
> > > >>>
> > > >>> Gleb Natapov <gleb@redhat.com> wrote on 12/04/2013 01:48:04 PM:
> > > >>>
> > > >>>> On Fri, Apr 12, 2013 at 01:44:14PM +0300, Abel Gordon wrote:
> > > >>>>>
> > > >>>>> Ok, so then you prefer to add the inline functions to read/
> > write to the
> > > >>>>> vmcs12
> > > >>>>> fields, (to set the request bit if shadowed field changed) and
> you are
> > > >>> not
> > > >>>>> concerned
> > > >>>>> about any merge/rebase mess. I will work on this direction.
> > > >>>>> I'll first send an independent patch to introduce the accessors.
> Once
> > > >>> you
> > > >>>>> apply this patch, I'll continue and send you v2 patches for
> shadow
> > > >>> vmcs.
> > > >>>>>
> > > >>>>> Do you agree ?
> > > >>>> Yes.
> > > >>>
> > > >>> Looking again at the code it seems like we could avoid adding the
> > > >>> accessors.
> > > >>> We could just set a flag in nested_vmx_vmexit and
> > > >>> nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset
> > the flag and
> > > >>> call copy_vmcs12_to_shadow (if required).
> > > >>>
> > > >>> What do you think ?
> > > >> Good idea! With accessors we can do further optimization by copying
> only
> > > >> things that changed, but it will be premature optimization at this
> > > >> point.
> > > >>
> > > > Actually this is good idea only if we know for sure that VMX
> emulation
> > > > changes vmcs12 only during guest entry/exit. Is this the case? I
> think
> > > > so.
> > >
> > > Some vmcs12 fields that are exposed to L1 are changed outside L2<->L1
> > > transitions. What comes to my mind: L0 emulates some change that L1
> does
> > > not trap, e.g. CRx accesses. Or what do you mean?
> > >
> > If vmcs12 is changed by L0 while L2 is running this is OK. If L0 changes
> > shadowed vmcs12 field while L1 is running this is not OK. So for
> > instance if field XXX is R/W but we allow only read to be shadowed then
> > write emulation in L0 has to sync new value back to shadow before going
> > back to L1.
> 
> Exactly.
> 
> While L1 runs (L1 root mode), L0 does NOT change VMCS12 (unless L1
> executes vmwrite).
> VMCS12 fields are changed once L1 launches/resumes L2 and there is a
> L2 exit.
> 
> L0 can change VMCS12 while it handles a L2 exit directly which is not
> forwarded to L1. But that's OK because L1 will eventually see the change
> once we switch to L1 due to other exit that L0 let L1 handle.
> 
> L0 should NOT change VMCS12 fields if L1 is running and L1 didn't
> execute any vmlaunch, vmresume or vmwrite instruction.
> 
The question is: is there a case like I described above when we shadow
only reads from R/W field and handle vmwrites in L0?

--
			Gleb.