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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Fri, Mar 31, 2023, Robert Hoo wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> 于2023年3月16日周四 00:36写道：
> > > 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.
> >
> 
> Sorry still don't follow you. Do you mean in nested case? the "guest"
> above is L1?

Please take the time to walk through the code with possible inputs/scenarios before
asking these types of questions, e.g. if necessary use a whiteboard, pen+paper, etc.
I'm happy to explain subtleties and add answer specific questions, but as evidenced
by my delayed response, I simply do not have the bandwidth to answer questions where
the answer is literally a trace-through of a small, fully contained section of code.

	if (!boot_cpu_has(X86_FEATURE_UMIP)) {    <= evaluates true when UMIP is NOT supported
		if (cr4 & X86_CR4_UMIP) {         <= evaluates false when guest.CR4.UMIP == 0
			secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_DESC);
			hw_cr4 &= ~X86_CR4_UMIP;
		} else if (!is_guest_mode(vcpu) || <= evalutes true when L2 is NOT active
			!nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) {
			secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_DESC); <= KVM "blindly" writes secondary controls
		}
	}