On 8/3/2023 3:43 AM, Sean Christopherson wrote:
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.
I'll do so in next version, thanks!
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.
OK, will add these changes in next version.
+ 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.
I think this case is executed with !CPU_URG, so RM is "converted" to PM because we
have below in KVM:
bool urg = nested_cpu_has2(vmcs12,
SECONDARY_EXEC_UNRESTRICTED_GUEST);
bool prot_mode = !urg || vmcs12->guest_cr0 & X86_CR0_PE;
...
if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
!nested_cpu_has_no_hw_errcode(vcpu)) {
/* VM-entry interruption-info field: deliver error code */
should_have_error_code =
intr_type == INTR_TYPE_HARD_EXCEPTION &&
prot_mode &&
x86_exception_has_error_code(vector);
if (CC(has_error_code != should_have_error_code))
return -EINVAL;
}
so on platform with basic.errcode == 1, this case passes.
report_prefix_pop();
ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |
