xen/arch/x86/hvm/svm/intr.c | 9 ++++++--- xen/arch/x86/hvm/svm/svm.c | 8 ++++++-- 2 files changed, 12 insertions(+), 5 deletions(-)
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@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>
CC: Jason Andryuk <jason.andryuk@amd.com>
---
xen/arch/x86/hvm/svm/intr.c | 9 ++++++---
xen/arch/x86/hvm/svm/svm.c | 8 ++++++--
2 files changed, 12 insertions(+), 5 deletions(-)
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);
}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1628c5e0ae05..57779806bf6a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -511,12 +511,16 @@ static void cf_check svm_set_interrupt_shadow(
{
struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
bool block_nmi = intr_shadow & HVM_INTR_SHADOW_NMI;
+ vintr_t vintr = vmcb_get_vintr(vmcb);
vmcb->int_stat.intr_shadow =
!!(intr_shadow & (HVM_INTR_SHADOW_MOV_SS|HVM_INTR_SHADOW_STI));
- if ( vmcb->_vintr.fields.vnmi_enable )
- vmcb->_vintr.fields.vnmi_blocking = block_nmi;
+ if ( vintr.fields.vnmi_enable )
+ {
+ vintr.fields.vnmi_blocking = block_nmi;
+ vmcb_set_vintr(vmcb, vintr);
+ }
else
{
uint32_t gen1 = vmcb_get_general1_intercepts(vmcb);
--
2.39.5
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
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@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monné <roger@xenproject.org>
> CC: Teddy Astie <teddy.astie@vates.tech>
> CC: Jason Andryuk <jason.andryuk@amd.com>
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
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Monday, November 02, 2026 13:54
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger@xenproject.org>; Teddy Astie <teddy.astie@vates.tech>; Jason Andryuk <jason.andryuk@amd.com>
> 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@citrix.com>
> > ---
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Roger Pau Monné <roger@xenproject.org>
> > CC: Teddy Astie <teddy.astie@vates.tech>
> > CC: Jason Andryuk <jason.andryuk@amd.com>
>
> 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
On 02/11/2026 4:12 pm, Ross Lagerwall wrote:
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Andrew Cooper <andrew.cooper3@citrix.com>
>> Sent: Monday, November 02, 2026 13:54
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger@xenproject.org>; Teddy Astie <teddy.astie@vates.tech>; Jason Andryuk <jason.andryuk@amd.com>
>> 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@citrix.com>
>>> ---
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Roger Pau Monné <roger@xenproject.org>
>>> CC: Teddy Astie <teddy.astie@vates.tech>
>>> CC: Jason Andryuk <jason.andryuk@amd.com>
>> 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
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@lists.xenproject.org> on behalf of Andrew Cooper <andrew.cooper3@citrix.com>
>>> Sent: Monday, November 02, 2026 13:54
>>> To: Xen-devel <xen-devel@lists.xenproject.org>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger@xenproject.org>; Teddy Astie <teddy.astie@vates.tech>; Jason Andryuk <jason.andryuk@amd.com>
>>> 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@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>> CC: Roger Pau Monné <roger@xenproject.org>
>>>> CC: Teddy Astie <teddy.astie@vates.tech>
>>>> CC: Jason Andryuk <jason.andryuk@amd.com>
>>> 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
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Monday, November 02, 2026 20:34
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger@xenproject.org>; Teddy Astie <teddy.astie@vates.tech>; Jason Andryuk <jason.andryuk@amd.com>
> 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@lists.xenproject.org> on behalf of Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Sent: Monday, November 02, 2026 13:54
> >>> To: Xen-devel <xen-devel@lists.xenproject.org>
> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger@xenproject.org>; Teddy Astie <teddy.astie@vates.tech>; Jason Andryuk <jason.andryuk@amd.com>
> >>> 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@citrix.com>
> >>>> ---
> >>>> CC: Jan Beulich <jbeulich@suse.com>
> >>>> CC: Roger Pau Monné <roger@xenproject.org>
> >>>> CC: Teddy Astie <teddy.astie@vates.tech>
> >>>> CC: Jason Andryuk <jason.andryuk@amd.com>
> >>> 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
On 02.11.2026 14:46, 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.
Nit: s/tt/it/ ?
> Fixes: 86a203ab8d38 ("x86/svm: Support vNMI on capable hardware")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Assuming the first sentence of the description kind of implies trust into this
being the final word on the matter:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
On 02/11/2026 1:51 pm, Jan Beulich wrote:
> On 02.11.2026 14:46, 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.
> Nit: s/tt/it/ ?
Fixed.
>> Fixes: 86a203ab8d38 ("x86/svm: Support vNMI on capable hardware")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Assuming the first sentence of the description kind of implies trust into this
> being the final word on the matter:
It is formally written statement on the matter, where previously there
was none.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
~Andrew
© 2016 - 2026 Red Hat, Inc.