Re: [PATCH 4/4 v2][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of nested guests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 27, 2019 at 05:56:50PM -0400, Krish Sadhukhan wrote:
> .to verify KVM performs the appropriate consistency checks for loading
> IA32_PAT as part of running a nested guest.
> 
> According to section "Checks on Host Control Registers and MSRs" in Intel
> SDM vol 3C, the following check is performed on vmentry:
> 
>     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-).
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
> ---
>  x86/vmx_tests.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 66a87f6..7ebea74 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4995,6 +4995,69 @@ static void test_sysenter_field(u32 field, const char *name)
>  	vmcs_write(field, addr_saved);
>  }
>  
> +static void test_pat_bytes(u64 vmcs_fld, const char * vmcs_fld_name, u64 ctrl,

VMCS fields are 32-bit encodings, i.e. {vmcs,ctrl}_fld should be u32.  That
should help differentiate between field values and encodings.

E.g. SDM blurb:

  Every component of the VMCS is encoded by a 32-bit field that can be used
  by VMREAD and VMWRITE.

> +			   u64 ctrl_fld)

Oof, this is especially confusing since "vmcs_fld" is an encoding, whereas
"ctrl_fld" is the bit.  How about:

static void test_pat(u32 field, const char *name, u32 ctrl_field, u64 ctrl_bit)

That yields:

  u32 ctrl_saved = vmcs_read(ctrl_field);
  u64 pat_saved = vmcs_read(field);

  ...

  vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit);

  ...

> +{
> +	u32 ctrl_saved = vmcs_read(ctrl);
> +	u64 vmcs_fld_saved = vmcs_read(vmcs_fld);
> +	u64 i, val;
> +	u32 j;
> +	int error;
> +
> +	vmcs_write(ctrl, ctrl_saved & ~ctrl_fld);
> +	for (i = 0; i < 9; i++) {

This needs a comment explaning the purpose/effects of i=0..8, and the
changelog needs to document why we're choosing to test 0..8 instead of
0..255.

> +		/* Test PAT0..PAT7 fields */
> +		for (j = 0; j < 8; j++) {
> +			val = i << j * 8;
> +			vmcs_write(vmcs_fld, val);
> +			report_prefix_pushf("%s %lx", vmcs_fld_name, val);
> +			test_vmx_vmlaunch(0, false);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	vmcs_write(ctrl, ctrl_saved | ctrl_fld);
> +	for (i = 0; i < 9; i++) {
> +		/* Test PAT0..PAT7 fields */
> +		for (j = 0; j < 8; j++) {
> +			val = i << j * 8;
> +			vmcs_write(vmcs_fld, val);
> +			report_prefix_pushf("%s %lx", vmcs_fld_name, val);
> +			if (i == 0x2 || i == 0x3 || i == 0x8)

This should be "i >= 8" to match the actual check and so that it doesn't
explode if someone modifies the outer loop.

> +				error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> +			else
> +				error = 0;
> +			test_vmx_vmlaunch(error, false);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	vmcs_write(ctrl, ctrl_saved);
> +	vmcs_write(vmcs_fld, vmcs_fld_saved);
> +}
> +
> +/*
> + *  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-).
> + *
> + *  [Intel SDM]
> + */
> +static void test_load_host_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_bytes(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
> +}
> +
>  /*
>   * Check that the virtual CPU checks the VMX Host State Area as
>   * documented in the Intel SDM.
> @@ -5010,6 +5073,8 @@ 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_host_pat();
>  }
>  
>  static bool valid_vmcs_for_vmentry(void)
> -- 
> 2.17.2
> 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux