Re: [kvm-unit-tests PATCH] x86:VMX: Fixup for VMX test failures

[Sean Christopherson <seanjc@google.com>]

This is not "fixup", this is support for CET and for new CPU functionality.

On Thu, Jul 20, 2023, Yang Weijiang wrote:
> CET KVM enabling patch series introduces extra constraints
> on CR0.WP and CR4.CET bits, i.e., setting CR4.CET=1 faults if
> CR0.WP==0. Simply skip CR4.CET bit test to avoid setting it in
> flexible_cr4 and finally triggering a #GP when write the CR4
> with CET bit set while CR0.WP is cleared.
> 
> The enable series also introduces IA32_VMX_BASIC[56 bit] check before
> inject exception to VM, per SDM(Vol 3D, A-1):
> "If bit 56 is read as 1, software can use VM entry to deliver a hardware
> exception with or without an error code, regardless of vector."

This clearly should be at least two separate patches, maybe event three.

  1. Exclude CR4.CET from the test_vmxon_bad_cr()
  2. Add the bit in the "basic" MSR that says the error code consistency check
     is skipped for protected mode and tweak test_invalid_event_injection()

2 could arguably be split, but IMO that's overkill.

> With the change, some test cases expected VM entry failure  will
> end up with successful results which causes reporting failures. Now
> checks the VM launch status conditionally against the bit support
> to get consistent results with the change enforced by KVM.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  x86/vmx.c       |  2 +-
>  x86/vmx.h       |  3 ++-
>  x86/vmx_tests.c | 21 +++++++++++++++++----
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 12e42b0..1c27850 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1430,7 +1430,7 @@ static int test_vmxon_bad_cr(int cr_number, unsigned long orig_cr,
>  		 */
>  		if ((cr_number == 0 && (bit == X86_CR0_PE || bit == X86_CR0_PG)) ||
>  		    (cr_number == 4 && (bit == X86_CR4_PAE || bit == X86_CR4_SMAP ||
> -					bit == X86_CR4_SMEP)))
> +					bit == X86_CR4_SMEP || bit == X86_CR4_CET)))
>  			continue;
>  
>  		if (!(bit & required1) && !(bit & disallowed1)) {
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 604c78f..e53f600 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -167,7 +167,8 @@ union vmx_basic {
>  			type:4,
>  			insouts:1,
>  			ctrl:1,
> -			reserved2:8;
> +			errcode:1,

Way too terse.  Please something similar to whatever #define we use on the KVM
side.  Ignore the existing names, this is one of those "the existing code is
awful" scenarios.

Also, I wouldn't be opposed to a patch to rename the union to "vmx_basic_msr",
and the global variable to basic_msr.  At first glance, I thought "basic.errcode"
was somehow looking at whether or not the basic VM-Exit reason had an error code.

> +			reserved2:7;
>  	};
>  };
>  
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 7952ccb..b6d4982 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4173,7 +4173,10 @@ static void test_invalid_event_injection(void)
>  			    ent_intr_info);
>  	vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG);
>  	vmcs_write(ENT_INTR_INFO, ent_intr_info);
> -	test_vmx_invalid_controls();
> +	if (basic.errcode)
> +		test_vmx_valid_controls();
> +	else
> +		test_vmx_invalid_controls();

This is wrong, no?  The consistency check is only skipped for PM, the above CR0.PE
modification means the target is RM.

>  	report_prefix_pop();
>  
>  	ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |