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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/11/2026 6:16 pm, Andrew Cooper wrote:
> 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.

It would appear that AMD reserve the right to retroactively introduce
cleanbits.

I maintain that it's absurd to suggest both "write ~0" and "clear all
cleanbits you don't know if you write anything".

The advice should be "write the cleanbits you know" in order to signify
the VMCB as clean.

~Andrew