Radim Krčmář <rkrcmar@xxxxxxxxxx> writes: > 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. Ok, lemme switch this to a switch statement style handler function. That way, future additions would be easier. > Btw. have you looked what we'd need to do for the hardware pass-through? > I'd expect big changes to MMU. :) Yes, the first version was actually using vmfunc 0 directly, well not exatly, the first time would go through this path and then the next time the processor would handle it directly. Paolo pointed out an issue that I was ready to fix but he wasn't comfortable with the idea. I actually agree with him, it's too much untested code for something that would be rarely used. >> >> > + 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). Actually, I misread 25.5.5.3. There are two checks. Here, the list entry needs to be checked so that eptp won't cause a vmentry failure. The vmentry needs to check the eptp list address itself. I will add that check for the list address in the next version. Bandan
