[PATCH v2] kvm/svm: PKU not currently supported

[PATCH v2] kvm/svm: PKU not currently supported

[John Allen]

Current SVM implementation does not have support for handling PKU. Guests
running on a host with future AMD cpus that support the feature will read
garbage from the PKRU register and will hit segmentation faults on boot as
memory is getting marked as protected that should not be. Ensure that cpuid
from SVM does not advertise the feature.

Signed-off-by: John Allen <john.allen@amd.com>
---
v2:
  -Introduce kvm_x86_ops->pku_supported()
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/cpuid.c            | 4 +++-
 arch/x86/kvm/svm.c              | 6 ++++++
 arch/x86/kvm/vmx/capabilities.h | 5 +++++
 arch/x86/kvm/vmx/vmx.c          | 1 +
 5 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b79cd6aa4075..4a9d869465ad 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1145,6 +1145,7 @@ struct kvm_x86_ops {
 	bool (*xsaves_supported)(void);
 	bool (*umip_emulated)(void);
 	bool (*pt_supported)(void);
+	bool (*pku_supported)(void);
 
 	int (*check_nested_events)(struct kvm_vcpu *vcpu, bool external_intr);
 	void (*request_immediate_exit)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index cfafa320a8cf..ace146d663db 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -352,6 +352,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
 	unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
 	unsigned f_intel_pt = kvm_x86_ops->pt_supported() ? F(INTEL_PT) : 0;
 	unsigned f_la57;
+	unsigned f_pku = kvm_x86_ops->pku_supported() ? F(PKU) : 0;
 
 	/* cpuid 7.0.ebx */
 	const u32 kvm_cpuid_7_0_ebx_x86_features =
@@ -363,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
 
 	/* cpuid 7.0.ecx*/
 	const u32 kvm_cpuid_7_0_ecx_x86_features =
-		F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) |
+		F(AVX512VBMI) | F(LA57) | 0 /*PKU*/ | 0 /*OSPKE*/ | F(RDPID) |
 		F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
 		F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
 		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
@@ -392,6 +393,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
 		/* Set LA57 based on hardware capability. */
 		entry->ecx |= f_la57;
 		entry->ecx |= f_umip;
+		entry->ecx |= f_pku;
 		/* PKU is not yet implemented for shadow paging. */
 		if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
 			entry->ecx &= ~F(PKU);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 122d4ce3b1ab..8b0620f3aed6 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6001,6 +6001,11 @@ static bool svm_has_wbinvd_exit(void)
 	return true;
 }
 
+static bool svm_pku_supported(void)
+{
+	return false;
+}
+
 #define PRE_EX(exit)  { .exit_code = (exit), \
 			.stage = X86_ICPT_PRE_EXCEPT, }
 #define POST_EX(exit) { .exit_code = (exit), \
@@ -7341,6 +7346,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 	.xsaves_supported = svm_xsaves_supported,
 	.umip_emulated = svm_umip_emulated,
 	.pt_supported = svm_pt_supported,
+	.pku_supported = svm_pku_supported,
 
 	.set_supported_cpuid = svm_set_supported_cpuid,
 
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index 7aa69716d516..283bdb7071af 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -145,6 +145,11 @@ static inline bool vmx_umip_emulated(void)
 		SECONDARY_EXEC_DESC;
 }
 
