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).
