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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: Monday, November 02, 2026 20:34
> To: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>; 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 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.

V_INTR_MASKING is covered by cleanbit 3 so I'm not sure that logic holds.

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

VGIF in byte 61h isn't documented as being subject to the cleanbit (yet).

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

Both approaches would surely function correctly but the doc's suggestion
potentially allows more caching if a new bit is introduced (but more complex to
implement). In any case, what Xen currently does wouldn't work.

Ross