Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

["Radim Krčmář" <rkrcmar@redhat.com>]

[David did a great review, so I'll just point out things I noticed.]

2017-07-11 09:51+0200, David Hildenbrand:
> On 10.07.2017 22:49, Bandan Das wrote:
> > When L2 uses vmfunc, L0 utilizes the associated vmexit to
> > emulate a switching of the ept pointer by reloading the
> > guest MMU.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Bandan Das <bsd@redhat.com>
> > ---
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -7784,11 +7801,46 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
> >  	}
> >  
> >  	vmcs12 = get_vmcs12(vcpu);
> > -	if ((vmcs12->vm_function_control & (1 << function)) == 0)
> > +	if (((vmcs12->vm_function_control & (1 << function)) == 0) ||
> > +	    WARN_ON_ONCE(function))
> 
> "... instruction causes a VM exit if the bit at position EAX is 0 in the
> VM-function controls (the selected VM function is
> not enabled)."
> 
> So g2 can trigger this WARN_ON_ONCE, no? I think we should drop it then
> completely.

It assumes that vm_function_control is not > 1, which is (should be)
guaranteed by VM entry check, because the nested_vmx_vmfunc_controls MSR
is 1.

> > +		goto fail;

The rest of the code assumes that the function is
VMX_VMFUNC_EPTP_SWITCHING, so some WARN_ON_ONCE is reasonable.

Writing it as

  WARN_ON_ONCE(function != VMX_VMFUNC_EPTP_SWITCHING)

would be cleared and I'd prefer to move the part that handles
VMX_VMFUNC_EPTP_SWITCHING into a new function. (Imagine that Intel is
going to add more than one VM FUNC. :])

> > +	if (!nested_cpu_has_ept(vmcs12) ||
> > +	    !nested_cpu_has_eptp_switching(vmcs12))
> > +		goto fail;

This brings me to a missing vm-entry check:

 If “EPTP switching” VM-function control is 1, the “enable EPT”
 VM-execution control must also be 1. In addition, the EPTP-list address
 must satisfy the following checks:
 • Bits 11:0 of the address must be 0.
 • The address must not set any bits beyond the processor’s
   physical-address width.

so this one could be

  if (!nested_cpu_has_eptp_switching(vmcs12) ||
      WARN_ON_ONCE(!nested_cpu_has_ept(vmcs12)))

after adding the check.