Re: [kvm-unit-tests PATCH 5/5] nVMX: add a test for canonical checks of various host state vmcs12 fields.

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> +#define TEST_VALUE1  0xff45454545000000
> +#define TEST_VALUE2  0xff55555555000000
> +
> +static void vmx_canonical_test_guest(void)
> +{
> +	while (true)
> +		vmcall();
> +}

...

> +static void test_host_value_natively(const char *field_name, u64 vmcs_field, u64 value)

The error messages say "directly", wheras this says "natively".  I don't have a
strong preference, but they should use the same terminology.  I'll go with
"direct", because "native" has paravirt connotations that could confuse things.

> +{
> +	int vector;
> +	u64 actual_value;
> +
> +	/*
> +	 * Set the register via host native interface (e.g lgdt) and check
> +	 * that we got no exception
> +	 */
> +	vector = set_host_value(vmcs_field, value);
> +	if (vector) {
> +		report(false,

report_fail()

> +		       "Exception %d when setting %s to 0x%lx via host",
> +		       vector, field_name, value);
> +		return;
> +	}
> +
> +	/*
> +	 * Now check that the host value matches what we expect for fields
> +	 * that can be read back (these that we can't we assume that are correct)
> +	 */
> +
> +	if (get_host_value(vmcs_field, &actual_value))
> +		actual_value = value;
> +
> +	report(actual_value == value,

Rather than clobber actual_value, incorporate the get() in the report, e.g.

	report(get_host_value(vmcs_field, &actual_value) || actual_value == value

> +	       "%s: HOST value is set to test value 0x%lx directly",
> +	       field_name, value);

Print the actual value as well, otherwise debugging is painful (well, more painful).

> +}
> +
> +static void test_host_value_via_guest(const char *field_name, u64 vmcs_field, u64 value)

It's not via "guest", it's via the HOST_xxx fields in the VMCS.  test_host_value_vmcs()?

> +{
> +	u64 actual_value;
> +
> +	/* Set host state field in the vmcs and do the VM entry
> +	 * Success of VM entry already shows that L0 accepted the value
> +	 */
> +	vmcs_write(vmcs_field, TEST_VALUE2);

This should be value, not TEST_VALUE2.  Ditto below.  The whole @value idea is
rather pointless though.  There's one caller, and it passes one value.  The only
requirement is that the "direct" vs. "vmcs" settings use different values, e.g.
to avoid false passes.  To handle that, just use more descriptive names for the
#defines.

> +	enter_guest();
> +	skip_exit_vmcall();
> +
> +	/*
> +	 * Now check that the host value matches what we expect for fields
> +	 * that can be read back (these that we can't we assume that are correct)
> +	 */
> +
> +	if (get_host_value(vmcs_field, &actual_value))
> +		actual_value = value;
> +
> +	/* Check that now the msr value is the same as the field value */
> +	report(actual_value == TEST_VALUE2,
> +	       "%s: HOST value is set to test value 0x%lx via VMLAUNCH/VMRESUME",
> +	       field_name, value);
> +}