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@xxxxxxxxx>
> ---
> 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 |
