On 8/4/2023 1:11 AM, Sean Christopherson wrote:
On Thu, Aug 03, 2023, Weijiang Yang wrote:
On 8/3/2023 3:43 AM, Sean Christopherson wrote:
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.
Huh. I get the logic, but IMO based on the SDM, that's a ucode bug that got
propagated into KVM (or an SDM bug, which is my bet for how this gets treated).
I verified HSW at least does indeed generate VM-Fail and not VM-Exit(INVALID_STATE),
so it doesn't appear that KVM is making stuff (for once). Either that or I'm
misreading the SDM (definite possibility), but the only relevant condition I see is:
bit 0 (corresponding to CR0.PE) is set in the CR0 field in the guest-state area
I don't see anything in the SDM that states the CR0.PE is assumed to be '1' for
consistency checks when unrestricted guest is disabled.
Can you bug a VMX architect again to get clarification, e.g. to get an SDM update?
Or just point out where I missed something in the SDM, again...
Sorry for the delayed response! Also added Gil in cc.
I got reply from Gil as below:
"I am not sure whether you (or Sean) are referring to guest state or host state.
The IA32_VMX_CR0_FIXED0 and IA32_VMX_CR0_FIXED1 MSRs enumerate which bits must have fixed values in CR0 in VMX operation.
Every CPU that supports VMX operation (not just the first ones) should support these MSRs to set bits 0 and 31, corresponding to PE and PG.
Section 24.8 explains what this means:
1. VMXON fails if attempting to activated VMX root operation when either of those bits is 0.
2. Attempting to clear either bit (with MOV to CR) in VMX operation (root or non-root) will #GP.
3. Attempting to clear either bit through VM entry (loading guest state) will cause the VM entry to fail (with what Sean calls “VM-Exit(INVALID_STATE)”).
4. If a VM exit would clear either bit (loading host state), the VM entry that would indicate the host state will fail (with what Sean calls “VM-Fail”).
Exceptions to #2 and #3 are made only if “unrestricted guest” is 1. (If “unrestricted guest” is 0, the items above all apply.)
If “unrestricted guest” is 1, #2 is relaxed in VMX non-root operation (guest software can clear PE or PG with MOV to CR).
If “unrestricted guest” is 1, #3 is relaxed in that VM entry can load CR0 to clear either PE or PG (but cannot set PG without also setting PE).
I suppose that we could clarify that SDM to indicate that all VMX CPUs enumerate PE and PG as being VMX-fixed to 1 (despite the fact that many CPUs do support the 1-setting of “unrestricted guest”)."
