x86: nvmx: fix bug in __enter_guest()

PATCH In review Marc Orr 5 participants 14 messages Sep 25, 2019 – Oct 1, 2019

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Marc Orr

author

2019-09-25 01:18 UTC #0 lore raw

PATCH v7 1/2 x86: nvmx: fix bug in __enter_guest()

__enter_guest() should only set the launched flag when a launch has succeeded. Thus, don't set the launched flag when the VMX_ENTRY_FAILURE, bit 31, is set in the VMCS exit reason.

Reviewed-by Krish Sadhukhan <[email protected]>

Reviewed-by Sean Christopherson <[email protected]>

Signed-off-by Marc Orr <[email protected]>

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

x86/vmx.c +1 -1

index 6079420db33a..7313c78f15c2 100644
@@ -1820,7 +1820,7 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
 		abort();
 	}
 
-	if (!failure->early) {
+	if (!failure->early && !(vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
 		launched = 1;
 		check_for_guest_termination();
 	}

Signature · 1 line

2.23.0.444.g18eeb5a265-goog

Marc Orr

author

2019-09-25 01:18 UTC #1 lore raw

PATCH v7 2/2 x86: nvmx: test max atomic switch MSRs

Excercise nested VMX's atomic MSR switch code (e.g., VM-entry MSR-load list) at the maximum number of MSRs supported, as described in the SDM, in the appendix chapter titled "MISCELLANEOUS DATA".

Reviewed-by Krish Sadhukhan <[email protected]>

Signed-off-by Marc Orr <[email protected]>

---
 lib/alloc_page.c  |   5 ++
 lib/alloc_page.h  |   1 +
 x86/unittests.cfg |   2 +-
 x86/vmx_tests.c   | 124 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 131 insertions(+), 1 deletion(-)

lib/alloc_page.c +5 -0

index 97d13395ff08..ed236389537e 100644
@@ -53,6 +53,11 @@ void free_pages(void *mem, unsigned long size)
 	spin_unlock(&lock);
 }
 
+void free_pages_by_order(void *mem, unsigned long order)
+{
+	free_pages(mem, 1ul << (order + PAGE_SHIFT));
+}
+
 void *alloc_page()
 {
 	void *p;

lib/alloc_page.h +1 -0

index 5cdfec57a0a8..739a91def979 100644
@@ -14,5 +14,6 @@ void *alloc_page(void);
 void *alloc_pages(unsigned long order);
 void free_page(void *page);
 void free_pages(void *mem, unsigned long size);
+void free_pages_by_order(void *mem, unsigned long order);
 
 #endif

x86/unittests.cfg +1 -1

index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 
 [vmx]
 file = vmx.flat
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
 arch = x86_64
 groups = vmx

x86/vmx_tests.c +124 -0

135 lines not on this page · raw message

Signature · 1 line

2.23.0.444.g18eeb5a265-goog

Sean Christopherson

Reviewed-by on v7 2/2

2019-09-25 19:13 UTC #2 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 8 lines · Marc Orr

Excercise nested VMX's atomic MSR switch code (e.g., VM-entry MSR-load list) at the maximum number of MSRs supported, as described in the SDM, in the appendix chapter titled "MISCELLANEOUS DATA".
Suggested-by Jim Mattson <[email protected]>
Reviewed-by Krish Sadhukhan <[email protected]>
Signed-off-by Marc Orr <[email protected]>
---

Thanks for being patient and fixing all the nits!

Reviewed-by Sean Christopherson <[email protected]>

Paolo Bonzini

maintainer

2019-09-26 09:25 UTC #3 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

In reply to Marc Orr

x86/unittests.cfg extra_params = -cpu qemu64,+umip · line 232

Quoted 12 lines · Marc Orr

x86/unittests.cfg +1 -1

index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 
 [vmx]
 file = vmx.flat
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
 arch = x86_64
 groups = vmx

I just noticed this, why is the test disabled by default?

Paolo

Sean Christopherson

2019-09-26 14:32 UTC #4 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines · Marc Orr
x86/unittests.cfg +0 -0
index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 

Quoted 8 lines

x86/unittests.cfg extra_params = -cpu qemu64,+umip · line 232
Quoted 6 lines
x86/unittests.cfg +1 -1
 [vmx]
 file = vmx.flat
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
 arch = x86_64
 groups = vmx
I just noticed this, why is the test disabled by default?

The negative test triggers undefined behavior, e.g. on bare metal the test would fail because VM-Enter would succeed due to lack of an explicit check on the MSR count.

Since the test relies on somehwat arbitrary KVM behavior, we made it opt-in.

Liran Alon

2019-09-26 14:37 UTC #5 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 21 lines · Sean Christopherson

Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · Marc Orr
x86/unittests.cfg +0 -0
index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 
Quoted 8 lines
Quoted 6 lines
[vmx]
file = vmx.flat
x86/unittests.cfg +1 -1
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
arch = x86_64
groups = vmx
I just noticed this, why is the test disabled by default?

Quoted 5 lines

The negative test triggers undefined behavior, e.g. on bare metal the test would fail because VM-Enter would succeed due to lack of an explicit check on the MSR count.
Since the test relies on somehwat arbitrary KVM behavior, we made it opt-in.

Just note that when commit 5ac120c23753 ("x86: vmx: Test INIT processing during various CPU VMX states”) was merged to master, it was changed to accidentally remove “-atomic_switch_overflow_msrs_test”. (Probably a merge mistake).

So this should be fixed by a new patch :P

-Liran

Paolo Bonzini

maintainer

2019-09-26 14:49 UTC #6 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 25 lines · Liran Alon

Quoted 25 lines · Sean Christopherson
Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · Marc Orr
x86/unittests.cfg +0 -0
index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 
Quoted 8 lines
Quoted 6 lines
[vmx]
file = vmx.flat
x86/unittests.cfg +1 -1
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
arch = x86_64
groups = vmx
I just noticed this, why is the test disabled by default?
The negative test triggers undefined behavior, e.g. on bare metal the test would fail because VM-Enter would succeed due to lack of an explicit check on the MSR count.

Quoted 7 lines

Quoted 1 line
Since the test relies on somehwat arbitrary KVM behavior, we made it opt-in.
Just note that when commit 5ac120c23753 ("x86: vmx: Test INIT processing during various CPU VMX states”) was merged to master, it was changed to accidentally remove “-atomic_switch_overflow_msrs_test”. (Probably a merge mistake).
So this should be fixed by a new patch :P

I will just rewrite history and fix the mistake.

Paolo

Nadav Amit

2019-09-30 23:25 UTC #7 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

In reply to Sean Christopherson

Quoted 19 lines · Sean Christopherson

Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · Marc Orr
x86/unittests.cfg +0 -0
index 694ee3d42f3a..05122cf91ea1 100644
@@ -227,7 +227,7 @@ extra_params = -cpu qemu64,+umip
 
Quoted 8 lines
Quoted 6 lines
[vmx]
file = vmx.flat
x86/unittests.cfg +1 -1
-extra_params = -cpu host,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_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"
arch = x86_64
groups = vmx
I just noticed this, why is the test disabled by default?

Quoted 5 lines

The negative test triggers undefined behavior, e.g. on bare metal the test would fail because VM-Enter would succeed due to lack of an explicit check on the MSR count.
Since the test relies on somehwat arbitrary KVM behavior, we made it opt-in.

Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...

Besides, it seems that v6 was used and not v7, so the error messages are strange:

Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures

I also think that printing the exit-reason in hex format would be more readable.

Marc Orr

author

2019-09-30 23:34 UTC #8 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 7 lines

Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:

Quoted 7 lines

Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures
I also think that printing the exit-reason in hex format would be more readable.

Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).

To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?

Nadav Amit

2019-09-30 23:58 UTC #9 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 14 lines · Marc Orr

Quoted 13 lines
Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:
Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures

Quoted 5 lines

Quoted 2 lines
I also think that printing the exit-reason in hex format would be more readable.
Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).

I know, but when the failed VM entry indication is on, it is just a huge mess. Never mind, this is a minor issue.

Quoted 3 lines

To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?

I ran it on bare-metal and needed to spend some time to realize that it is expected to fail on bare-metal “by design”.

Marc Orr

author

2019-10-01 00:03 UTC #10 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 21 lines

Quoted 12 lines
Quoted 12 lines
Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:
Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures
Quoted 5 lines
Quoted 2 lines
I also think that printing the exit-reason in hex format would be more readable.
Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).
I know, but when the failed VM entry indication is on, it is just a huge mess. Never mind, this is a minor issue.

Quoted 6 lines

Quoted 3 lines
To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?
I ran it on bare-metal and needed to spend some time to realize that it is expected to fail on bare-metal “by design”.

Ack. Maybe we should move tests like this into a *_virt_only.c counter-part? E.g., we could create a new, opt-in, file, vmx_tests_virt_only.c for this test. When similar scenarios arise in the future, this new precedent could be replicated, to make it obvious which tests are expected to fail on bare metal.

Nadav Amit

2019-10-01 00:08 UTC #11 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 27 lines · Marc Orr

Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:
Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures
Quoted 5 lines
Quoted 2 lines
I also think that printing the exit-reason in hex format would be more readable.
Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).
I know, but when the failed VM entry indication is on, it is just a huge mess. Never mind, this is a minor issue.
Quoted 3 lines
To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?

