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 ?
+ 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 ?
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.
+}
+
+/*
+ * Tests for VMX exit controls
+ */
+static void test_exit_ctls(void)
+{
+ test_exit_msr_store();
+}
+
+/*
* Check that the virtual CPU checks all of the VMX controls as
* documented in the Intel SDM.
*/
@@ -4756,6 +4825,7 @@ static void vmx_controls_test(void)
test_invalid_event_injection();
test_vpid();
test_eptp();
+ test_exit_ctls();
}
static bool valid_vmcs_for_vmentry(void)
--
2.9.5
