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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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

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?

Ross