[PATCH 0/3] KVM: x86: SVM INVPCID fix, and cleanups

[PATCH 0/3] KVM: x86: SVM INVPCID fix, and cleanups

[Sean Christopherson]

Fix an INVPCID bug on SVM where it fails to injected a #UD when INVPCID is
supported but not exposed to the guest.  Do a bit of cleanup in patch 02
now that both VMX and SVM support PCID/INVPCID.

Patch 03 address KVM behavior that has long confused the heck out of me.
KVM currently allows enabling INVPCID if and only if PCID is also enabled
for the guest, the justification being that the guest will see incorrect
fault behavior (#UD instead of #GP) due to the way the VMCS control works.

But that makes no sense, because nothing is forcing KVM to disable INVCPID
in the VMCS when PCID is disabled.  AFACIT, the myth was the result of a
bug in the original _submission_, not even the original _commit_ was buggy.

Digging back, the very original submission had this code, where
vmx_pcid_supported() was further conditioned on EPT being enabled.  This
would lead to the buggy scenario of unexpected #UD, as a host with PCID
and INVCPID would fail to enable INVPCID if EPT was disabled.

> > +	if (vmx_pcid_supported()) {
> > +		exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> > +		if (exec_control & SECONDARY_EXEC_ENABLE_INVPCID) {
> > +			best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
> > +			if (best && (best->ecx & bit(X86_FEATURE_PCID)))
> > +				vmx->invpcid_enabled = true;
> > +			else {
> > +				exec_control &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> > +				vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> > +						exec_control);
> > +				best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> > +				best->ecx &= ~bit(X86_FEATURE_INVPCID);
> > +			}
> > +		}
> > +	}

The incorrect behavior is especially problematic now that SVM also
supports INVCPID, as KVM allows !PCID && INVPCID on SVM but not on VMX.

Patches to fix kvm-unit-tests are also incoming...

Sean Christopherson (3):
  KVM: SVM: Intercept INVPCID when it's disabled to inject #UD
  KVM: x86: Advertise INVPCID by default
  KVM: VMX: Allow INVPCID in guest without PCID

 arch/x86/kvm/cpuid.c   |  2 +-
 arch/x86/kvm/svm/svm.c | 11 ++++-------
 arch/x86/kvm/vmx/vmx.c | 14 ++------------
 3 files changed, 7 insertions(+), 20 deletions(-)

-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 1/3] KVM: SVM: Intercept INVPCID when it's disabled to inject #UD

[Sean Christopherson]

Intercept INVPCID if it's disabled in the guest, even when using NPT,
as KVM needs to inject #UD in this case.

Fixes: 4407a797e941 ("KVM: SVM: Enable INVPCID feature on AMD")
Cc: Babu Moger <babu.moger@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 42d4710074a6..ca9706c2f99b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1103,12 +1103,12 @@ static u64 svm_write_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
 static void svm_check_invpcid(struct vcpu_svm *svm)
 {
 	/*
-	 * Intercept INVPCID instruction only if shadow page table is
-	 * enabled. Interception is not required with nested page table
-	 * enabled.
+	 * Intercept INVPCID if shadow paging is enabled to sync/free shadow
+	 * roots, or if INVPCID is disabled in the guest to inject #UD.
 	 */
 	if (kvm_cpu_cap_has(X86_FEATURE_INVPCID)) {
-		if (!npt_enabled)
+		if (!npt_enabled ||
+		    !guest_cpuid_has(&svm->vcpu, X86_FEATURE_INVPCID))
 			svm_set_intercept(svm, INTERCEPT_INVPCID);
 		else
 			svm_clr_intercept(svm, INTERCEPT_INVPCID);
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH 1/3] KVM: SVM: Intercept INVPCID when it's disabled to inject #UD

[Jim Mattson]

On Thu, Feb 11, 2021 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Intercept INVPCID if it's disabled in the guest, even when using NPT,
> as KVM needs to inject #UD in this case.
>
> Fixes: 4407a797e941 ("KVM: SVM: Enable INVPCID feature on AMD")
> Cc: Babu Moger <babu.moger@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 2/3] KVM: x86: Advertise INVPCID by default

