nVMX: Two PCIDE related fixes

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 x86/vmx_tests.c | 5 +++++
 1 file changed, 5 insertions(+)

x86/vmx_tests.c +5 -1

index 29f3d0e..cb42a2d 100644
@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
                vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);
                vmcs_write(HOST_RIP, rip_saved);
                vmcs_write(HOST_CR4, cr4_saved);
+
+               /* Restore host's active RIP and CR4 values. */
+               report_prefix_pushf("restore host state");
+               test_vmx_vmlaunch(0);
+               report_prefix_pop();
        }
 }
 
--

2.26.0

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
  x86/vmx_tests.c | 5 +++++
  1 file changed, 5 insertions(+)

x86/vmx_tests.c +0 -0

index 29f3d0e..cb42a2d 100644
@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
  		vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);

Quoted 25 lines · Sean Christopherson

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 x86/vmx_tests.c | 5 +++++
 1 file changed, 5 insertions(+)

x86/vmx_tests.c +3 -0

index 29f3d0e..cb42a2d 100644
@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
 		vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);
 		vmcs_write(HOST_RIP, rip_saved);
 		vmcs_write(HOST_CR4, cr4_saved);
+
+		/* Restore host's active RIP and CR4 values. */
+		report_prefix_pushf("restore host state");

x86/vmx_tests.c static void test_host_addr_size(void) · line 7682

Quoted 4 lines

x86/vmx_tests.c +2 -0

+		test_vmx_vmlaunch(0);
+		report_prefix_pop();
 	}
 }

Just for my understanding.  When you say, "other misguided tests", which tests are you referring to ?  In the current sequence of tests in vmx_host_state_area_test(), test_load_host_perf_global_ctrl() is the  one that follows and it runs fine.

Quoted 26 lines · Krish Sadhukhan

Quoted 25 lines · Sean Christopherson

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
  x86/vmx_tests.c | 5 +++++
  1 file changed, 5 insertions(+)

x86/vmx_tests.c +3 -0

index 29f3d0e..cb42a2d 100644
@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
  		vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);
  		vmcs_write(HOST_RIP, rip_saved);
  		vmcs_write(HOST_CR4, cr4_saved);
+
+		/* Restore host's active RIP and CR4 values. */
+		report_prefix_pushf("restore host state");

Quoted 8 lines

x86/vmx_tests.c static void test_host_addr_size(void) · line 7682

Quoted 4 lines

x86/vmx_tests.c +2 -0

+		test_vmx_vmlaunch(0);
+		report_prefix_pop();
  	}
  }

Just for my understanding.  When you say, "other misguided tests", which tests are you referring to ?  In the current sequence of tests in vmx_host_state_area_test(), test_load_host_perf_global_ctrl() is the  one that follows and it runs fine.

See test_mtf_guest() in patch 2/2. https://patchwork.kernel.org/patch/11661189/

Quoted 35 lines · Sean Christopherson

Quoted 26 lines · Krish Sadhukhan

Quoted 25 lines · Sean Christopherson

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 x86/vmx_tests.c | 5 +++++
 1 file changed, 5 insertions(+)

x86/vmx_tests.c +3 -0

index 29f3d0e..cb42a2d 100644
@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
 		vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);
 		vmcs_write(HOST_RIP, rip_saved);
 		vmcs_write(HOST_CR4, cr4_saved);
+
+		/* Restore host's active RIP and CR4 values. */
+		report_prefix_pushf("restore host state");

Quoted 8 lines

x86/vmx_tests.c static void test_host_addr_size(void) · line 7682

Quoted 4 lines

x86/vmx_tests.c +2 -0

+		test_vmx_vmlaunch(0);
+		report_prefix_pop();
 	}
 }

Just for my understanding.  When you say, "other misguided tests", which tests are you referring to ?  In the current sequence of tests in vmx_host_state_area_test(), test_load_host_perf_global_ctrl() is the  one that follows and it runs fine.

Quoted 1 line

See test_mtf_guest() in patch 2/2. https://patchwork.kernel.org/patch/11661189/

I ran the two tests as follows but couldn't reproduce it:

    ./x86/run x86/vmx.flat  -smp 1 -cpu host,+vmx -append "vmx_host_state_area_test vmx_mtf_test"

How did you run the them ?

Quoted 36 lines · Krish Sadhukhan

Quoted 35 lines · Sean Christopherson

Quoted 34 lines · Krish Sadhukhan

