[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. :])
> > + 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.
