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

[Sean Christopherson <seanjc@google.com>]

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...