Quoted 8 lines

Quoted 2 lines
I ran it on bare-metal and needed to spend some time to realize that it is expected to fail on bare-metal “by design”.
Ack. Maybe we should move tests like this into a *_virt_only.c counter-part? E.g., we could create a new, opt-in, file, vmx_tests_virt_only.c for this test. When similar scenarios arise in the future, this new precedent could be replicated, to make it obvious which tests are expected to fail on bare metal.

Thanks for the willingness, but I don’t know whether any intrusive change is needed at the moment. Even just getting the print-out to have something like “(KVM-specific)” comment would be enough, assuming the test can easily be disabled (as is the case with atomic_switch_overflow_msrs_test).

Marc Orr

author

2019-10-01 00:12 UTC #12 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 34 lines

Quoted 25 lines
Quoted 25 lines
Quoted 12 lines
Quoted 12 lines
Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:
Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures
Quoted 5 lines
Quoted 2 lines
I also think that printing the exit-reason in hex format would be more readable.
Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).
I know, but when the failed VM entry indication is on, it is just a huge mess. Never mind, this is a minor issue.
Quoted 3 lines
To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?
Quoted 8 lines
Quoted 2 lines
I ran it on bare-metal and needed to spend some time to realize that it is expected to fail on bare-metal “by design”.
Ack. Maybe we should move tests like this into a *_virt_only.c counter-part? E.g., we could create a new, opt-in, file, vmx_tests_virt_only.c for this test. When similar scenarios arise in the future, this new precedent could be replicated, to make it obvious which tests are expected to fail on bare metal.

