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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/11/2026 1:46 pm, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
> index 4b0debfa9a2e..2224f1834741 100644
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -30,12 +30,14 @@
>  static void svm_inject_nmi(struct vcpu *v)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> +    vintr_t vintr = vmcb_get_vintr(vmcb);
> +    uint32_t general1_intercepts;
>      intinfo_t event;
>  
> -    if ( vmcb->_vintr.fields.vnmi_enable )
> +    if ( vintr.fields.vnmi_enable )
>      {
> -        vmcb->_vintr.fields.vnmi_pending = true;
> +        vintr.fields.vnmi_pending = true;
> +        vmcb_set_vintr(vmcb, vintr);
>          return;
>      }
>  
> @@ -51,6 +53,7 @@ static void svm_inject_nmi(struct vcpu *v)
>       * SVM does not virtualise the NMI mask, so we emulate it by intercepting
>       * the next IRET and blocking NMI injection until the intercept triggers.
>       */
> +    general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>      vmcb_set_general1_intercepts(
>          vmcb, general1_intercepts | GENERAL1_INTERCEPT_IRET);
>  }

An observation.  The code generation for this is terrible.

ffff82d0402e8640 <svm_inject_nmi>:
ffff82d0402e8640:    48 83 ec 18              sub    $0x18,%rsp
ffff82d0402e8644:    48 8b 87 80 05 00 00     mov    0x580(%rdi),%rax
ffff82d0402e864b:    48 8b 50 60              mov    0x60(%rax),%rdx    // Read 
vmcb->_vintr
ffff82d0402e864f:    48 89 54 24 08           mov    %rdx,0x8(%rsp)     // 
Spill once
ffff82d0402e8654:    48 89 14 24              mov    %rdx,(%rsp)        // 
Spill twice
ffff82d0402e8658:    81 e2 00 00 00 04        and    $0x4000000,%edx    // Test 
vintr.vnmi_en
ffff82d0402e865e:    75 28                    jne    ffff82d0402e8688 
<svm_inject_nmi+0x48>
ffff82d0402e8660:    b9 02 02 00 80           mov    $0x80000202,%ecx
ffff82d0402e8665:    81 48 0c 00 00 10 00     orl    $0x100000,0xc(%rax)
ffff82d0402e866c:    80 a0 c0 00 00 00 fe     andb   $0xfe,0xc0(%rax)
ffff82d0402e8673:    48 89 88 a8 00 00 00     mov    %rcx,0xa8(%rax)
ffff82d0402e867a:    48 83 c4 18              add    $0x18,%rsp
ffff82d0402e867e:    e9 9d a9 f1 ff           jmp    ffff82d040203020 
<__x86_return_thunk>
ffff82d0402e8683:    0f 1f 44 00 00           nopl   0x0(%rax,%rax,1)
ffff82d0402e8688:    80 4c 24 01 08           orb    $0x8,0x1(%rsp)     // set 
vnmi_pending=true, on the stack
ffff82d0402e868d:    48 8b 14 24              mov    (%rsp),%rdx        // 
unspill
ffff82d0402e8691:    80 a0 c0 00 00 00 f7     andb   $0xf7,0xc0(%rax)   // 
Clear vmcb->_cleanbits.tpr
ffff82d0402e8698:    48 89 54 24 10           mov    %rdx,0x10(%rsp)    // 
Re-spill to the other stack copy
ffff82d0402e869d:    48 89 50 60              mov    %rdx,0x60(%rax)    // 
Write vmcb->_vintr
ffff82d0402e86a1:    48 83 c4 18              add    $0x18,%rsp
ffff82d0402e86a5:    e9 76 a9 f1 ff           jmp    ffff82d040203020 
<__x86_return_thunk>


That is, vintr is spilled to the stack twice, with one being a
write-only copy.

Changing the static inline accessors to macros drops two words off the
stack, but there's still one vintr on the stack, and it's the one which
accumulates setting vnmi_pending=true.

The only way I can get the code generation to work nicely is to bypass
the getter/setter entirely.  With only the getter, there's still a dead
word on the stack which doesn't get accessed.  With no vintr local
variable, we get this:

ffff82d0402e8620 <svm_inject_nmi>:
ffff82d0402e8620:    48 8b 87 80 05 00 00     mov    0x580(%rdi),%rax
ffff82d0402e8627:    f6 40 63 04              testb  $0x4,0x63(%rax)
ffff82d0402e862b:    75 23                    jne    ffff82d0402e8650 
<svm_inject_nmi+0x30>
ffff82d0402e862d:    ba 02 02 00 80           mov    $0x80000202,%edx
ffff82d0402e8632:    81 48 0c 00 00 10 00     orl    $0x100000,0xc(%rax)
ffff82d0402e8639:    80 a0 c0 00 00 00 fe     andb   $0xfe,0xc0(%rax)
ffff82d0402e8640:    48 89 90 a8 00 00 00     mov    %rdx,0xa8(%rax)
ffff82d0402e8647:    e9 d4 a9 f1 ff           jmp    ffff82d040203020 
<__x86_return_thunk>
ffff82d0402e864c:    0f 1f 40 00              nopl   0x0(%rax)
ffff82d0402e8650:    80 48 61 08              orb    $0x8,0x61(%rax)
ffff82d0402e8654:    80 a0 c0 00 00 00 f7     andb   $0xf7,0xc0(%rax)
ffff82d0402e865b:    e9 c0 a9 f1 ff           jmp    ffff82d040203020 
<__x86_return_thunk>


which is a far more reasonable attempt by the compiler.

~Andrew