Re: [PATCH v3 2/2] x86/svm: Use the virtual NMI when available

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/02/2026 10:46 am, Teddy Astie wrote:
> Le 16/02/2026 à 11:16, Andrew Cooper a écrit :
>> On 16/02/2026 10:07 am, Teddy Astie wrote:
>>> Le 15/02/2026 à 19:24, Abdelkareem Abdelsaamad a écrit :
>>>> With the Virtual NMI (vNMI), the pending NMI is simply stuffed into the 
>>>> VMCB
>>>> and handed off to the hardware. There is no need for the artificial 
>>>> tracking
>>>> of the NMI handling completion with the IRET instruction interception.
>>>>
>>>> Adjust the svm_inject_nmi to rather inject the NMIs using the vNMI Hardware
>>>> accelerated feature when the AMD platform support the vNMI.
>>>>
>>>> Adjust the svm_get_interrupt_shadow to check if the vNMI is currently 
>>>> blocked
>>>> by servicing another in-progress NMI.
>>>>
>>>> Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>
>>>> ---
>>>>    xen/arch/x86/hvm/svm/intr.c | 9 +++++++++
>>>>    xen/arch/x86/hvm/svm/svm.c  | 5 ++++-
>>>>    xen/arch/x86/hvm/svm/vmcb.c | 2 ++
>>>>    3 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
>>>> index 6453a46b85..3e8959f155 100644
>>>> --- a/xen/arch/x86/hvm/svm/intr.c
>>>> +++ b/xen/arch/x86/hvm/svm/intr.c
>>>> @@ -33,6 +33,15 @@ static void svm_inject_nmi(struct vcpu *v)
>>>>        u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>>>>        intinfo_t event;
>>>>    
>>>> +    if ( vmcb->_vintr.fields.vnmi_enable )
>>>> +    {
>>>> +       if ( !vmcb->_vintr.fields.vnmi_pending &&
>>>> +            !vmcb->_vintr.fields.vnmi_blocking )
>>>> +           vmcb->_vintr.fields.vnmi_pending = 1;
>>>> +
>>>> +        return;
>>>> +    }
>>>> +
>>> I think you need to update the clearbit for tpr (related to vintr) for
>>> the hardware to know that you modified the vnmi_pending bit.
>> What makes you think this?  The APM states otherwise.
>>
> The APM state doesn't appears to state regarding this;

The APM does state what is part of the TPR cleanbit, and vNMI is not
amongst these.

>  it's neither a 
> part of the "VMCB Clear Field" layout part nor the the "explicitely not 
> cached" list.
>
> So I assume that it may be covered by the TPR clean-bit (which is the 
> same part of vmcb); as it's actually unclear if hardware wants it or not.

This is very different to your original feedback though.

It's fine for review feedback to be of the form "have you investigated
the clean bits?", but if you're not sure, you must not phrase your
feedback as an instruction to check the cleanbit.

~Andrew