On Mon, Mar 18, 2019 at 09:46:24PM -0400, Krish Sadhukhan wrote:
> 1. According to section "Checks on Host Control Registers and MSRs" in Intel
> SDM vol 3C, the following check is performed on vmentry of L2 guests:
Heh, "of L2 guests" again.
>
> If the “load IA32_PAT” VM-exit control is 1, the value of the field
> for the IA32_PAT MSR must be one that could be written by WRMSR
> without fault at CPL 0. Specifically, each of the 8 bytes in the
> field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> 6 (WB), or 7 (UC-).
>
> 2. According to section "CHECKING AND LOADING GUEST STATE" in Intel
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Why use all caps here?
> SDM vol 3C, the following check is performed on vmentry of L2 guests:
>
> If the "load IA32_PAT" VM-entry control is 1, the value of the field
> for the IA32_PAT MSR must be one that could be written by WRMSR
> without fault at CPL 0. Specifically, each of the 8 bytes in the
> field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> 6 (WB), or 7 (UC-).
IMO this is a bit gratuitous. The paragraphs are identical except for
"exit" vs. "entry". A little consolidation:
According to sections "Checks on Host Control Registers and MSRs" and
"Checking and Loading Guest State" in Intel SDM vol 3c, the following
checks are performed on VM-Entry:
If the "load IA32_PAT" VM-{entry,exit} control is 1, the value of
the field for the IA32_PAT MSR must be one that could be written by
WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in
the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
6 (WB), or 7 (UC-).
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
> ---
> x86/vmx_tests.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 87 insertions(+)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 66a87f6..7dae0f0 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4995,6 +4995,90 @@ static void test_sysenter_field(u32 field, const char *name)
> vmcs_write(field, addr_saved);
> }
>
> +static void test_pat_fields(u64 vmcs_fld, const char * vmcs_fld_name,
> + u64 ctrl, u64 ctrl_fld)
> +{
> + u32 ctrl_saved = vmcs_read(ctrl);
> + u64 vmcs_fld_saved = vmcs_read(vmcs_fld);
> + int i, j, error = 0;
> + u64 val;
> +
> + vmcs_write(ctrl, ctrl_saved & ~ctrl_fld);
> + for (i = 0; i < 256; i++) {
> + /* Test PAT0..PAT7 fields */
> + for (j = 0; j < 8; j++) {
> + val = i << j * 8;
This shifts an 'int' by more than 32-bits and likely breaks a 32-bit build.
> + vmcs_write(vmcs_fld, val);
> + report_prefix_pushf("%s %lx", vmcs_fld_name, val);
> + test_vmx_vmlaunch(error, false);
I'd prefer to just pass '0' for error, it took me a second to find where
error was set.
> + report_prefix_pop();
> + }
> + }
> +
> + vmcs_write(ctrl, ctrl_saved | ctrl_fld);
> + for (i = 0; i < 256; i++) {
> + /* Test PAT0..PAT7 fields */
> + for (j = 0; j < 8; j++) {
This is the third or fourth test we have that iterates over every possible
combination of illegal value, the vast majority of which are redundant to
some degree. E.g. 'i > 8' is basically just 'i==8', and is particularly
uninteresting when checking every value in every byte.
And on the flip side, a shortcoming is that the value of the bytes we're
not explicitly targeting will always be '0'.
I wonder if it makes sense to split these types of exhaustive tests into
two separate tests, one to be run as a standard VMX test, and the other to
be binned into an "exhuastive" test case. That way people can get 99% of
coverage using the standard test without having to wait for thousands of
VM-Entries.
I.e. pare this down to i=0..8 and j=0..8 (64 vs. 2048 entries), and put
the i>8 portion into the more exhaustive test case and make it even more
exhaustive by loading the other fields with non-zero legal values.
> + val = i << j * 8;
Again, an int is being shifted by more than 32-bits.
> + vmcs_write(vmcs_fld, val);
> + report_prefix_pushf("%s %lx", vmcs_fld_name, val);
> + if (i == 0x2 || i == 0x3 || i > 0x7) {
> + if (vmcs_fld == HOST_PAT)
> + error =
> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> + else
> + error =
> + VMXERR_ENTRY_INVALID_CONTROL_FIELD;
Invalid GUEST_PAT is a VM-Exit consistency check.
> + } else {
> + error = 0;
> + }
> + test_vmx_vmlaunch(error, false);
> + report_prefix_pop();
> + }
> + }
> +
> + vmcs_write(ctrl, ctrl_saved);
> + vmcs_write(vmcs_fld, vmcs_fld_saved);
> +}
> +
> +/*
> + * 1. If the "load IA32_PAT" VM-exit control is 1, the value of the field
> + * for the IA32_PAT MSR must be one that could be written by WRMSR
> + * without fault at CPL 0. Specifically, each of the 8 bytes in the
> + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> + * 6 (WB), or 7 (UC-).
> + *
> + * 2. If the "load IA32_PAT" VM-entry control is 1, the value of the field
> + * for the IA32_PAT MSR must be one that could be written by WRMSR
> + * without fault at CPL 0. Specifically, each of the 8 bytes in the
> + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> + * 6 (WB), or 7 (UC-).
> + *
> + * [Intel SDM]
> + */
> +static void test_load_pat(void)
> +{
> + /*
> + * "load IA32_PAT" VM-exit control
> + */
> + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) {
> + printf("\"Load-IA32-PAT\" exit control not supported\n");
> + return;
> + }
> +
> + test_pat_fields(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
> +
> + /*
> + * "load IA32_PAT" VM-entry control
> + */
> + if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT)) {
> + printf("\"Load-IA32-PAT\" entry control not supported\n");
> + return;
> + }
> +
> + test_pat_fields(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
> +}
> +
> /*
> * Check that the virtual CPU checks the VMX Host State Area as
> * documented in the Intel SDM.
> @@ -5010,6 +5094,9 @@ static void vmx_host_state_area_test(void)
>
> test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP");
> test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
> +
> + test_load_pat();
You're testing both guest and host state in vmx_HOST_state_area_test().
> +
> }
>
> static bool valid_vmcs_for_vmentry(void)
> --
> 2.17.2
>
