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?
> 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.
> + 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.
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]
> +}
> +
> +/*
> + * 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
>
