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

[Abel Gordon <ABELG@il.ibm.com>]

Gleb Natapov <gleb@redhat.com> wrote on 14/04/2013 02:16:02 PM:

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

No, there is no such thing with the patches I submitted.
The shadow VMCS patches assumes that only R/O fields can be shadowed
for read and not for write (the shadow_read_only_fields array). R/O
fields can't be written by L1 thus L0 will trap vmwrite
to any r/o field and emulate a fail
(VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT).

Writable fields are shadowed for both read and write
(the shadow_read_write_fields array).

There are no writable fields shadowed for write and not for read.
There are no writable fields shadowed for read and not for write