[Sean Christopherson]

Advertise INVPCID by default (if supported by the host kernel) instead
of having both SVM and VMX opt in.  INVPCID was opt in when it was a
VMX only feature so that KVM wouldn't prematurely advertise support
if/when it showed up in the kernel on AMD hardware.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c   | 2 +-
 arch/x86/kvm/svm/svm.c | 3 ---
 arch/x86/kvm/vmx/vmx.c | 4 ++--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c8f2592ccc99..6bd2f8b830e4 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -408,7 +408,7 @@ void kvm_set_cpu_caps(void)
 
 	kvm_cpu_cap_mask(CPUID_7_0_EBX,
 		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
-		F(BMI2) | F(ERMS) | 0 /*INVPCID*/ | F(RTM) | 0 /*MPX*/ | F(RDSEED) |
+		F(BMI2) | F(ERMS) | F(INVPCID) | F(RTM) | 0 /*MPX*/ | F(RDSEED) |
 		F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
 		F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
 		F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | 0 /*INTEL_PT*/
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ca9706c2f99b..f4eca77f65c6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -926,9 +926,6 @@ static __init void svm_set_cpu_caps(void)
 	if (boot_cpu_has(X86_FEATURE_LS_CFG_SSBD) ||
 	    boot_cpu_has(X86_FEATURE_AMD_SSBD))
 		kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
-
-	/* Enable INVPCID feature */
-	kvm_cpu_cap_check_and_set(X86_FEATURE_INVPCID);
 }
 
 static __init int svm_hardware_setup(void)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e0a3a9be654b..6d265b2523f8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7330,8 +7330,8 @@ static __init void vmx_set_cpu_caps(void)
 	/* CPUID 0x7 */
 	if (kvm_mpx_supported())
 		kvm_cpu_cap_check_and_set(X86_FEATURE_MPX);
-	if (cpu_has_vmx_invpcid())
-		kvm_cpu_cap_check_and_set(X86_FEATURE_INVPCID);
+	if (!cpu_has_vmx_invpcid())
+		kvm_cpu_cap_clear(X86_FEATURE_INVPCID);
 	if (vmx_pt_mode_is_host_guest())
 		kvm_cpu_cap_check_and_set(X86_FEATURE_INTEL_PT);
 
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH 2/3] KVM: x86: Advertise INVPCID by default

[Jim Mattson]

On Thu, Feb 11, 2021 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Advertise INVPCID by default (if supported by the host kernel) instead
> of having both SVM and VMX opt in.  INVPCID was opt in when it was a
> VMX only feature so that KVM wouldn't prematurely advertise support
> if/when it showed up in the kernel on AMD hardware.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 3/3] KVM: VMX: Allow INVPCID in guest without PCID

[Sean Christopherson]

Remove the restriction that prevents VMX from exposing INVPCID to the
guest without PCID also being exposed to the guest.  The justification of
the restriction is that INVPCID will #UD if it's disabled in the VMCS.
While that is a true statement, it's also true that RDTSCP will #UD if
it's disabled in the VMCS.  Neither of those things has any dependency
whatsoever on the guest being able to set CR4.PCIDE=1, which is what is
effectively allowed by exposing PCID to the guest.

Removing the bogus restriction aligns VMX with SVM, and also allows for
an interesting configuration.  INVPCID is that fastest way to do a global
TLB flush, e.g. see native_flush_tlb_global().  Allowing INVPCID without
PCID would let a guest use the expedited flush while also limiting the
number of ASIDs consumed by the guest.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6d265b2523f8..e1b84008a05d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4295,18 +4295,8 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
 	}
 
 	vmx_adjust_sec_exec_feature(vmx, &exec_control, rdtscp, RDTSCP);
