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

[Radim KrÄmÃÅ]

2017-07-11 14:05-0400, Bandan Das:
> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> writes:
> 
> > [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@xxxxxxxxxx>
> >> > Signed-off-by: Bandan Das <bsd@xxxxxxxxxx>
> >> > ---
> >> > 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. :])
> 
> IMO, for now, this should be fine because we are not even passing through the
> hardware's eptp switching. Even if there are other vm functions, they
> won't be available for the nested case and cause any conflict.

Yeah, it is fine function-wise, I was just pointing out that it looks
ugly to me.

Btw. have you looked what we'd need to do for the hardware pass-through?
I'd expect big changes to MMU. :)

> >> > +	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)))
> 
> I will reverse the order here but the vm entry check is unnecessary because
> the check on the list address is already being done in this function.

Here is too late, the nested VM-entry should have failed, never letting
this situation happen.  We want an equivalent of

  if (nested_cpu_has_eptp_switching(vmcs12) && !nested_cpu_has_ept(vmcs12))
  	return VMXERR_ENTRY_INVALID_CONTROL_FIELD;

in nested controls checks, right next to the reserved fields check.
And then also the check EPTP-list check.  All of them only checked when
nested_cpu_has_vmfunc(vmcs12).