[PATCH 0/3] Some code refactor surround CR4.UMIP virtualization

[PATCH 0/3] Some code refactor surround CR4.UMIP virtualization

[Robert Hoo]

This is some small code refactor/cleanup surround CR4.UMIP virtualization.

vmx_umip_emulated() is misleading, it is actually cpu_has_vmx_desc(). E.g.,
if host has both UMIP cap and VMX_FEATURE_DESC_EXITING feature,
vmx_umip_emulated() returns true, but UMIP in fact isn't emulated. Rename
it.

Also, vCPU can have UMIP feature (i.e. CR4.UMIP is valid) if and only if
host has UMIP and/or VMX_FEATURE_DESC_EXITING cap, (see vmx_set_cpu_caps());
then checking cpu_has_vmx_desc() for emulating UMIP is redundant, because
boot_cpu_has(X86_FEATURE_UMIP) and cpu_has_vmx_desc() must at least one be
true. 

===Test===
kvm-unit-test umip cases, enabled and disabled (unittests.cfg patch below),
pass. But currently I have no machine with UMIP feature, so actually only
emulation path is covered.
In January, test was carried out on Simics platform with UMIP. Passed.

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 08a9b20..32a3b79 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -323,6 +323,10 @@ arch = x86_64
 file = umip.flat
 extra_params = -cpu qemu64,+umip
 
+[umip-disabled]
+file = umip.flat
+extra_params = -cpu qemu64,-umip
+


Robert Hoo (3):
  KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()
  KVM: VMX: Remove a unnecessary cpu_has_vmx_desc() check in
    vmx_set_cr4()
  KVM: VMX: Use the canonical interface to read CR4.UMIP bit

 arch/x86/kvm/vmx/capabilities.h |  2 +-
 arch/x86/kvm/vmx/nested.c       |  4 ++--
 arch/x86/kvm/vmx/vmx.c          | 12 +++++++++---
 3 files changed, 12 insertions(+), 6 deletions(-)


base-commit: 45dd9bc75d9adc9483f0c7d662ba6e73ed698a0b
-- 
2.31.1

[PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Robert Hoo]

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(-)

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 */
-- 
2.31.1

Re: [PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Sean Christopherson]

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.

Re: [PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Robert Hoo]

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.

Re: [PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Sean Christopherson]

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
-- 

Re: [PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Robert Hoo]

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 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.
>
[...]
> there's no
> legitimate use for checking if UMIP _can_ be emulated.

Agree.
>
> Functionally, this should be a glorified nop,

Personally, I think it is more than this; good naming, not misleading
its user, saving their time to look into, is always good.

> but I agree it's worth changing.
> I'll formally post this after testing.

Thanks, looks good to me, it enriches the original patch, i.e. the
cr4_fixed1 part is beyond my original findings. Just don't quite
understand the last paragraph of the description. I ask inline below.
>
> 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.
>
Sorry I don't fully understand the paragraph below, though I believe
you're right.:)

> 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,

What bug?
By your assumption below:
    * host supports UMIP, host doesn't allow (nested?) vmx
    * UMIP enumerated to L1, L1 thinks it has UMIP capability and
enumerates to L2
    * L1 MSR_IA32_VMX_CR4_FIXED1.UMIP is set (meaning allow 1, not fixed to 1)

L2 can use UMIP, no matter L1's UMIP capability is backed by L0 host's
HW UMIP or L0's vmx emulation, I don't see any problem. Shed more
light?

> as enumerating UMIP support
> based solely on descriptor table

What "descriptor table" do you mean here?

> existing works only because KVM doesn't
> sanity check MSR_IA32_VMX_CR4_FIXED1.

Emm, nested_cr4_valid() should be applied to vmx_set_cr4()?

> 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.

Re: [PATCH 1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc()

[Sean Christopherson]

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.🙂

Heh, none?  The "on Windows" is a bit problematic.  Sorry I can't help on this
front.

> > 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,
> 
> What bug?
> By your assumption below:
>     * host supports UMIP, host doesn't allow (nested?) vmx
>     * UMIP enumerated to L1, L1 thinks it has UMIP capability and
> enumerates to L2
>     * L1 MSR_IA32_VMX_CR4_FIXED1.UMIP is set (meaning allow 1, not fixed to 1)
> 
> L2 can use UMIP, no matter L1's UMIP capability is backed by L0 host's
> HW UMIP or L0's vmx emulation, I don't see any problem. Shed more
> light?
> 
> > as enumerating UMIP support
> > based solely on descriptor table
> 
> What "descriptor table" do you mean here?

There's a typo below.  It should be "exiting", not "existing".  As in "descriptor
table exiting", i.e. the feature that allows KVM to intercept LGDT and friends.

> > existing works only because KVM doesn't
> > sanity check MSR_IA32_VMX_CR4_FIXED1.
> 
> Emm, nested_cr4_valid() should be applied to vmx_set_cr4()?

No, what this is saying is that if a (virtual) CPU does support UMIP for bare
metal (from the host's perspective), but does NOT allow UMIP to be set in a VMX
guest's CR4, then KVM would end up advertising UMIP to L1 but would neither
virtualize (set in hardware) nor emulate UMIP for L1.

The blurb about KVM exploding is calling out that in this very, very theoretical
scenario, KVM will fail on the very first VM-Entry (if not before) as KVM uses the
host kernel's CR4 verbatim when setting vmcs.HOST_CR4, i.e. will fail the consistency
check on a "cannot be 1" bits being set in HOST_CR4.

> > 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.

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

[Robert Hoo]

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(-)

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;
-- 
2.31.1

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

[Sean Christopherson]

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.

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

[Robert Hoo]

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.

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

[Sean Christopherson]

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);
		}
	}

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

