| Message ID | 20230310125718.1442088-2-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: > Just rename, no functional changes intended. > > vmx_umip_emulated() comes from the ancient time when there was a No, vmx_umip_emulated() comes from the fact that "cpu_has_vmx_desc()" is inscrutable for the relevant code. There is zero reason to require that readers have a priori knowledge of why intercepting descriptor table access instructions is relevant to handing CR4.UMIP changes. If it really bothers someone, we could do static inline bool cpu_has_vmx_desc(void) { return vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_DESC; } static inline bool vmx_umip_emulated(void) { return cpu_has_vmx_desc(); } but I don't see the point since there is no usage for SECONDARY_EXEC_DESC outside of UMIP emulation.
Sean Christopherson <seanjc@google.com> 于2023年3月10日周五 23:59写道： > > On Fri, Mar 10, 2023, Robert Hoo wrote: > > Just rename, no functional changes intended. > > > > vmx_umip_emulated() comes from the ancient time when there was a > > No, vmx_umip_emulated() comes from the fact that "cpu_has_vmx_desc()" is > inscrutable for the relevant code. There is zero reason to require that readers > have a priori knowledge of why intercepting descriptor table access instructions > is relevant to handing CR4.UMIP changes. I think this is where comments can play its role. > > If it really bothers someone, we could do Yeah, below also came to my mind, as one option. > > static inline bool cpu_has_vmx_desc(void) > { > return vmcs_config.cpu_based_2nd_exec_ctrl & > SECONDARY_EXEC_DESC; > } > > static inline bool vmx_umip_emulated(void) > { > return cpu_has_vmx_desc(); > } > > but I don't see the point since there is no usage for SECONDARY_EXEC_DESC outside > of UMIP emulation. SECONDARY_EXEC_DESC concerns more than UMIP does. UMIP emulation just leverages part of it. Also, vmx_umip_emulated() == true doesn't necessarily mean, as its name indicates, UMIP-being-emulated, e.g. Host has UMIP capability, then UMIP isn't emulated though vmx_umip_emulated() indicates true.
Please fix your editor or whatever it is that is resulting your emails being wrapped at very bizarre boundaries. On Sat, Mar 11, 2023, Robert Hoo wrote: > Also, vmx_umip_emulated() == true doesn't necessarily mean, as its name > indicates, UMIP-being-emulated, e.g. Host has UMIP capability, then UMIP > isn't emulated though vmx_umip_emulated() indicates true. True. I was going to say "there's no perfect solution" since KVM needs to check both "is UMIP emulated" and "can UMIP be emulated", but that's not actually the case. vmx_emulate_umip() _should_ check for native support, as there's no legitimate use for checking if UMIP _can_ be emulated. Functionally, this should be a glorified nop, but I agree it's worth changing. I'll formally post this after testing. From: Sean Christopherson <seanjc@google.com> Date: Wed, 15 Mar 2023 10:27:40 -0700 Subject: [PATCH] KVM: VMX: Treat UMIP as emulated if and only if the host doesn't have UMIP Advertise UMIP as emulated if and only if the host doesn't natively support UMIP, otherwise vmx_umip_emulated() is misleading when the host _does_ support UMIP. Of the four users of vmx_umip_emulated(), two already check for native support, and the logic in vmx_set_cpu_caps() is relevant if and only if UMIP isn't natively supported as UMIP is set in KVM's caps by kvm_set_cpu_caps() when UMIP is present in hardware. That leaves KVM's stuffing of X86_CR4_UMIP into the default cr4_fixed1 value enumerated for nested VMX. In that case, checking for (lack of) host support is actually a bug fix of sorts, as enumerating UMIP support based solely on descriptor table existing works only because KVM doesn't sanity check MSR_IA32_VMX_CR4_FIXED1. E.g. if a (very theoretical) host supported UMIP in hardware but didn't allow UMIP+VMX, KVM would advertise UMIP but not actually emulate UMIP. Of course, KVM would explode long before it could run a nested VM on said theoretical CPU, as KVM doesn't modify host CR4 when enabling VMX. Reported-by: Robert Hoo <robert.hu@intel.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/capabilities.h | 4 ++-- arch/x86/kvm/vmx/nested.c | 3 +-- arch/x86/kvm/vmx/vmx.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h index 45162c1bcd8f..d0abee35d7ba 100644 --- a/arch/x86/kvm/vmx/capabilities.h +++ b/arch/x86/kvm/vmx/capabilities.h @@ -152,8 +152,8 @@ static inline bool cpu_has_vmx_ept(void) static inline bool vmx_umip_emulated(void) { - return vmcs_config.cpu_based_2nd_exec_ctrl & - SECONDARY_EXEC_DESC; + return !boot_cpu_has(X86_FEATURE_UMIP) && + (vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_DESC); } static inline bool cpu_has_vmx_rdtscp(void) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 7c4f5ca405c7..e8347bf2e4fa 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2322,8 +2322,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0 * Preset *DT exiting when emulating UMIP, so that vmx_set_cr4() * will not have to rewrite the controls just for this bit. */ - if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated() && - (vmcs12->guest_cr4 & X86_CR4_UMIP)) + if (vmx_umip_emulated() && (vmcs12->guest_cr4 & X86_CR4_UMIP)) exec_control |= SECONDARY_EXEC_DESC; if (exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 8626010f5d54..c7bd8931eda6 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -3467,7 +3467,7 @@ void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) else hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON; - if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) { + if (vmx_umip_emulated()) { if (cr4 & X86_CR4_UMIP) { secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_DESC); hw_cr4 &= ~X86_CR4_UMIP; base-commit: 0da2a674e56a7fb429eb1f96a3da04d56ec167fd --
Sean Christopherson <seanjc@google.com> 于2023年3月16日周四 01:50写道： > > Please fix your editor or whatever it is that is resulting your emails being > wrapped at very bizarre boundaries. > (Sorry for late reply.) Yes, I also noticed this. Just began using Gmail web portal for community mails. I worried that it has no auto wrapping (didn't find the setting), so manually wrapped; but now looks like it has some. Give me some time, I'm going to switch to some mail client. Welcome suggestions of mail clients which is suited for community communications, on Windows platform.
On Fri, Mar 31, 2023, Robert Hoo wrote: > Sean Christopherson <seanjc@google.com> 于2023年3月16日周四 01:50写道： > > > > Please fix your editor or whatever it is that is resulting your emails being > > wrapped at very bizarre boundaries. > > > (Sorry for late reply.) > Yes, I also noticed this. Just began using Gmail web portal for community mails. > I worried that it has no auto wrapping (didn't find the setting), so manually > wrapped; but now looks like it has some. > Give me some time, I'm going to switch to some mail client. > Welcome suggestions of mail clients which is suited for community > communications, on Windows platform.
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h index 45162c1bcd8f..afa116063acd 100644 --- a/arch/x86/kvm/vmx/capabilities.h +++ b/arch/x86/kvm/vmx/capabilities.h @@ -150,7 +150,7 @@ static inline bool cpu_has_vmx_ept(void) SECONDARY_EXEC_ENABLE_EPT; } -static inline bool vmx_umip_emulated(void) +static inline bool cpu_has_vmx_desc(void) { return vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_DESC; diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 7c4f5ca405c7..6804b4fcf2b9 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2322,7 +2322,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0 * Preset *DT exiting when emulating UMIP, so that vmx_set_cr4() * will not have to rewrite the controls just for this bit. */ - if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated() && + if (!boot_cpu_has(X86_FEATURE_UMIP) && cpu_has_vmx_desc() && (vmcs12->guest_cr4 & X86_CR4_UMIP)) exec_control |= SECONDARY_EXEC_DESC; @@ -6984,7 +6984,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps) rdmsrl(MSR_IA32_VMX_CR0_FIXED1, msrs->cr0_fixed1); rdmsrl(MSR_IA32_VMX_CR4_FIXED1, msrs->cr4_fixed1); - if (vmx_umip_emulated()) + if (cpu_has_vmx_desc()) msrs->cr4_fixed1 |= X86_CR4_UMIP; msrs->vmcs_enum = nested_vmx_calc_vmcs_enum_msr(); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index bcac3efcde41..96f7c9f37afd 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -3431,7 +3431,13 @@ void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) else hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON; - if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) { + /* + * Emulate UMIP via enable secondary_exec_control.DESC + * It can get here means it has passed valid_cr4() check, i.e. + * 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 (cr4 & X86_CR4_UMIP) { secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_DESC); hw_cr4 &= ~X86_CR4_UMIP; @@ -7820,7 +7826,7 @@ static __init void vmx_set_cpu_caps(void) kvm_cpu_cap_clear(X86_FEATURE_SGX2); } - if (vmx_umip_emulated()) + if (cpu_has_vmx_desc()) kvm_cpu_cap_set(X86_FEATURE_UMIP); /* CPUID 0xD.1 */
Just rename, no functional changes intended. vmx_umip_emulated() comes from the ancient time when there was a kvm_x86_ops::umip_emulated(), which originally simply returned false. (66336cab3531d "KVM: x86: add support for emulating UMIP"). Afterwards, its body changed and moved from vmx.c to the dedicated capabilities.h, but kept its old name which looks weired among cpu_has_vmx_XXX() group. Rename it to align with other analogous functions, the new name is more accurate for what it does. And, vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_DESC just means it has the capability of emulating UMIP, not *umip-being-emulated*, e.g. if host has caps of UMIP, it's actually not emulated. On the other hand, UMIP concerned instructions are just subset of those SECONDARY_EXEC_DESC intercepts [1][2]. [1] SDM. Vol.3 Table 25-7. Definitions of Secondary Processor-Based VM-Execution Controls SECONDARY_EXEC_DESC "determines whether executions of LGDT, LIDT, LLDT, LTR, SGDT, SIDT, SLDT, and STR cause VM exits." [2] SDM. Vol.3 2.5 Control Registers CR4.UMIP is about SGDT, SIDT, SLDT, SMSW, and STR. Signed-off-by: Robert Hoo <robert.hu@intel.com> --- arch/x86/kvm/vmx/capabilities.h | 2 +- arch/x86/kvm/vmx/nested.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 10 ++++++++-- 3 files changed, 11 insertions(+), 5 deletions(-)