Re: [PATCH 2/3] KVM: VMX: Remove a unnecessary cpu_has_vmx_desc() check in vmx_set_cr4()

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Sat, Mar 11, 2023, Robert Hoo wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> 于2023年3月11日周六 00:12写道：
> >
> > On Fri, Mar 10, 2023, Robert Hoo wrote:
> > > Remove the unnecessary cpu_has_vmx_desc() check for emulating UMIP.
> >
> > It's not unnecessary.  See commit 64f7a11586ab ("KVM: vmx: update sec exec controls
> > for UMIP iff emulating UMIP").  Dropping the check will cause KVM to execute
> >
> >         secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_DESC);
> >
> > on CPUs that don't have SECONDARY_VM_EXEC_CONTROL.
> 
> Sorry I don't follow you.
> My point is that, given it has passed kvm_is_valid_cr4() (in kvm_set_cr4()),
> we can assert boot_cpu_has(X86_FEATURE_UMIP)  and vmx_umip_emulated() must be
> at least one true.

This assertion is wrong for the case where guest.CR4.UMIP=0.  The below code is
not guarded with a check on guest.CR4.UMIP.  If the vmx_umip_emulated() check goes
away and guest.CR4.UMIP=0, KVM will attempt to write secondary controls.

Technically, now that controls_shadow exists, KVM won't actually do a VMWRITE,
but I most definitely don't want to rely on controls_shadow for functional
correctness.  And controls_shadow aside, the "vmx_umip_emulated()" effectively
serves as documentation for why KVM is mucking with UMIP when it's obviously not
supported in hardware.

	if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) {
		if (cr4 & X86_CR4_UMIP) {
			secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_DESC);
			hw_cr4 &= ~X86_CR4_UMIP;
		} else if (!is_guest_mode(vcpu) ||
			!nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) {
			secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_DESC);
		}
	}