Re: [PATCH] x86/svm: Adjust vNMI handling to update the cleanbit

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/11/2026 4:12 pm, Ross Lagerwall wrote:
>> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> on behalf of Andrew 
>> Cooper <andrew.cooper3@xxxxxxxxxx>
>> Sent: Monday, November 02, 2026 13:54
>> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Jan Beulich 
>> <jbeulich@xxxxxxxx>; Roger Pau Monné <roger@xxxxxxxxxxxxxx>; Teddy Astie 
>> <teddy.astie@xxxxxxxxxx>; Jason Andryuk <jason.andryuk@xxxxxxx>
>> Subject: Re: [PATCH] x86/svm: Adjust vNMI handling to update the cleanbit
>>
>>
>> On 02/11/2026 1:46 pm, Andrew Cooper wrote:
>>> Despite prior information to the contrary, APM Vol2 revision 3.46 October 
>>> 2026
>>> has been adjusted to state that the vNMI fields in vINTR are covered by the
>>> TPR cleanbit.
>>>
>>> Adjust svm_inject_nmi() and svm_set_interrupt_shadow() to use 
>>> vmcb_set_vintr()
>>> rather than writing into the VMCB directly.
>>>
>>> In svm_inject_nmi(), fix an inefficiency from the introduction of vNMI.  
>>> Move
>>> the reading of vmcb_get_general1_intercepts() to the point at which it is
>>> used; tt does not need reading on the case that vNMIs are enabled.
>>>
>>> Fixes: 86a203ab8d38 ("x86/svm: Support vNMI on capable hardware")
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>>> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
>>> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>> CC: Jason Andryuk <jason.andryuk@xxxxxxx>
>> I forgot the extra comment.  This bug is hidden by:
>>
>>     /* The exit may have updated the TPR: reflect this in the hardware
>> vtpr */
>>     intr = vmcb_get_vintr(vmcb);
>>     intr.fields.tpr =
>>         (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
>>     vmcb_set_vintr(vmcb, intr);
>> }
>>
>> at the very end of the vmexit handler which unconditionally writes vINTR
>> and hits the cleanbit.
>>
>> This is an absurd codepath.  SVM has virtualised CR8 since the very
>> beginning, and put in CR8_LEGACY for Windows to use even in 32bit mode,
>> so the only path modifying TPR is a non-accelerated write to APIC_TPR. 
>>  That path can be updated to modify vINTR, and this logic dropped.
>>
>> ~Andrew
> The APM says "(Offset 60h–67h)" for this bit in the current and previous
> versions. Perhaps that means that more or everything in that range may be
> covered by this bit in future? In which case more extensive changes would be
> needed to avoid future breakage.

The vINTR field is complicated, because some bits are unconditional,
while some are covered by a cleanbit.

Byte 63h (V_INTR_MASKING, vGIF_EN, vNMI_EN, {,x2}AVIC_EN) all control
virtualisation behaviour.  These are unconditional, and I bet the 3
reserved bits in there will continue to behave in the same way when they
are given meaning.

All other bytes are state of the vCPU, and are (now documented to be)
subject to the cleanbit.

> On a related note, I see the APM says this:
>> If any VMCB fields (excluding explicitly uncached fields) have been modified,
>> all clean bits that are undefined (within the scope of the hypervisor) must 
>> be
>> cleared to zero.
> AFAICT Xen doesn't do this. Might that cause breakage in future?

Fun...  That's incompatible with the previous sentence.

Xen follows the previous advice, and sets:

    vmcb->cleanbits.raw = ~0u;

early(ish) in the VMExit handler.

~Andrew