Skip to contentSkip to conversation

kvm/svm: PKU not currently supported

PATCH In review John Allen 3 participants 5 messages Dec 19, 2019 – Jan 18, 2020

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

John Allen

author
#0 lore raw

PATCH v2 1/1 kvm/svm: PKU not currently supported

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 <[email protected]>

---
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(-)
arch/x86/include/asm/kvm_host.h +1 -0
index b79cd6aa4075..4a9d869465ad 100644
@@ -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);
arch/x86/kvm/cpuid.c +3 -1
index cfafa320a8cf..ace146d663db 100644
@@ -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);
arch/x86/kvm/svm.c +6 -0
index 122d4ce3b1ab..8b0620f3aed6 100644
@@ -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,
arch/x86/kvm/vmx/capabilities.h +5 -0
index 7aa69716d516..283bdb7071af 100644
@@ -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 &
arch/x86/kvm/vmx/vmx.c +1 -0
index e3394c839dea..6ba72440b3e3 100644
@@ -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,
Signature · 1 line
2.24.0
#1 lore raw
Quoted 10 lines · 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 <[email protected]>

---
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.

maintainer
#2 lore raw
Quoted 15 lines · Sean Christopherson
Quoted 10 lines · 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 <[email protected]>

---
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.

Quoted 8 lines

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

author
#3 lore raw
Quoted 25 lines · Paolo Bonzini
Quoted 15 lines · Sean Christopherson
Quoted 10 lines · 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 <[email protected]>

---
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.

Quoted 8 lines

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.

Quoted 8 lines

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

Quoted 3 lines

Paolo

maintainer
#4 lore raw
Quoted 7 lines · John Allen

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