| Message ID | 20230310125718.1442088-3-robert.hu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc() | expand |
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.
Sean Christopherson <seanjc@google.com> 于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. Therefore when !boot_cpu_has(X86_FEATURE_UMIP), vmx_umip_emulated() must be true, therefore checking it is redundant. Do you mean other call path other than kvm_set_cr4() --> vmx_set_cr4()? i.e. vmx_set_cr0() --> vmx_set_cr4()? nested_... --> vmx_set_cr4()? Emm, they seem don't go through kvm_is_valid_cr4() firstly.
On Sat, Mar 11, 2023, Robert Hoo wrote: > Sean Christopherson <seanjc@google.com> 于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); } }
Sean Christopherson <seanjc@google.com> 于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? > 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); > } > }
On Fri, Mar 31, 2023, Robert Hoo wrote: > Sean Christopherson <seanjc@google.com> 于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 } }
On 2023/4/11 2:35, Sean Christopherson wrote: > On Fri, Mar 31, 2023, Robert Hoo wrote: >> Sean Christopherson <seanjc@google.com> 于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. Sure, fully understand your busyness, thanks for still providing detailed explanation. Sorry for the annoyance. Given that you've merged host UMIP cap check into vmx_umip_emulated(), I might have not tangled in this point. We can close this thread of patch set now. > > 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 > } > }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 96f7c9f37afd..bec5792acffe 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -3437,7 +3437,7 @@ void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) * guest been exposed with UMIP feature, i.e. either host has cap * of UMIP or vmx_set_cpu_caps() set it because of cpu_has_vmx_desc() */ - if (!boot_cpu_has(X86_FEATURE_UMIP) && cpu_has_vmx_desc()) { + if (!boot_cpu_has(X86_FEATURE_UMIP)) { if (cr4 & X86_CR4_UMIP) { secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_DESC); hw_cr4 &= ~X86_CR4_UMIP;
Remove the unnecessary cpu_has_vmx_desc() check for emulating UMIP. When it arrives vmx_set_cr4(), it has passed kvm_is_valid_cr4(), meaning vmx_set_cpu_caps() has already assign X86_FEATURE_UMIP with this vcpu, because of either host CPU has the capability or it can be emulated, vmx_set_cpu_caps(): if (cpu_has_vmx_desc()) kvm_cpu_cap_set(X86_FEATURE_UMIP); that is to say, if !boot_cpu_has(X86_FEATURE_UMIP) here, cpu_has_vmx_desc() must be true, it should be a meaningless check here. Signed-off-by: Robert Hoo <robert.hu@intel.com> --- arch/x86/kvm/vmx/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)