[PATCH] kvm: x86: Use KVM CPU capabilities to determine CR4 reserved bits

[PATCH] kvm: x86: Use KVM CPU capabilities to determine CR4 reserved bits

[Paolo Bonzini]

Using CPUID data can be useful for the processor compatibility
check, but that's it.  Using it to compute guest-reserved bits
can have both false positives (such as LA57 and UMIP which we
are already handling) and false negatives: in particular, with
this patch we don't allow anymore a KVM guest to set CR4.PKE
when CR4.PKE is clear on the host.

Fixes: b9dd21e104bc ("KVM: x86: simplify handling of PKRU")
Reported-by: Jim Mattson <jmattson@google.com>
Tested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 45688d075044..e0639b2c332e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -929,19 +929,6 @@ EXPORT_SYMBOL_GPL(kvm_set_xcr);
 	__reserved_bits;				\
 })
 
-static u64 kvm_host_cr4_reserved_bits(struct cpuinfo_x86 *c)
-{
-	u64 reserved_bits = __cr4_reserved_bits(cpu_has, c);
-
-	if (kvm_cpu_cap_has(X86_FEATURE_LA57))
-		reserved_bits &= ~X86_CR4_LA57;
-
-	if (kvm_cpu_cap_has(X86_FEATURE_UMIP))
-		reserved_bits &= ~X86_CR4_UMIP;
-
-	return reserved_bits;
-}
-
 static int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	if (cr4 & cr4_reserved_bits)
@@ -9674,7 +9661,9 @@ int kvm_arch_hardware_setup(void *opaque)
 	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
 		supported_xss = 0;
 
-	cr4_reserved_bits = kvm_host_cr4_reserved_bits(&boot_cpu_data);
+#define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
+	cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
+#undef __kvm_cpu_cap_has
 
 	if (kvm_has_tsc_control) {
 		/*
@@ -9706,7 +9695,8 @@ int kvm_arch_check_processor_compat(void *opaque)
 
 	WARN_ON(!irqs_disabled());
 
-	if (kvm_host_cr4_reserved_bits(c) != cr4_reserved_bits)
+	if (__cr4_reserved_bits(cpu_has, c) !=
+	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
 		return -EIO;
 
 	return ops->check_processor_compatibility();
-- 
2.18.2

Re: [PATCH] kvm: x86: Use KVM CPU capabilities to determine CR4 reserved bits

[Xiaoyao Li]

On 5/6/2020 5:44 PM, Paolo Bonzini wrote:
> Using CPUID data can be useful for the processor compatibility
> check, but that's it.  Using it to compute guest-reserved bits
> can have both false positives (such as LA57 and UMIP which we
> are already handling) and false negatives: 

> in particular, with
> this patch we don't allow anymore a KVM guest to set CR4.PKE
> when CR4.PKE is clear on the host.

A common question about whether a feature can be exposed to guest:

Given a feature, there is a CPUID bit to enumerate it, and a CR4 bit to 
turn it on/off. Whether the feature can be exposed to guest only depends 
on host CR4 setting? I.e., if CPUID bit is not cleared in cpu_data in 
host but host kernel doesn't set the corresponding CR4 bit to turn it 
on, we cannot expose the feature to guest. right?

Re: [PATCH] kvm: x86: Use KVM CPU capabilities to determine CR4 reserved bits

[Paolo Bonzini]

On 18/05/20 06:52, Xiaoyao Li wrote:
> On 5/6/2020 5:44 PM, Paolo Bonzini wrote:
>> Using CPUID data can be useful for the processor compatibility
>> check, but that's it.  Using it to compute guest-reserved bits
>> can have both false positives (such as LA57 and UMIP which we
>> are already handling) and false negatives: 
> 
>> in particular, with
>> this patch we don't allow anymore a KVM guest to set CR4.PKE
>> when CR4.PKE is clear on the host.
> 
> A common question about whether a feature can be exposed to guest:
> 
> Given a feature, there is a CPUID bit to enumerate it, and a CR4 bit to
> turn it on/off. Whether the feature can be exposed to guest only depends
> on host CR4 setting? I.e., if CPUID bit is not cleared in cpu_data in
> host but host kernel doesn't set the corresponding CR4 bit to turn it
> on, we cannot expose the feature to guest. right?

It depends.  The most obvious case is that the host kernel doesn't use
CR4.PSE but we even use 4MB pages to emulate paging disabled mode when
the processor doesn't support unrestricted guests.

Basically, the question is whether we are able to save/restore any
processor state attached to the CR4 bit on vmexit/vmentry.  In this case
there is no PKRU field in the VMCS and the RDPKRU/WRPKRU instructions
require CR4.PKE=1; therefore, we cannot let the guest enable CR4.PKE
unless it's also enabled on the host.

Paolo

Re: [PATCH] kvm: x86: Use KVM CPU capabilities to determine CR4 reserved bits

[Xiaoyao Li]

On 5/18/2020 8:31 PM, Paolo Bonzini wrote:
> On 18/05/20 06:52, Xiaoyao Li wrote:
>> On 5/6/2020 5:44 PM, Paolo Bonzini wrote:
>>> Using CPUID data can be useful for the processor compatibility
>>> check, but that's it.  Using it to compute guest-reserved bits
>>> can have both false positives (such as LA57 and UMIP which we
>>> are already handling) and false negatives:
>>
>>> in particular, with
>>> this patch we don't allow anymore a KVM guest to set CR4.PKE
>>> when CR4.PKE is clear on the host.
>>
>> A common question about whether a feature can be exposed to guest:
>>
>> Given a feature, there is a CPUID bit to enumerate it, and a CR4 bit to
>> turn it on/off. Whether the feature can be exposed to guest only depends
>> on host CR4 setting? I.e., if CPUID bit is not cleared in cpu_data in
>> host but host kernel doesn't set the corresponding CR4 bit to turn it
>> on, we cannot expose the feature to guest. right?
> 
> It depends.  The most obvious case is that the host kernel doesn't use
> CR4.PSE but we even use 4MB pages to emulate paging disabled mode when
> the processor doesn't support unrestricted guests.
> 
> Basically, the question is whether we are able to save/restore any
> processor state attached to the CR4 bit on vmexit/vmentry.  In this case
> there is no PKRU field in the VMCS and the RDPKRU/WRPKRU instructions
> require CR4.PKE=1; therefore, we cannot let the guest enable CR4.PKE
> unless it's also enabled on the host.
> 

aha! That's reason!
Thanks for the clarification.