-
-	/*
-	 * Expose INVPCID if and only if PCID is also exposed to the guest.
-	 * INVPCID takes a #UD when it's disabled in the VMCS, but a #GP or #PF
-	 * if CR4.PCIDE=0.  Enumerating CPUID.INVPCID=1 would lead to incorrect
-	 * behavior from the guest perspective (it would expect #GP or #PF).
-	 */
-	if (!guest_cpuid_has(vcpu, X86_FEATURE_PCID))
-		guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);
 	vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
 
-
 	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
 	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdseed, RDSEED);
 
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH 3/3] KVM: VMX: Allow INVPCID in guest without PCID

[Jim Mattson]

On Thu, Feb 11, 2021 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Remove the restriction that prevents VMX from exposing INVPCID to the
> guest without PCID also being exposed to the guest.  The justification of
> the restriction is that INVPCID will #UD if it's disabled in the VMCS.
> While that is a true statement, it's also true that RDTSCP will #UD if
> it's disabled in the VMCS.  Neither of those things has any dependency
> whatsoever on the guest being able to set CR4.PCIDE=1, which is what is
> effectively allowed by exposing PCID to the guest.
>
> Removing the bogus restriction aligns VMX with SVM, and also allows for
> an interesting configuration.  INVPCID is that fastest way to do a global
> TLB flush, e.g. see native_flush_tlb_global().  Allowing INVPCID without
> PCID would let a guest use the expedited flush while also limiting the
> number of ASIDs consumed by the guest.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
I always thought this was a bizarre one-off restriction.
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 0/3] KVM: x86: SVM INVPCID fix, and cleanups

[Paolo Bonzini]

On 12/02/21 01:34, Sean Christopherson wrote:
> Fix an INVPCID bug on SVM where it fails to injected a #UD when INVPCID is
> supported but not exposed to the guest.  Do a bit of cleanup in patch 02
> now that both VMX and SVM support PCID/INVPCID.
> 
> Patch 03 address KVM behavior that has long confused the heck out of me.
> KVM currently allows enabling INVPCID if and only if PCID is also enabled
> for the guest, the justification being that the guest will see incorrect
> fault behavior (#UD instead of #GP) due to the way the VMCS control works.
> 
> But that makes no sense, because nothing is forcing KVM to disable INVCPID
> in the VMCS when PCID is disabled.  AFACIT, the myth was the result of a
> bug in the original _submission_, not even the original _commit_ was buggy.
> 
> Digging back, the very original submission had this code, where
> vmx_pcid_supported() was further conditioned on EPT being enabled.  This
> would lead to the buggy scenario of unexpected #UD, as a host with PCID
> and INVCPID would fail to enable INVPCID if EPT was disabled.
> 
>>> +	if (vmx_pcid_supported()) {
>>> +		exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>>> +		if (exec_control & SECONDARY_EXEC_ENABLE_INVPCID) {
>>> +			best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
>>> +			if (best && (best->ecx & bit(X86_FEATURE_PCID)))
>>> +				vmx->invpcid_enabled = true;
>>> +			else {
>>> +				exec_control &= ~SECONDARY_EXEC_ENABLE_INVPCID;
>>> +				vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
>>> +						exec_control);
>>> +				best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
>>> +				best->ecx &= ~bit(X86_FEATURE_INVPCID);
>>> +			}
>>> +		}
>>> +	}
> 
> The incorrect behavior is especially problematic now that SVM also
> supports INVCPID, as KVM allows !PCID && INVPCID on SVM but not on VMX.
> 
> Patches to fix kvm-unit-tests are also incoming...
> 
> Sean Christopherson (3):
>    KVM: SVM: Intercept INVPCID when it's disabled to inject #UD
>    KVM: x86: Advertise INVPCID by default
>    KVM: VMX: Allow INVPCID in guest without PCID
> 
>   arch/x86/kvm/cpuid.c   |  2 +-
>   arch/x86/kvm/svm/svm.c | 11 ++++-------
>   arch/x86/kvm/vmx/vmx.c | 14 ++------------
>   3 files changed, 7 insertions(+), 20 deletions(-)
> 

Queued, thanks.

Paolo