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

Message

Sean Christopherson Feb. 12, 2021, 12:34 a.m. UTC

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

Comments

Paolo Bonzini Feb. 15, 2021, 4:45 p.m. UTC | #1

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