x86: Add control register pinning tests
PATCH 1/1 x86: Add control register pinning tests
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
We check that the bits allowed to be pinned for CR4 are UMIP, SMEP, and SMAP. Only WP should be allowed to be pinned in CR0.
We turn on all of the allowed bits, pin them, then attempt to disable them. We verify that the attempt to disable was unsuccessful, and that it generated a general protection fault.
For nested, we check that for when pinning enabled in L1, changing HOST_CR0/4 will not result in the un-setting of pinned bits. The VMX CR pinning tests is it's own test so that the pinning doesn't potentially affect other tests within the same .flat testing VM.
--- x86/Makefile.common | 3 +- lib/x86/desc.h | 1 + lib/x86/msr.h | 8 ++ lib/x86/processor.h | 1 + lib/x86/desc.c | 8 ++ x86/cr_pin_high.c | 185 ++++++++++++++++++++++++++++++++++++++++++++ x86/cr_pin_low.c | 55 +++++++++++++ x86/pcid.c | 8 -- x86/vmx_tests.c | 138 +++++++++++++++++++++++++++++++++ x86/unittests.cfg | 16 +++- 10 files changed, 413 insertions(+), 10 deletions(-) create mode 100644 x86/cr_pin_high.c create mode 100644 x86/cr_pin_low.c
x86/Makefile.common +2 -1
@@ -58,7 +58,8 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \ $(TEST_DIR)/init.flat $(TEST_DIR)/smap.flat \ $(TEST_DIR)/hyperv_synic.flat $(TEST_DIR)/hyperv_stimer.flat \ $(TEST_DIR)/hyperv_connections.flat \ - $(TEST_DIR)/umip.flat $(TEST_DIR)/tsx-ctrl.flat + $(TEST_DIR)/umip.flat $(TEST_DIR)/tsx-ctrl.flat \ + $(TEST_DIR)/cr_pin_low.flat $(TEST_DIR)/cr_pin_high.flat test_cases: $(tests-common) $(tests)
lib/x86/desc.h +1 -0
@@ -211,6 +211,7 @@ extern tss64_t tss;#endif unsigned exception_vector (void ); +int write_cr0_checking (unsigned long val);int write_cr4_checking (unsigned long val);unsigned exception_error_code (void );bool exception_rflags_rf (void );
lib/x86/msr.h +8 -0
@@ -431,4 +431,12 @@#define MSR_VM_IGNNE 0xc0010115#define MSR_VM_HSAVE_PA 0xc0010117 +/* KVM MSRs */ +#define MSR_KVM_CR0_PIN_ALLOWED 0x4b564d08 +#define MSR_KVM_CR4_PIN_ALLOWED 0x4b564d09 +#define MSR_KVM_CR0_PINNED_LOW 0x4b564d0a +#define MSR_KVM_CR0_PINNED_HIGH 0x4b564d0b +#define MSR_KVM_CR4_PINNED_LOW 0x4b564d0c +#define MSR_KVM_CR4_PINNED_HIGH 0x4b564d0d +#endif /* _ASM_X86_MSR_INDEX_H */
lib/x86/processor.h +1 -0
@@ -146,6 +146,7 @@ static inline u8 cpuid_maxphyaddr(void)#define X86_FEATURE_SMEP (CPUID(0x7, 0, EBX, 7))#define X86_FEATURE_INVPCID (CPUID(0x7, 0, EBX, 10))#define X86_FEATURE_RTM (CPUID(0x7, 0, EBX, 11)) +#define X86_FEATURE_SMEP (CPUID(0x7, 0, EBX, 7))#define X86_FEATURE_SMAP (CPUID(0x7, 0, EBX, 20))#define X86_FEATURE_PCOMMIT (CPUID(0x7, 0, EBX, 22))#define X86_FEATURE_CLFLUSHOPT (CPUID(0x7, 0, EBX, 23))
lib/x86/desc.c +8 -0
@@ -251,6 +251,14 @@ unsigned exception_vector(void)return vector; } +int write_cr0_checking (unsigned long val) +{ + asm volatile(ASM_TRY("1f") + "mov %0, %%cr0\n\t" + "1:": : "r" (val)); +return exception_vector (); +} +int write_cr4_checking (unsigned long val) { asm volatile(ASM_TRY("1f")
x86/cr_pin_high.c new file +185 -0
185 lines not on this page · raw message
x86/cr_pin_low.c new file +55 -0
55 lines not on this page · raw message
x86/pcid.c +0 -8
@@ -10,14 +10,6 @@ struct invpcid_desc { unsigned long addr : 64; }; -static int write_cr0_checking (unsigned long val) -{ - asm volatile(ASM_TRY("1f") - "mov %0, %%cr0\n\t" - "1:": : "r" (val)); -return exception_vector (); -} -static int invpcid_checking (unsigned long type,void *desc) { asm volatile (ASM_TRY("1f")
x86/vmx_tests.c +138 -0
150 lines not on this page · raw message
x86/unittests.cfg +15 -1
@@ -245,9 +245,17 @@ arch = x86_64 file = umip.flat extra_params = -cpu qemu64,+umip +[cr_pin_low] +file = cr_pin_low.flat +extra_params = -cpu qemu64,+umip,+smap,+smep + +[cr_pin_high] +file = cr_pin_high.flat +extra_params = -cpu qemu64,+umip,+smap,+smep + [vmx] file = vmx.flat -extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_test -atomic_switch_overflow_msrs_test -vmx_init_signal_test -vmx_apic_passthrough_tpr_threshold_test" +extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_test -atomic_switch_overflow_msrs_test -vmx_init_signal_test -vmx_apic_passthrough_tpr_threshold_test -vmx_cr_pin_test" arch = x86_64 groups = vmx
@@ -306,6 +314,12 @@ extra_params = -cpu host,+vmx -append vmx_vmcs_shadow_test arch = x86_64 groups = vmx +[vmx_cr_pin_test] +file = vmx.flat +extra_params = -cpu host,+vmx -append vmx_cr_pin_test +arch = x86_64 +groups = vmx + [debug] file = debug.flat arch = x86_64
Signature · 1 line
2.21.0
Quoted 6 lines · John Andersen
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
[ sni[
x86/vmx_tests.c static void vmentry_movss_shadow_test(void) · line 7943
Quoted 6 lines
x86/vmx_tests.c +6
-0+static void vmx_cr_pin_test_guest (void ) +{ +unsigned long i, cr0, cr4; + +/* Step 1. Skip feature detection to skip handling VMX_CPUID */ +/* nop */
I do not quite get this comment. Why do you skip checking whether the feature is enabled? What happens if KVM/bare-metal/other-hypervisor that runs this test does not support this feature?
Quoted 10 lines · Nadav Amit
Quoted 6 lines · John Andersen
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
[ sni[
Quoted 10 lines
x86/vmx_tests.c static void vmentry_movss_shadow_test(void) · line 7943
Quoted 6 lines
x86/vmx_tests.c +6
-0+static void vmx_cr_pin_test_guest (void ) +{ +unsigned long i, cr0, cr4; + +/* Step 1. Skip feature detection to skip handling VMX_CPUID */ +/* nop */ I do not quite get this comment. Why do you skip checking whether the feature is enabled? What happens if KVM/bare-metal/other-hypervisor that runs this test does not support this feature?
My bad, I was confused between the nested checks and the non-nested ones.
Nevertheless, can we avoid situations in which rdmsr(MSR_KVM_CR0_PIN_ALLOWED) causes #GP when the feature is not implemented? Is there some protocol for detection that this feature is supported by the hypervisor, or do we need something like rdmsr_safe()?
Quoted 21 lines · Nadav Amit
Quoted 10 lines · Nadav Amit
Quoted 6 lines · John Andersen
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
[ sni[
Quoted 10 lines
x86/vmx_tests.c static void vmentry_movss_shadow_test(void) · line 7943
Quoted 6 lines
x86/vmx_tests.c +6
-0+static void vmx_cr_pin_test_guest (void ) +{ +unsigned long i, cr0, cr4; + +/* Step 1. Skip feature detection to skip handling VMX_CPUID */ +/* nop */ I do not quite get this comment. Why do you skip checking whether the feature is enabled? What happens if KVM/bare-metal/other-hypervisor that runs this test does not support this feature?
Quoted 7 lines
My bad, I was confused between the nested checks and the non-nested ones.
Nevertheless, can we avoid situations in which
rdmsr(MSR_KVM_CR0_PIN_ALLOWED)causes #GP when the feature is not implemented? Is there some protocol for detection that this feature is supported by the hypervisor, or do we need something likerdmsr_safe()?
Ah, yes we can. By checking the CPUID for the feature bit. Thanks for pointing this out, I was confused about this. I was operating under the assumption that the unit tests assume the features in the latest kvm/next are present and available when the unit tests are being run.
I'm happy to add the check, but I haven't see anywhere else where a KVM_FEATURE_ was checked for. Which is why it doesn't check in this patch. As soon as I get an answer from you or anyone else as to if the unit tests assume that the features in the latest kvm/next are present and available or not when the unit tests are being run I'll modify if necessary.
Thanks,
John
Quoted 36 lines · John
Quoted 21 lines · Nadav Amit
Quoted 10 lines · Nadav Amit
Quoted 6 lines · John Andersen
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
[ sni[
Quoted 10 lines
x86/vmx_tests.c static void vmentry_movss_shadow_test(void) · line 7943
Quoted 6 lines
x86/vmx_tests.c +6
-0+static void vmx_cr_pin_test_guest (void ) +{ +unsigned long i, cr0, cr4; + +/* Step 1. Skip feature detection to skip handling VMX_CPUID */ +/* nop */ I do not quite get this comment. Why do you skip checking whether the feature is enabled? What happens if KVM/bare-metal/other-hypervisor that runs this test does not support this feature?
Quoted 6 lines
My bad, I was confused between the nested checks and the non-nested ones.
Nevertheless, can we avoid situations in which
rdmsr(MSR_KVM_CR0_PIN_ALLOWED)causes #GP when the feature is not implemented? Is there some protocol for detection that this feature is supported by the hypervisor, or do we need something likerdmsr_safe()?Ah, yes we can. By checking the CPUID for the feature bit. Thanks for pointing this out, I was confused about this. I was operating under the assumption that the unit tests assume the features in the latest kvm/next are present and available when the unit tests are being run.
Quoted 5 lines
I'm happy to add the check, but I haven't see anywhere else where a
KVM_FEATURE_was checked for. Which is why it doesn't check in this patch. As soon as I get an answer from you or anyone else as to if the unit tests assume that the features in the latest kvm/next are present and available or not when the unit tests are being run I'll modify if necessary.
I would appreciate if you add a check of CPUID and not run the test if the feature is not supported.
I run the tests on bare-metal (and other non-KVM environment) from time to time. Doing so allows to find bugs in tests due to wrong assumptions of KVM test developers. Liran runs the tests using QEMU/WHPX (non-KVM). So allowing the tests to run on non-KVM environments is important, at least for some of us, and benefits KVM as well.
While I can disable this specific test using the test parameters, I prefer that the test will first check the environment they run on. Debugging test failures on bare-metal is hard enough without the paravirt stuff noise.
Quoted 51 lines · Nadav Amit
Quoted 36 lines · John
Quoted 21 lines · Nadav Amit
Quoted 10 lines · Nadav Amit
Quoted 6 lines · John Andersen
Paravirutalized control register pinning adds MSRs guests can use to discover which bits in CR0/4 they may pin, and MSRs for activating pinning for any of those bits.
[ sni[
Quoted 10 lines
x86/vmx_tests.c static void vmentry_movss_shadow_test(void) · line 7943
Quoted 6 lines
x86/vmx_tests.c +6
-0+static void vmx_cr_pin_test_guest (void ) +{ +unsigned long i, cr0, cr4; + +/* Step 1. Skip feature detection to skip handling VMX_CPUID */ +/* nop */ I do not quite get this comment. Why do you skip checking whether the feature is enabled? What happens if KVM/bare-metal/other-hypervisor that runs this test does not support this feature?
Quoted 6 lines
My bad, I was confused between the nested checks and the non-nested ones.
Nevertheless, can we avoid situations in which
rdmsr(MSR_KVM_CR0_PIN_ALLOWED)causes #GP when the feature is not implemented? Is there some protocol for detection that this feature is supported by the hypervisor, or do we need something likerdmsr_safe()?Ah, yes we can. By checking the CPUID for the feature bit. Thanks for pointing this out, I was confused about this. I was operating under the assumption that the unit tests assume the features in the latest kvm/next are present and available when the unit tests are being run.
Quoted 5 lines
I'm happy to add the check, but I haven't see anywhere else where a
KVM_FEATURE_was checked for. Which is why it doesn't check in this patch. As soon as I get an answer from you or anyone else as to if the unit tests assume that the features in the latest kvm/next are present and available or not when the unit tests are being run I'll modify if necessary.I would appreciate if you add a check of CPUID and not run the test if the feature is not supported.
I run the tests on bare-metal (and other non-KVM environment) from time to time. Doing so allows to find bugs in tests due to wrong assumptions of KVM test developers. Liran runs the tests using QEMU/WHPX (non-KVM). So allowing the tests to run on non-KVM environments is important, at least for some of us, and benefits KVM as well.
Quoted 4 lines
While I can disable this specific test using the test parameters, I prefer that the test will first check the environment they run on. Debugging test failures on bare-metal is hard enough without the paravirt stuff noise.
Great point! I'll add the check