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

[Gleb Natapov <gleb@redhat.com>]

On Fri, Apr 12, 2013 at 01:44:14PM +0300, Abel Gordon wrote:
> 
> 
> kvm-owner@vger.kernel.org wrote on 12/04/2013 01:31:17 PM:
> 
> > From: Gleb Natapov <gleb@redhat.com>
> > To: Abel Gordon/Haifa/IBM@IBMIL,
> > Cc: dongxiao.xu@intel.com, jun.nakajima@intel.com,
> > kvm@vger.kernel.org, "Nadav Har'El" <nyh@math.technion.ac.il>,
> > owasserm@redhat.com
> > Date: 12/04/2013 01:31 PM
> > Subject: Re: [PATCH 10/11] KVM: nVMX: Synchronize VMCS12 content
> > with the shadow vmcs
> > Sent by: kvm-owner@vger.kernel.org
> >
> > On Fri, Apr 12, 2013 at 01:26:32PM +0300, Abel Gordon wrote:
> > >
> > >
> > > Gleb Natapov <gleb@redhat.com> wrote on 11/04/2013 09:54:11 AM:
> > >
> > > > On Wed, Apr 10, 2013 at 10:15:37PM +0300, Abel Gordon wrote:
> > > > >
> > > > >
> > > > > Gleb Natapov <gleb@redhat.com> wrote on 09/04/2013 04:14:35 PM:
> > > > > > I think the patch already miss some fields. What if
> nested_vmx_run()
> > > > > > fails and calls nested_vmx_entry_failure().
> nested_vmx_entry_failure
> > > ()
> > > > > > sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but
> where
> > > do
> > > > > > we copy them back to shadow before going back to L1?
> > > > >
> > > > > Good catch! :)
> > > > >
> > > > > Note that the entry path is easy to handle because we copy the
> fields
> > > > > as part of nested_vmx_entry. This is not like exit path where
> > > > > KVM(L0) code can modify fields after nested_vmx_vmexit is called.
> > > > >
> > > > > So here, we could simple call copy_vmcs12_to_shadow if the entry
> fails
> > > > > (as part of nested_vmx_entry_failure or nested_vmx). We could
> optimize
> > > > > the code by updating these specific fields directly, but I don't
> think
> > > > > we really need to optimize code that is part of the error path.
> > > > >
> > > > We needn't.
> > > >
> > > > > > May be we need to introduce vmcs12 accessors to track what is
> changes
> > > > > > and if something need to be copied to shadow before going back to
> L1.
> > > > >
> > > > > That means we will need to modify all the lines of code that uses
> > > > > "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write
> > > function.
> > > > > Inside these inline functions we could access the shadow vmcs
> directly.
> > > > > However, to access the shadow vmcs we need to vmptrld first and
> this
> > > will
> > > > > force
> > > > > unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01)
> each
> > > time
> > > > > the code accesses a vmcs12 field. Alternatively, if we want to
> avoid
> > > > > unnecessary vmptrlds each time we access vmcs12 we could simple set
> a
> > > > > flag that indicates when a shadow field was changed. In this case,
> we
> > > will
> > > > > need to find all the places to check the flag and copy the fields,
> > > > > considering both success and error paths.
> > > > That's not how I see it. nested_vmcs_write() will set a request bit
> in
> > > > vcpu (this is the flag you mention above). The bit will be checked
> during
> > > > a guest entry and vmcs12 will be synced to shadow at this point.
> Later
> > > > we can track what fields were written and sync only them.
> > > >
> > XXX
> >
> > > > > Finally, I am afraid that these directions will introduce new
> issues,
> > > > > will force us to modify too many lines and they may create a
> > > merge/rebase
> > > > > mess...
> > > > Conflicts should be trivial and I do not expect many iterations for
> the
> > > > patch series. I like the approach you take to use vmcs shadowing and
> most
> > > > comment are nitpicks.  I promise to review the next version as soon
> as
> > > > it is posted :)
> > > >
> > > > >
> > > > > Maybe we should simple fix nested_vmx_entry_failure (as well as the
> > > > > other fixes you suggested in other patches) and apply the code.
> > > > > Do you agree ?
> > > > >
> > > > The approach is error prone. Even if we will fix all bugs in the
> current
> > > > code each new nested vmx modification will have to be reviewed with
> > > shadowing
> > > > in mind and sometimes it is hard to see global picture just from a
> > > > patch. It is better to hide shadow details from occasional nested vmx
> > > hacker.
> > >
> > > But how ? I suggested 2 alternatives with pros/cons.
> > I outlined how above (search for XXX). Use accessors, set request bit,
> > sync during guest entry if request bit is set.
> 
> 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.

--
			Gleb.