Re: [Xen-devel] [PATCH v1 1/2] x86/vvmx: check vmcs address in vmread/vmwrite

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, March 01, 2017 11:40 PM
> 
> >>> On 01.03.17 at 16:23, <sergey.dyasli@xxxxxxxxxx> wrote:
> > On Wed, 2017-03-01 at 07:28 -0700, Jan Beulich wrote:
> >> > > > On 01.03.17 at 15:22, <sergey.dyasli@xxxxxxxxxx> wrote:
> >> >
> >> > On Wed, 2017-03-01 at 07:04 -0700, Jan Beulich wrote:
> >> > > > > > On 01.03.17 at 14:44, <sergey.dyasli@xxxxxxxxxx> wrote:
> >> > > >
> >> > > > Additionally, it would be painful to return the correct error value
> >> > > > all the way back to nvmx_handle_vmptrld().
> >> > >
> >> > > Surely that's at best relevant in the other patch. Here you're in
> >> > > virtual_vmcs_vmwrite_safe(), which already knows how to
> >> > > communicate back an error indicator.
> >> >
> >> > How checking the return value of virtual_vmcs_enter() is different
> >> > from checking nv_vvmcxaddr?
> >>
> >> Checking the address is just one step. As the other patch shows,
> >> checking the ID is necessary too. Over time more such checks may
> >> be found necessary. Checking what hardware hands us is a single
> >> check, and is always going to be sufficient no matter what new
> >> features get added to hardware.
> >
> > Conceptually, the result of __vmptrld() is irrelevant to a guest
> > during nested vmread/vmwrite emulation.  The fact that Xen is doing
> > __vmptrld() is a side effect of nested VMX.
> 
> True.
> 
> > All necessary checks may be found in Intel SDM.
> 
> All _currently_ necessary checks. It is precisely possible new ones
> getting added which I'd like to see covered by other than adding
> further checks to our software when hardware already does them.
> 
> >  And it states that
> > there can be only 1 VMfail if VMCS pointer is not valid: VMfailInvalid.
> > vmptrld can end up in 3 different VMfails and returning them to the
> > guest as a result of vmread/vmwrite emulation is wrong.
> 
> As is crashing Xen because of such.
> 
> The implication of course would be that the insn-error may need
> adjustment in such a case.
> 
> > (Even though each of them will end up being VMfailInvalid in current
> > implementation)
> >
> > I think that Xen's goal in nested VMX is emulating H/W as close as
> > possible.
> 
> Correct. Preferably by leveraging hardware instead of re-doing the
> same thing in software.
> 
> Anyway - we'll see what the VMX maintainers think.
> 

Although leveraging HW check is a generally good idea, I buy-in 
Sergey's comment that we may emulate different VMX feature set 
to guest in the future then in such case we'll need both SW/HW 
checks and then may still need to track latest SDM change.

So:

Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel