Re: Re: [PATCH v4] x86/nSVM: Check injected event consistency

[Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>]

On 07.09.2026 08:50, Jan Beulich wrote:
>On 06.09.2026 15:11, Abdelkareem Abdelsaamad wrote:
>> On 26.08.2026 15:33, Jan Beulich wrote:
>>> On 23.08.2026 18:11, Abdelkareem Abdelsaamad wrote:
>>>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>>>> @@ -320,6 +320,44 @@ void svm_vmcb_dump(const char *from, const struct 
>>>> vmcb_struct *vmcb)
>>>>      svm_dump_sel("  TR", &vmcb->tr);
>>>>  }
>>>>  
>>>> +static bool is_valid_injected_exception_vector(const struct vmcb_struct 
>>>> *vmcb,
>>>> +    uint8_t vmcb_injected_vector)
>>>> +{
>>>> +    switch ( vmcb_injected_vector )
>>>> +    {
>>>> +    case X86_EXC_DE:
>>>> +    case X86_EXC_DB:
>>>> +    case X86_EXC_BP:
>>>> +    case X86_EXC_UD:
>>>> +    case X86_EXC_NM:
>>>> +    case X86_EXC_DF:
>>>> +    case X86_EXC_TS:
>>>> +    case X86_EXC_NP:
>>>> +    case X86_EXC_SS:
>>>> +    case X86_EXC_GP:
>>>> +    case X86_EXC_PF:
>>>> +    case X86_EXC_MF:
>>>> +    case X86_EXC_AC:
>>>> +    case X86_EXC_MC:
>>>
>>> Is #MC valid to inject without CR4.MCE set?
>> The testing I performed (see previous comment) does not show that set CR4.MCE
>> is required for the valid injection.
>
>I find this worrying. Roger, any chance you could try to find out whether
>that's perhaps more an erratum than intended behavior?
>
I believe X86_EXC_MC was introduced long before AMD CPUs with SVM. Therefore,
on all the SVM-capable CPUs I am targeting, it is valid to trigger this
exception vector regardless of whether the guest has explicitly opted in for
the capability.
That said, if Roger can double-check and confirm this behavior, it would be
highly appreciated.

>>>> +        return true;
>>>> +
>>>> +    case X86_EXC_OF:
>>>> +    case X86_EXC_BR:
>>>> +        return !(vmcb_get_efer(vmcb) & EFER_LMA) || !vmcb->cs.l;
>>>> +
>>>> +    case X86_EXC_VC:
>>>> +        return vmcb_get_sev_es(vmcb);
>>>> +
>>>> +    case X86_EXC_CP:
>>>> +        return vmcb_get_cr4(vmcb) & X86_CR4_CET;
>>>
>>> ... e.g. here. That is, if a CR4 (or other) check is needed here, but not
>>> for #XM (or #SX), that's surely worth (briefly) commenting upon. The more
>>> that, afaics, none of this is spelled out in the PM.
>> In my testing, the hardware behavior differs across the generations support 
>> for
>> the Control-flow Enforcement Technology (CET):
>>  - Naples (No hardware support): Injecting the event when the feature is
>>    completely unsupported by the CPU results in VMEXIT_INVALID. The VMCB's 
>> CR4
>>    bit is not set as it is expected.
>>  - Genoa (Hardware support exists): If the CPU supports the feature but the
>>    guest has not enabled it in CR4 (not opted-in), injecting the event
>>    results in a triple fault. I am accordingly checking for the CPU feature 
>> and
>>    report it as invalid.
>
>A guest triple fault, I assume?
Yes, that is correct. I meant a guest triple fault.
> I'm not entirely convinced this is a sufficient
>indication of injection being permitted, even though I agree it very much looks
>so. Then again, like above, I'm also unconvinced this is actually intended
>behavior. Guests unaware of a feature (and hence not enabling it) should never
>observe exceptions related to only that feature.
The testing, I performed shows the following behavior across the CPU
generations:
 - On CPUU generations that support the feature (e.g., Genoa supporting 
   Control-flow Enforcement Technology / CET), the injection results in
   a guest triple fault. If the the guest did not opt-in for the CET feature.
   No VMEXIT_INVALID results by the injection.
 - On older hardware generations that completely lack the feature (e.g., 
Naples), 
   the injection immediately results in a VMEXIT_INVALID.

The current hardware behavior seems to depend on whether the underlying
physical CPU understands the feature, rather than whether the guest has opted
into it via CR4. The patch expands this to consider the injection will result
in VMEXIT_INVALID if the guest did not opt into the feature.

Are you suggesting to reather explicitly check the CPU model/generation instead
of checking X86_CR4_CET? For example, allowing the injection on Genoa platforms
regardless of whether the guest has enabled the CET capability? I am concerned 
that handling this via CPU model checks might introduce architectural edge
cases and/or add maintenance complication—what are your thoughts on that
approach?

>>>> @@ -392,6 +436,20 @@ bool svm_vmcb_isvalid(
>>>>          PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
>>>>                 vmcb->event_inj.raw);
>>>>  
>>>> +    if ( !vmcb->event_inj.v )
>>>> +        PRINTF("eventinj: valid bit is not set (%#"PRIx64")\n",
>>>> +               vmcb->event_inj.raw);
>>>
>>> I understand the parentheses in the log message here. Yet ...
>>>
>>>> +    if ( !((1 << vmcb_injected_type) & vmcb_valid_event_inj_types_mask) )
>>>
>>> If vmcb_injected_type really could take all possible uint8_t values (see
>>> below), this shift would be at risk of becoming UB. And uint8_t is a
>>> stronger hint that all possible values may appear than unsigned int is.
>> I am OK to change to unsigned int for safety with future possible extensions.
>>>
>>>> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
>>>> +               vmcb_injected_type);
>>>
>>> ... what purpose do they serve here (and below)?
>> I did it just to go with the previous format but I am OK to drop in v5.
>
>You did notice the difference in message type, though? Where parentheses
>are used in existing messages, the values put there serve as auxiliary
>information to the wording used. Whereas here you plainly dump a value,
>without saying what exactly is wrong (that's actually said by the value
>dumped).
Got it. I will drop the parentheses in v5.