Quoted 4 lines

Thanks for the willingness, but I don’t know whether any intrusive change is needed at the moment. Even just getting the print-out to have something like “(KVM-specific)” comment would be enough, assuming the test can easily be disabled (as is the case with atomic_switch_overflow_msrs_test).

"(as is the case with atomic_switch_overflow_msrs_test)" --> now I'm confused. Are you saying that the first test, atomic_switch_max_msrs_test() failed on bare metal? That test is expected to pass on bare metal. However, I did not test it on bare metal.

Nadav Amit

2019-10-01 00:37 UTC #13 lore raw

Re: [kvm-unit-tests PATCH v7 2/2] x86: nvmx: test max atomic switch MSRs

Quoted 41 lines · Marc Orr

Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
Thanks for caring, but it would be better to explicitly skip the test if it is not running on bare-metal. For instance, I missed this thread and needed to check why the test fails on bare-metal...
Besides, it seems that v6 was used and not v7, so the error messages are strange:
Test suite: atomic_switch_overflow_msrs_test
FAIL: exit_reason, 18, is 2147483682.
FAIL: exit_qual, 0, is 513.
SUMMARY: 11 tests, 2 unexpected failures
Quoted 5 lines
Quoted 2 lines
I also think that printing the exit-reason in hex format would be more readable.
Exit reasons are enumerated in decimal rather than hex in the SDM (volume 3, appendix C).
I know, but when the failed VM entry indication is on, it is just a huge mess. Never mind, this is a minor issue.
Quoted 3 lines
To be clear, are you saying you "opted in" to the test on bare metal, and got confused when it failed? Or, are you saying that our patch on unittest.cfg to make the test not run by default didn't work?
Quoted 8 lines
Quoted 2 lines
I ran it on bare-metal and needed to spend some time to realize that it is expected to fail on bare-metal “by design”.
Ack. Maybe we should move tests like this into a *_virt_only.c counter-part? E.g., we could create a new, opt-in, file, vmx_tests_virt_only.c for this test. When similar scenarios arise in the future, this new precedent could be replicated, to make it obvious which tests are expected to fail on bare metal.
Quoted 4 lines
Thanks for the willingness, but I don’t know whether any intrusive change is needed at the moment. Even just getting the print-out to have something like “(KVM-specific)” comment would be enough, assuming the test can easily be disabled (as is the case with atomic_switch_overflow_msrs_test).

Quoted 5 lines

"(as is the case with atomic_switch_overflow_msrs_test)" --> now I'm confused. Are you saying that the first test, atomic_switch_max_msrs_test() failed on bare metal? That test is expected to pass on bare metal. However, I did not test it on bare metal.

Sorry, my bad:

atomic_switch_max_msrs_test() passes on bare-metal. atomic_switch_overflow_msrs_test() fails.

Everything is as (I understand is) expected to be. I just copy-pasted the wrong test name in my last reply.