x86: Fix max VMCS field encoding index check

PATCH Applied Nadav Amit 4 participants 7 messages May 18, 2019 – Dec 19, 2019 KVM

Share & tools

Nadav Amit

author

2019-05-18 23:57 UTC #0 lore raw

PATCH 1/1 x86: Fix max VMCS field encoding index check

The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.

Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.

This test might still fail on bare-metal due to errata (e.g., BDX30).

Cc Sean Christopherson <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 x86/vmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

x86/vmx.c +2 -2

index 962ec0f..f540e15 100644
@@ -361,8 +361,8 @@ static void test_vmwrite_vmread(void)
 	report("VMWRITE/VMREAD", __check_all_vmcs_fields(0x42, &max_index));
 
 	vmcs_enum_max = rdmsr(MSR_IA32_VMX_VMCS_ENUM) & VMCS_FIELD_INDEX_MASK;
-	report("VMX_VMCS_ENUM.MAX_INDEX expected: %x, actual: %x",
-		vmcs_enum_max == max_index, max_index, vmcs_enum_max);
+	report("VMX_VMCS_ENUM.MAX_INDEX expected at least: %x, actual: %x",
+		vmcs_enum_max >= max_index, max_index, vmcs_enum_max);
 
 	assert(!vmcs_clear(vmcs));
 	free_page(vmcs);

Signature · 1 line

2.17.1

Paolo Bonzini

maintainer Applied

2019-05-20 14:51 UTC #1 lore raw

Quoted 22 lines · Nadav Amit

The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.
This test might still fail on bare-metal due to errata (e.g., BDX30).
Cc Sean Christopherson <[email protected]>
Signed-off-by Nadav Amit <[email protected]>
---
 x86/vmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
x86/vmx.c +0 -0
index 962ec0f..f540e15 100644

x86/vmx.c static void test_vmwrite_vmread(void) · line 368

Quoted 10 lines

x86/vmx.c +2 -2

@@ -361,8 +361,8 @@ static void test_vmwrite_vmread(void)
 	report("VMWRITE/VMREAD", __check_all_vmcs_fields(0x42, &max_index));
 
 	vmcs_enum_max = rdmsr(MSR_IA32_VMX_VMCS_ENUM) & VMCS_FIELD_INDEX_MASK;
-	report("VMX_VMCS_ENUM.MAX_INDEX expected: %x, actual: %x",
-		vmcs_enum_max == max_index, max_index, vmcs_enum_max);
+	report("VMX_VMCS_ENUM.MAX_INDEX expected at least: %x, actual: %x",
+		vmcs_enum_max >= max_index, max_index, vmcs_enum_max);
 
 	assert(!vmcs_clear(vmcs));
 	free_page(vmcs);

Queued, thanks.

Paolo

Jim Mattson

2019-12-12 23:00 UTC #2 lore raw

In reply to Nadav Amit

Quoted 8 lines · Nadav Amit

The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.

Wouldn't it be better to probe all possible VMCS fields and keep the test for equality?

Nadav Amit

author

2019-12-13 09:13 UTC #3 lore raw

Quoted 7 lines · Jim Mattson

Quoted 4 lines · Nadav Amit
The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.

Quoted 6 lines

Quoted 3 lines
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.
Wouldn't it be better to probe all possible VMCS fields and keep the test for equality?

It might take a while though…

How about probing VMREAD/VMWRITE to MAX_INDEX in addition to all the known VMCS fields and then checking for equation?

Jim Mattson

2019-12-13 20:39 UTC #4 lore raw

It can't take that long. VMCS field encodings are only 15 bits, and you can ignore the "high" part of 64-bit fields, so that leaves only 14 bits.

Quoted 20 lines · Nadav Amit

Quoted 8 lines · Jim Mattson
Quoted 4 lines · Nadav Amit
The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.
Quoted 6 lines
Quoted 3 lines
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.
Wouldn't it be better to probe all possible VMCS fields and keep the test for equality?
It might take a while though…
How about probing VMREAD/VMWRITE to MAX_INDEX in addition to all the known VMCS fields and then checking for equation?

Sean Christopherson

2019-12-18 00:24 UTC #5 lore raw

Quoted 16 lines · Jim Mattson

Quoted 15 lines · Nadav Amit
Quoted 14 lines · Jim Mattson
Quoted 7 lines · Nadav Amit
The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.
Wouldn't it be better to probe all possible VMCS fields and keep the test for equality?

Quoted 8 lines

Quoted 5 lines
It might take a while though…
How about probing VMREAD/VMWRITE to MAX_INDEX in addition to all the known VMCS fields and then checking for equation?
It can't take that long. VMCS field encodings are only 15 bits, and you can ignore the "high" part of 64-bit fields, so that leaves only 14 bits.

Unless kvm-unit-tests is being run in L1, in which case things like this are painful. That being said, I do agree that probing "all" VMCS fields is the way to go. Walking from highest->lowest probably won't even take all that many VMREADS. If it is slow, the test can be binned to its own config.

Nadav Amit

author

2019-12-19 08:52 UTC #6 lore raw

Quoted 26 lines · Sean Christopherson

Quoted 15 lines · Jim Mattson
Quoted 14 lines · Nadav Amit
Quoted 13 lines · Jim Mattson
Quoted 7 lines · Nadav Amit
The test that checks the maximum VMCS field encoding does not probe all possible VMCS fields. As a result it might fail since the actual IA32_VMX_VMCS_ENUM.MAX_INDEX would be higher than the expected value.
Change the test to check that the maximum of the supported probed VMCS fields is lower/equal than the actual reported IA32_VMX_VMCS_ENUM.MAX_INDEX.
Wouldn't it be better to probe all possible VMCS fields and keep the test for equality?
Quoted 7 lines
Quoted 4 lines
It might take a while though…
How about probing VMREAD/VMWRITE to MAX_INDEX in addition to all the known VMCS fields and then checking for equation?
It can't take that long. VMCS field encodings are only 15 bits, and you can ignore the "high" part of 64-bit fields, so that leaves only 14 bits.

Quoted 5 lines

Unless kvm-unit-tests is being run in L1, in which case things like this are painful. That being said, I do agree that probing "all" VMCS fields is the way to go. Walking from highest->lowest probably won't even take all that many VMREADS. If it is slow, the test can be binned to its own config.

Ok. I’ll send a patch for that.