On 12/07/2018 09:57 AM, Jim Mattson wrote:
On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
On 12/06/2018 03:02 PM, Jim Mattson wrote:
On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
following checks performed for the VM-exit MSR-store address if the
the VM-exit MSR-store count field is non-zero:
- The lower 4 bits of the VM-exit MSR-store address must be 0.
The address should not set any bits beyond the processor’s
physical-address width.
- The address of the last byte in the VM-exit MSR-store area
should not set any bits beyond the processor’s physical-address
width. The address of this last byte is VM-exit MSR-store address
+ (MSR count * 16) - 1. (The arithmetic used for the computation
uses more bits than the processor’s physical-address width.)
If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
in the range 63:32.
I think there are several existing tests that have ignored this
footnote. Perhaps you could fix this omission in a general way in a
separate patch?
Will do.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx>
---
x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 900dcf4..af856cf 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4731,6 +4731,75 @@ static void test_pml(void)
}
/*
+ * The following checks are performed for the VM-exit MSR-store address if
+ * the VM-exit MSR-store count field is non-zero:
+ *
+ * - The lower 4 bits of the VM-exit MSR-store address must be 0.
+ * The address should not set any bits beyond the processor’s
+ * physical-address width.
+ *
+ * - The address of the last byte in the VM-exit MSR-store area
+ * should not set any bits beyond the processor’s physical-address
+ * width. The address of this last byte is VM-exit MSR-store address
+ * + (MSR count * 16) - 1. (The arithmetic used for the computation
+ * uses more bits than the processor’s physical-address width.)
+ *
+ * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
+ * in the range 63:32.
+ *
+ * [Intel SDM]
+ */
+static void test_exit_msr_store(void)
+{
+ exit_msr_store = alloc_page();
+ u64 tmp;
+ u32 exit_msr_st_cnt = 1;
+ int i;
+ bool ctrl = true;
+ u32 addr_len = 64;
+
+ vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
+
+ /* Check first 4 bits of VM-exit MSR-store address */
+ for (i = 0; i < 4; i++) {
+ tmp = (u64)exit_msr_store | 1ull << i;
+ vmcs_write(EXIT_MSR_ST_ADDR, tmp);
+ report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
+ tmp & 0xf);
+ if (i && ctrl == true) {
+ ctrl = false;
+ }
+ test_vmx_controls(false, false);
+ report_prefix_pop();
+ }
+
+ if (basic.val & (1ul << 48))
+ addr_len = 32;
+
+ test_vmcs_page_values("VM-exit-MSR-store address",
+ EXIT_MSR_ST_ADDR, false, false, 16,
+ 4, addr_len - 1);
+
+ /* Check last byte of VM-exit MSR-store address */
+ for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {
Why the different initializers? Even when IA32_VMX_BASIC[48] is set,
the *end* of the MSR store buffer doesn't have to fit in 32 bits.
I am not sure I follow it...
According to the footnote, setting any bit in [63:32] in the address of
the last byte is illegal if IA32_VMX_BASIC[48] is set and according to
appendix A-1 in SDM vol 3D:
"If the bit is 0, these addresses are limited to the processor’s
physical-address width. If the bit is 1, these addresses are limited to
32 bits. This bit is always 0 for processors that support Intel 64
architecture."
Therefore, on x86_64, the addresses can set any bits in [40:0] but on
x86_32, they are limited to [31:0].
Am I missing something ?
My bad. Re-reading the SDM, the 32-bit constraint clearly applies to
both ends of the buffer.
+ exit_msr_store = (struct vmx_msr_entry *)
+ (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
+ 1ul << i);
+
+ vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
+ test_vmx_controls(false, false);
+ }
I could just be confused, but I don't think this is testing what you
think it's testing.
I have verified that the following check in nested_vmx_check_msr_switch(),
(addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr)
passes when a bit in [63:40] is set in the address of the last byte of
the MSR-store address.
Am I missing anything ?
If the initial exit_msr_store address is 0, then you are testing the
following addresses when addr_len ==32:
exit_msr_store is allocated and is not 0 in this part of the test. For
example, on my x86_64 system, I see the following addresses being tested:
0x1000047900f
0x3000047901e
0x7000047902d
0xf000047903c
0x1f000047904b
0x3f000047905a
0x7f0000479069
0xff0000479078
0x1ff0000479087
0x3ff0000479096
0x7ff00004790a5
0xfff00004790b4
0x1fff00004790c3
0x3fff00004790d2
0x7fff00004790e1
0xffff00004790f0
0x1ffff00004790ff
0x3ffff000047910e
0x7ffff000047911d
0xfffff000047912c
0x1fffff000047913b
0x3fffff000047914a
0x7fffff0000479159
0xffffff0000479168
Here, my goal is to use a real address (returned by alloc_page()) for
the starting byte of exit_msr_store and then make the address of the
last byte invalid by setting a bit in [63:40] in the latter.
10000000f
30000001e
70000002d
f0000003c
1f0000004b
3f0000005a
7f00000069
ff00000078
1ff00000087
3ff00000096
7ff000000a5
fff000000b4
1fff000000c3
3fff000000d2
7fff000000e1
ffff000000f0
1ffff000000ff
3ffff0000010e
7ffff0000011d
fffff0000012c
1fffff0000013b
3fffff0000014a
7fffff00000159
ffffff00000168
1ffffff00000177
3ffffff00000186
7ffffff00000195
fffffff000001a4
1fffffff000001b3
3fffffff000001c2
7fffffff000001d1
ffffffff000001e0
These addresses seem strange to me.
If addr_len == 64 and cpuid_maxphyaddr() is 46, you are testing:
40000000000f
c0000000001e
1c0000000002d
3c0000000003c
7c0000000004b
fc0000000005a
1fc00000000069
3fc00000000078
7fc00000000087
ffc00000000096
1ffc000000000a5
3ffc000000000b4
7ffc000000000c3
fffc000000000d2
1fffc000000000e1
3fffc000000000f0
7fffc000000000ff
ffffc0000000010e
These addresses also seem strange to me.
The constraints are that the entire msr_store buffer must reside
within the bounds of physically addressable memory. Thus, with
EXI_MSR_ST_CNT set to 1, it might be interesting to test
EXIT_MSR_ST_ADDR values of, say:
(1ULL << cpuid_maxphyaddr()) - 15 [FAIL]
(1ULL << cpuid_maxphyaddr()) - 16 [PASS]
(1ULL << cpuid_maxphyaddr()) - 17 [PASS]
I can certainly add these cases. BTW, the last one (17) on your list is
not a PASS case because it sets the low 4 bits in the MSR-store address
itself (and not address of the last byte) and that's not legal.
Okay. Given the multiple constraints, I'd suggest setting the store
count to 2 and testing:
(1ULL << cpuid_maxphyaddr()) - 48
(1ULL << cpuid_maxphyaddr()) - 32
(1ULL << cpuid_maxphyaddr()) - 16
Will add these cases.
