If PCID is not exposed to the guest, clear INVPCID in the guest's CPUID
even if the VMCS INVPCID enable is not supported. This will allow
consolidating the secondary execution control adjustment code without
having to special case INVPCID.
Technically, this fixes a bug where !CPUID.PCID && CPUID.INVCPID would
result in unexpected guest behavior (#UD instead of #GP/#PF), but KVM
doesn't support exposing INVPCID if it's not supported in the VMCS, i.e.
such a config is broken/bogus no matter what.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index cfed29329e4f..57e48c5a1e91 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4149,16 +4149,22 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
}
}
+ /*
+ * 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);
+
if (cpu_has_vmx_invpcid()) {
/* Exposing INVPCID only when PCID is exposed */
bool invpcid_enabled =
- guest_cpuid_has(vcpu, X86_FEATURE_INVPCID) &&
- guest_cpuid_has(vcpu, X86_FEATURE_PCID);
+ guest_cpuid_has(vcpu, X86_FEATURE_INVPCID);
- if (!invpcid_enabled) {
+ if (!invpcid_enabled)
exec_control &= ~SECONDARY_EXEC_ENABLE_INVPCID;
- guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);
- }
if (nested) {
if (invpcid_enabled)
--
2.28.0
On Wed, Sep 23, 2020 at 9:51 AM Sean Christopherson
<[email protected]> wrote:
>
> If PCID is not exposed to the guest, clear INVPCID in the guest's CPUID
> even if the VMCS INVPCID enable is not supported. This will allow
> consolidating the secondary execution control adjustment code without
> having to special case INVPCID.
>
> Technically, this fixes a bug where !CPUID.PCID && CPUID.INVCPID would
> result in unexpected guest behavior (#UD instead of #GP/#PF), but KVM
> doesn't support exposing INVPCID if it's not supported in the VMCS, i.e.
> such a config is broken/bogus no matter what.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index cfed29329e4f..57e48c5a1e91 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4149,16 +4149,22 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
> }
> }
>
> + /*
> + * 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);
> +
I thought the general rule for userspace provided CPUID bits was that
kvm only made any adjustments necessary to prevent bad things from
happening at the host level. Proper guest semantics are entirely the
responsibility of userspace. Or did I misunderstand?
On 23/09/20 19:15, Jim Mattson wrote:
> On Wed, Sep 23, 2020 at 9:51 AM Sean Christopherson
> <[email protected]> wrote:
>>
>> If PCID is not exposed to the guest, clear INVPCID in the guest's CPUID
>> even if the VMCS INVPCID enable is not supported. This will allow
>> consolidating the secondary execution control adjustment code without
>> having to special case INVPCID.
>>
>> Technically, this fixes a bug where !CPUID.PCID && CPUID.INVCPID would
>> result in unexpected guest behavior (#UD instead of #GP/#PF), but KVM
>> doesn't support exposing INVPCID if it's not supported in the VMCS, i.e.
>> such a config is broken/bogus no matter what.
>>
>> Signed-off-by: Sean Christopherson <[email protected]>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index cfed29329e4f..57e48c5a1e91 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4149,16 +4149,22 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
>> }
>> }
>>
>> + /*
>> + * 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);
>> +
>
> I thought the general rule for userspace provided CPUID bits was that
> kvm only made any adjustments necessary to prevent bad things from
> happening at the host level. Proper guest semantics are entirely the
> responsibility of userspace. Or did I misunderstand?
>
Yes, that's generally the idea. INVPCID has always been a bit special
due to the secondary execution control being of the "enable" kind; this
led the original author to try and disable the instruction (which is by
itself something we do not always do, and sometimes cannot always do).
So I agree that Sean's patch is of marginal utility by itself; however
it lets him use the new macros in patch 4 and it is a good idea to
separate the small functional change into its own commit.
Paolo