+static inline bool vmx_pku_supported(void)
+{
+	return boot_cpu_has(X86_FEATURE_PKU);
+}
+
 static inline bool cpu_has_vmx_rdtscp(void)
 {
 	return vmcs_config.cpu_based_2nd_exec_ctrl &
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e3394c839dea..6ba72440b3e3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7870,6 +7870,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 	.xsaves_supported = vmx_xsaves_supported,
 	.umip_emulated = vmx_umip_emulated,
 	.pt_supported = vmx_pt_supported,
+	.pku_supported = vmx_pku_supported,
 
 	.request_immediate_exit = vmx_request_immediate_exit,
 
-- 
2.24.0

Re: [PATCH v2] kvm/svm: PKU not currently supported

[Sean Christopherson]

On Thu, Dec 19, 2019 at 02:17:59PM -0600, John Allen wrote:
> Current SVM implementation does not have support for handling PKU. Guests
> running on a host with future AMD cpus that support the feature will read
> garbage from the PKRU register and will hit segmentation faults on boot as
> memory is getting marked as protected that should not be. Ensure that cpuid
> from SVM does not advertise the feature.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
> v2:
>   -Introduce kvm_x86_ops->pku_supported()

I like the v1 approach better, it's less code to unwind when SVM gains
support for virtualizaing PKU.

The existing cases of kvm_x86_ops->*_supported() in __do_cpuid_func() are
necessary to handle cases where it may not be possible to expose a feature
even though it's supported in hardware, host and KVM, e.g. VMX's separate
MSR-based features and PT's software control to hide it from guest.  In
this case, hiding PKU is purely due to lack of support in KVM.  The SVM
series to enable PKU can then delete a single line of SVM code instead of
having to go back in and do surgery on x86 and VMX.

Re: [PATCH v2] kvm/svm: PKU not currently supported

[Paolo Bonzini]

On 19/12/19 21:32, Sean Christopherson wrote:
> On Thu, Dec 19, 2019 at 02:17:59PM -0600, John Allen wrote:
>> Current SVM implementation does not have support for handling PKU. Guests
>> running on a host with future AMD cpus that support the feature will read
>> garbage from the PKRU register and will hit segmentation faults on boot as
>> memory is getting marked as protected that should not be. Ensure that cpuid
>> from SVM does not advertise the feature.
>>
>> Signed-off-by: John Allen <john.allen@amd.com>
>> ---
>> v2:
>>   -Introduce kvm_x86_ops->pku_supported()
> 
> I like the v1 approach better, it's less code to unwind when SVM gains
> support for virtualizaing PKU.
> 
> The existing cases of kvm_x86_ops->*_supported() in __do_cpuid_func() are
> necessary to handle cases where it may not be possible to expose a feature
> even though it's supported in hardware, host and KVM, e.g. VMX's separate
> MSR-based features and PT's software control to hide it from guest.  In
> this case, hiding PKU is purely due to lack of support in KVM.  The SVM
> series to enable PKU can then delete a single line of SVM code instead of
> having to go back in and do surgery on x86 and VMX.
> 

I sort of liked the V1 approach better, in that I liked using
set_supported_cpuid but I didn't like *removing* features from it.

I think all *_supported() should be removed, and the code moved from
__do_cpuid_func() to set_supported_cpuid.

For now, however, this one is consistent with other features so I am
applying it.

Paolo

Re: [PATCH v2] kvm/svm: PKU not currently supported

[John Allen]

On Fri, Dec 20, 2019 at 10:25:16AM +0100, Paolo Bonzini wrote:
> On 19/12/19 21:32, Sean Christopherson wrote:
> > On Thu, Dec 19, 2019 at 02:17:59PM -0600, John Allen wrote:
> >> Current SVM implementation does not have support for handling PKU. Guests
> >> running on a host with future AMD cpus that support the feature will read
> >> garbage from the PKRU register and will hit segmentation faults on boot as
> >> memory is getting marked as protected that should not be. Ensure that cpuid
> >> from SVM does not advertise the feature.
> >>
> >> Signed-off-by: John Allen <john.allen@amd.com>
> >> ---
> >> v2:
> >>   -Introduce kvm_x86_ops->pku_supported()
> > 
> > I like the v1 approach better, it's less code to unwind when SVM gains
> > support for virtualizaing PKU.
> > 
> > The existing cases of kvm_x86_ops->*_supported() in __do_cpuid_func() are
> > necessary to handle cases where it may not be possible to expose a feature
> > even though it's supported in hardware, host and KVM, e.g. VMX's separate
> > MSR-based features and PT's software control to hide it from guest.  In
> > this case, hiding PKU is purely due to lack of support in KVM.  The SVM
> > series to enable PKU can then delete a single line of SVM code instead of
> > having to go back in and do surgery on x86 and VMX.
> > 
> 
> I sort of liked the V1 approach better, in that I liked using
> set_supported_cpuid but I didn't like *removing* features from it.
> 
> I think all *_supported() should be removed, and the code moved from
> __do_cpuid_func() to set_supported_cpuid.
> 
> For now, however, this one is consistent with other features so I am
> applying it.

Hey Paolo,

If you haven't already applied this, would it be too much trouble to add a
fixes tag? If it's already applied, don't worry about it.

...
Fixes: 0556cbdc2fbc ("x86/pkeys: Don't check if PKRU is zero before writing it")

Thanks,
John

> 
> Paolo
> 

Re: [PATCH v2] kvm/svm: PKU not currently supported

[Paolo Bonzini]

On 23/12/19 16:21, John Allen wrote:
> Hey Paolo,
> 
> If you haven't already applied this, would it be too much trouble to add a
> fixes tag? If it's already applied, don't worry about it.
> 
> ...
> Fixes: 0556cbdc2fbc ("x86/pkeys: Don't check if PKRU is zero before writing it")

Done, thanks.

Paolo