[Robert Hoo]

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);
>                 }
>         }

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

[Sean Christopherson]

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
		}
	}

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

[Hoo Robert]

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
> 		}
> 	}

[PATCH 3/3] KVM: VMX: Use the canonical interface to read CR4.UMIP bit

[Robert Hoo]

Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that
we have reg cache layer and defined this wrapper.
Although, effectively for CR4.UMIP, it's the same, at present, as it's not
guest owned, in case of future changes, here better to use the canonical
interface.

Signed-off-by: Robert Hoo <robert.hu@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bec5792acffe..8853853def56 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5435,7 +5435,7 @@ static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val)
 
 static int handle_desc(struct kvm_vcpu *vcpu)
 {
-	WARN_ON(!(vcpu->arch.cr4 & X86_CR4_UMIP));
+	WARN_ON(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP));
 	return kvm_emulate_instruction(vcpu, 0);
 }
 
-- 
2.31.1

Re: [PATCH 3/3] KVM: VMX: Use the canonical interface to read CR4.UMIP bit

[Sean Christopherson]

On Fri, Mar 10, 2023, Robert Hoo wrote:
> Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that
> we have reg cache layer and defined this wrapper.

kvm_read_cr4_bits() predates this code by ~7 years.

> Although, effectively for CR4.UMIP, it's the same, at present, as it's not
> guest owned, in case of future changes, here better to use the canonical
> interface.

Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation.
I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog.

This would also be a good time to opportunistically convert the WARN_ON() to a
WARN_ON_ONCE() (when it fires, it fires a _lot).

This, with a reworded changelog?

	/*
	 * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this
	 * and other code needs to be updated if UMIP can be guest owned.
	 */
	BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP);

	WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP));
	return kvm_emulate_instruction(vcpu, 0);

Re: [PATCH 3/3] KVM: VMX: Use the canonical interface to read CR4.UMIP bit

[Sean Christopherson]

On Sat, Mar 11, 2023, Robert Hoo wrote:
> Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道：
> > Practically speaking, UMIP _can't_ be guest owned without breaking UMIP
> emulation.
> 
> Agree.
> Can we expect that in the future (not near future) that emulating UMIP
> isn't needed at all? I mean, when all x86 CPUs on the market have UMIP in native.

Heh, define "all x86s on the market".  UMIP didn't come along until Broadwell, and
we still get bug reports for Core2.  It's going to be a long, long time before
KVM can require UMIP support on Intel CPUs.

Re: [PATCH 3/3] KVM: VMX: Use the canonical interface to read CR4.UMIP bit

[Robert Hoo]

Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道：
>
> On Fri, Mar 10, 2023, Robert Hoo wrote:
> > Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that
> > we have reg cache layer and defined this wrapper.
>
> kvm_read_cr4_bits() predates this code by ~7 years.
>
> > Although, effectively for CR4.UMIP, it's the same, at present, as it's not
> > guest owned, in case of future changes, here better to use the canonical
> > interface.
>
> Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation.
> I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog.
>
> This would also be a good time to opportunistically convert the WARN_ON() to a
> WARN_ON_ONCE() (when it fires, it fires a _lot).
>
> This, with a reworded changelog?
>
>         /*
>          * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this
>          * and other code needs to be updated if UMIP can be guest owned.
>          */
>         BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP);
>
>         WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP));
>         return kvm_emulate_instruction(vcpu, 0);

Are you going to have this along with your "[PATCH] KVM: VMX: Treat
UMIP as emulated if and only if the host doesn't have UMIP"?

Re: [PATCH 3/3] KVM: VMX: Use the canonical interface to read CR4.UMIP bit

[Sean Christopherson]

On Fri, Mar 31, 2023, Robert Hoo wrote:
> Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道：
> >
> > On Fri, Mar 10, 2023, Robert Hoo wrote:
> > > Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that
> > > we have reg cache layer and defined this wrapper.
> >
> > kvm_read_cr4_bits() predates this code by ~7 years.
> >
> > > Although, effectively for CR4.UMIP, it's the same, at present, as it's not
> > > guest owned, in case of future changes, here better to use the canonical
> > > interface.
> >
> > Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation.
> > I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog.
> >
> > This would also be a good time to opportunistically convert the WARN_ON() to a
> > WARN_ON_ONCE() (when it fires, it fires a _lot).
> >
> > This, with a reworded changelog?
> >
> >         /*
> >          * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this
> >          * and other code needs to be updated if UMIP can be guest owned.
> >          */
> >         BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP);
> >
> >         WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP));
> >         return kvm_emulate_instruction(vcpu, 0);
> 
> Are you going to have this along with your "[PATCH] KVM: VMX: Treat
> UMIP as emulated if and only if the host doesn't have UMIP"?

Sure, I'll add a patch for that.