Quoted 19 lines · Sean Christopherson

Perform one last VMX transition to actually load the host's RIP and CR4 at the end of test_host_addr_size(). Simply writing the VMCS doesn't restore the values in hardware, e.g. as is, CR4.PCIDE can be left set, which causes spectacularly confusing explosions when other misguided tests assume setting bit 63 in CR3 will cause a non-canonical #GP.

Fixes 0786c0316ac05 ("kvm-unit-test: nVMX: Check Host Address Space Size on vmentry of nested guests")

Cc Krish Sadhukhan <[email protected]>

Cc Karl Heubaum <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
  x86/vmx_tests.c | 5 +++++
  1 file changed, 5 insertions(+)

x86/vmx_tests.c +0 -0

index 29f3d0e..cb42a2d 100644

x86/vmx_tests.c static void test_host_addr_size(void) · line 7682

Quoted 10 lines

x86/vmx_tests.c +5 -0

@@ -7673,6 +7673,11 @@ static void test_host_addr_size(void)
  		vmcs_write(ENT_CONTROLS, entry_ctrl_saved | ENT_GUEST_64);
  		vmcs_write(HOST_RIP, rip_saved);
  		vmcs_write(HOST_CR4, cr4_saved);
+
+		/* Restore host's active RIP and CR4 values. */
+		report_prefix_pushf("restore host state");
+		test_vmx_vmlaunch(0);
+		report_prefix_pop();
  	}
  }

Just for my understanding.  When you say, "other misguided tests", which tests are you referring to ?  In the current sequence of tests in vmx_host_state_area_test(), test_load_host_perf_global_ctrl() is the  one that follows and it runs fine.

Quoted 8 lines

Quoted 1 line

See test_mtf_guest() in patch 2/2. https://patchwork.kernel.org/patch/11661189/

I ran the two tests as follows but couldn't reproduce it:

    ./x86/run x86/vmx.flat  -smp 1 -cpu host,+vmx -append
"vmx_host_state_area_test vmx_mtf_test"

How did you run the them ?

I ran the VMX testcase from x86/unittest.cfg (below) on HSW. I eventually narrowed it down to just test_host_addr_size() and the MTF test. Note, the failure signature will change depending on whether vmx_cr_load_test() is run between those two. If it's not run, the failure is a straightforward triple fault. If it is run, for me the failure morphed into a an emulation error because the unit test was able to generate a valid translation out of CR3=0 and hit a non-existent memslot, which was all kinds of confusing.

./x86/run x86/vmx.flat -smp 1 -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"

Use the standard non-canonical value of repeating 'a' instead of a custom (1 << 63) value in test_mtf3. When PCID is enabled, bit 63 is a flag that controls TLB swithching on MOV CR3 and is not included in the canonical check of CR3, i.e. if CR4.PCIDE=1 then the test will load 0 into CR3 and all manner of confusion things happen.

Fixes 46cc038c6afb8 ("x86: VMX: Add tests for monitor trap flag")

Cc Oliver Upton <[email protected]>

Cc Jim Mattson <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 x86/vmx_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/vmx_tests.c +1 -2

index cb42a2d..32e3d4f 100644
@@ -5107,7 +5107,7 @@ static void test_mtf_guest(void)
              * MOV RAX is done before the VMCALL such that MTF is only enabled
              * for the instruction under test.
              */
-            "mov $0x8000000000000000, %rax;\n\t"
+            "mov $0xaaaaaaaaaaaaaaaa, %rax;\n\t"
             "vmcall;\n\t"
             "mov %rax, %cr3;\n\t"
             "test_mtf3:\n\t"
--

2.26.0

PCIDE fixes for two completely unrelated tests that managed to combine powers and create a super confusing error where the MTF test loads CR3 with 0 and sends things into the weeds.

Sean Christopherson (2):
nVMX: Restore active host RIP/CR4 after test_host_addr_size()
nVMX: Use the standard non-canonical value in test_mtf3

  x86/vmx_tests.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

Quoted 4 lines · Sean Christopherson

PCIDE fixes for two completely unrelated tests that managed to combine powers and create a super confusing error where the MTF test loads CR3 with 0 and sends things into the weeds.

Quoted 7 lines

Sean Christopherson (2):
   nVMX: Restore active host RIP/CR4 after test_host_addr_size()
   nVMX: Use the standard non-canonical value in test_mtf3

  x86/vmx_tests.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by Krish Sadhukhan <[email protected]>