All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write
@ 2026-10-19 14:26 Teddy Astie
  2026-10-19 21:09 ` Andrew Cooper
  2026-10-23 20:45 ` Andrew Cooper
  0 siblings, 2 replies; 5+ messages in thread
From: Teddy Astie @ 2026-10-19 14:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

The Intel SDM (Vol 3A-13.12.11, June 2026) says

> The semantics of sending a self-IPI via the SELF IPI register
> are identical to sending a self targeted edge triggered fixed
> interrupt with the specified vector.

However, this is not expected to alter the ICR register.

Fix the Xen behavior by sending the Self-IPI directly instead of
emulating it by modifying ICR register.

Fixes: 21c4d089ba2a ("x86 hvm: x2APIC emulation")
Assisted-by: Claude Fable 5.1
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
Linux/KVM has a similar patch :

KVM: x86: Do not change ICR on write to APIC_SELF_IPI
https://github.com/torvalds/linux/commit/d22a81b304a27fca6124174a8e842e826c193466

 xen/arch/x86/hvm/vlapic.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 885f5d304b..bb105225e2 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -963,6 +963,17 @@ static int cf_check vlapic_mmio_write(
     return X86EMUL_OKAY;
 }
 
+/*
+ * A write to the SELF IPI register sends a fixed, edge triggered IPI with
+ * the given vector to the local APIC itself.  Unlike an ICR write it doesn't
+ * update the ICR, which is guest visible, in x2APIC mode even without any
+ * intervention by Xen when APIC-register virtualization is in use.
+ */
+static void vlapic_self_ipi(struct vlapic *vlapic, uint32_t val)
+{
+    vlapic_ipi(vlapic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
+}
+
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
@@ -973,8 +984,9 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
         if ( offset != APIC_SELF_IPI )
             return X86EMUL_UNHANDLEABLE;
 
-        offset = APIC_ICR;
-        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
+        vlapic_self_ipi(vlapic, val);
+
+        return X86EMUL_OKAY;
     }
 
     vlapic_reg_write(v, offset, val);
@@ -1050,9 +1062,8 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val)
     case APIC_SELF_IPI:
         if ( val & ~APIC_VECTOR_MASK )
             return X86EMUL_EXCEPTION;
-        offset = APIC_ICR;
-        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
-        break;
+        vlapic_self_ipi(vlapic, val);
+        return X86EMUL_OKAY;
 
     case APIC_EOI:
     case APIC_ESR:
-- 
2.56.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write
  2026-10-19 14:26 [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write Teddy Astie
@ 2026-10-19 21:09 ` Andrew Cooper
  2026-10-19 23:29   ` Andrew Cooper
  2026-10-23 20:45 ` Andrew Cooper
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2026-10-19 21:09 UTC (permalink / raw)
  To: Teddy Astie, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On 19/10/2026 3:26 pm, Teddy Astie wrote:
> The Intel SDM (Vol 3A-13.12.11, June 2026) says
>
>> The semantics of sending a self-IPI via the SELF IPI register
>> are identical to sending a self targeted edge triggered fixed
>> interrupt with the specified vector.
> However, this is not expected to alter the ICR register.
>
> Fix the Xen behavior by sending the Self-IPI directly instead of
> emulating it by modifying ICR register.
>
> Fixes: 21c4d089ba2a ("x86 hvm: x2APIC emulation")
> Assisted-by: Claude Fable 5.1
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

What matters is the behaviour on real hardware.

If we're taking a strict interpretation, then "identical" means that ICR
does get mutated.

GNR:

    (XEN) '1' pressed -> Extreme debugging in progress...
    (XEN) *** Testing self-IPI
    (XEN)   Before: ICR 00000000000010ff, IRR 00000000
    (XEN)    After: ICR 00000000000400fc, IRR 90000000
    (XEN) *** SPIV

Turin:

    (XEN) '1' pressed -> Extreme debugging in progress...
    (XEN) *** Testing self-IPI
    (XEN)   Before: ICR 00000000000000ff, IRR 80000000
    (XEN)    After: ICR 00000000000000ff, IRR 90000000
    (XEN) *** SPIV


So, AMD doesn't mutate ICR.  Intel does.

Weirdly, Intel reads back ICR_BUSY which shouldn't exist in x2APIC,
while AMD fails to read back the shorthand.

See the attached patch.  The test sends SPIV to CPU0 using ICR, then a
self EVENT_CHECK sampling ICR and IRR before and after.  The SPIV
printk() only gets serviced later, which is why it's the last of the
printk()s

~Andrew

[-- Attachment #2: 0001-dbg-self-ipi.patch --]
[-- Type: text/x-patch, Size: 3218 bytes --]

From d46eee03ea23327b0a480c03fc227e8eeb6da96a Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Sat, 28 Mar 2015 18:06:47 +0000
Subject: dbg self-ipi


diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 176f1498e380..cc0ac160ea38 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1265,23 +1265,10 @@ static void cf_check spurious_interrupt(void)
      * Vectored interrupts are ACKed; spurious interrupts are not.
      */
     if (apic_isr_read(SPURIOUS_APIC_VECTOR)) {
-        bool is_spurious;
-
         ack_APIC_irq();
-        is_spurious = !nmi_check_continuation();
-        if (this_cpu(state_dump_pending)) {
-            this_cpu(state_dump_pending) = false;
-            dump_execstate(get_irq_regs());
-            is_spurious = false;
-        }
-
-        if ( !is_spurious )
-            return;
     }
 
-    /* see sw-dev-man vol 3, chapter 7.4.13.5 */
-    printk(KERN_INFO "spurious APIC interrupt on CPU#%d, should "
-           "never happen.\n", smp_processor_id());
+    printk("*** SPIV\n");
 }
 
 /*
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ac01ba08723d..b35d3ff28857 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2687,3 +2687,76 @@ void asmlinkage entry_from_xen(struct cpu_user_regs *regs)
  * indent-tabs-mode: nil
  * End:
  */
+
+#include <xen/keyhandler.h>
+
+static void cf_check check_self_ipi(void *_)
+{
+    unsigned long flags;
+    uint64_t icr_before, icr_after;
+    uint32_t irr_before, irr_after;
+
+    local_irq_save(flags);
+
+    asm ("mfence;lfence");
+
+    /* Force use of SPURIOUS_APIC_VECTOR, 0xff */
+    apic_wrmsr(APIC_ICR,
+               APIC_DM_FIXED | APIC_DEST_PHYSICAL | SPURIOUS_APIC_VECTOR);
+
+    asm ("mfence;lfence");
+
+    icr_before = apic_rdmsr(APIC_ICR);
+    irr_before = apic_rdmsr(APIC_IRR + (0xf0/32 * 0x10));
+
+    asm ("mfence;lfence");
+
+    /* Send ourselves EVENT_CHECK_VECTOR, 0xfc */
+    apic_wrmsr(APIC_SELF_IPI, EVENT_CHECK_VECTOR);
+
+    asm ("mfence;lfence");
+
+    icr_after = apic_rdmsr(APIC_ICR);
+    irr_after = apic_rdmsr(APIC_IRR + (0xf0/32 * 0x10));
+
+    asm ("mfence;lfence");
+
+    local_irq_restore(flags);
+
+    printk("  Before: ICR %016"PRIx64", IRR %08x\n", icr_before, irr_before);
+    printk("   After: ICR %016"PRIx64", IRR %08x\n", icr_after,  irr_after);
+}
+
+static void cf_check do_extreme_debug(unsigned char key, bool unused)
+{
+    printk("'%c' pressed -> Extreme debugging in progress...\n", key);
+
+    switch ( key )
+    {
+    case '1':
+        if ( !x2apic_enabled )
+        {
+            printk("*** No x2APIC, skipping\n");
+            break;
+        }
+
+        printk("*** Testing self-IPI\n");
+        on_selected_cpus(cpumask_of(0), check_self_ipi, NULL, 1);
+        break;
+
+    case '2':
+        break;
+
+    case '3':
+        break;
+    }
+}
+
+static int __init cf_check extreme_debug_keyhandler_init(void)
+{
+    register_irq_keyhandler('1', &do_extreme_debug, "dbg 1", 0);
+    register_irq_keyhandler('2', &do_extreme_debug, "dbg 2", 0);
+    register_irq_keyhandler('3', &do_extreme_debug, "dbg 3", 0);
+    return 0;
+}
+__initcall(extreme_debug_keyhandler_init);

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write
  2026-10-19 21:09 ` Andrew Cooper
@ 2026-10-19 23:29   ` Andrew Cooper
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2026-10-19 23:29 UTC (permalink / raw)
  To: Teddy Astie, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné

On 19/10/2026 10:09 pm, Andrew Cooper wrote:
> On 19/10/2026 3:26 pm, Teddy Astie wrote:
>> The Intel SDM (Vol 3A-13.12.11, June 2026) says
>>
>>> The semantics of sending a self-IPI via the SELF IPI register
>>> are identical to sending a self targeted edge triggered fixed
>>> interrupt with the specified vector.
>> However, this is not expected to alter the ICR register.
>>
>> Fix the Xen behavior by sending the Self-IPI directly instead of
>> emulating it by modifying ICR register.
>>
>> Fixes: 21c4d089ba2a ("x86 hvm: x2APIC emulation")
>> Assisted-by: Claude Fable 5.1
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> What matters is the behaviour on real hardware.
>
> If we're taking a strict interpretation, then "identical" means that ICR
> does get mutated.
>
> GNR:
>
>     (XEN) '1' pressed -> Extreme debugging in progress...
>     (XEN) *** Testing self-IPI
>     (XEN)   Before: ICR 00000000000010ff, IRR 00000000
>     (XEN)    After: ICR 00000000000400fc, IRR 90000000
>     (XEN) *** SPIV
>
> Turin:
>
>     (XEN) '1' pressed -> Extreme debugging in progress...
>     (XEN) *** Testing self-IPI
>     (XEN)   Before: ICR 00000000000000ff, IRR 80000000
>     (XEN)    After: ICR 00000000000000ff, IRR 90000000
>     (XEN) *** SPIV
>
>
> So, AMD doesn't mutate ICR.  Intel does.
>
> Weirdly, Intel reads back ICR_BUSY which shouldn't exist in x2APIC,
> while AMD fails to read back the shorthand.
>
> See the attached patch.  The test sends SPIV to CPU0 using ICR, then a
> self EVENT_CHECK sampling ICR and IRR before and after.  The SPIV
> printk() only gets serviced later, which is why it's the last of the
> printk()s

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2965354494

It looks like it's a behavioural difference between Core and Atom.  Core
modify ICR, while Atom do not.

Also Atom look to be delivering the "SPIV" IPI too early.  That will be
a separate bug.

~Andrew


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write
@ 2026-10-19 23:40 Christian Ludloff
  0 siblings, 0 replies; 5+ messages in thread
From: Christian Ludloff @ 2026-10-19 23:40 UTC (permalink / raw)
  To: xen-devel

> It looks like it's a behavioural difference between Core and Atom.
> Core modify ICR, while Atom do not.

For Core there might be more nuance yet.

I seem to have an ICL result for "does not".

And I seem to have two SKL machs (same
stepping, same ucode) that... disagree with
each other. Some sort of config thing...?

Maybe Intel can explain.

I consider "does not" the cleaner behavior,
unless a compelling explanation is offered.

--
C.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write
  2026-10-19 14:26 [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write Teddy Astie
  2026-10-19 21:09 ` Andrew Cooper
@ 2026-10-23 20:45 ` Andrew Cooper
  1 sibling, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2026-10-23 20:45 UTC (permalink / raw)
  To: Teddy Astie, xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné

On 19/10/2026 3:26 pm, Teddy Astie wrote:
> The Intel SDM (Vol 3A-13.12.11, June 2026) says
>
>> The semantics of sending a self-IPI via the SELF IPI register
>> are identical to sending a self targeted edge triggered fixed
>> interrupt with the specified vector.
> However, this is not expected to alter the ICR register.

Having spoken to Intel, all Core/P-core's implement SELF_IPI using ICR,
so do mutate the value.

More recent Atom/E-core designs implement SELF_IPI differently, and do
not mutate ICR.  From the testing I performed in Gitlab-CI, "More
recent" is at least AlderLake and later, so Gracemont cores.

Either way, this is conformation that both behaviours are legitimate,
and it is certainly not a "bugfix" to change the behaviour.

There might be enough reason to change the behaviour, as the Atom
behaviour "feels" cleaner, and because it reduces the work we need to do
in the SELF_IPI path, but a justification needs to be along these lines,
not in terms of being a bugfix.

Perhaps a better reason to make the change is because the VT-x
architecture appears to not overwrite ICR, even on Core designs, and
this would be a behavioural difference between APICV or not.

> Fix the Xen behavior by sending the Self-IPI directly instead of
> emulating it by modifying ICR register.
>
> Fixes: 21c4d089ba2a ("x86 hvm: x2APIC emulation")
> Assisted-by: Claude Fable 5.1
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> Linux/KVM has a similar patch :
>
> KVM: x86: Do not change ICR on write to APIC_SELF_IPI
> https://github.com/torvalds/linux/commit/d22a81b304a27fca6124174a8e842e826c193466
>
>  xen/arch/x86/hvm/vlapic.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 885f5d304b..bb105225e2 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -963,6 +963,17 @@ static int cf_check vlapic_mmio_write(
>      return X86EMUL_OKAY;
>  }
>  
> +/*
> + * A write to the SELF IPI register sends a fixed, edge triggered IPI with
> + * the given vector to the local APIC itself.  Unlike an ICR write it doesn't
> + * update the ICR, which is guest visible, in x2APIC mode even without any
> + * intervention by Xen when APIC-register virtualization is in use.
> + */
> +static void vlapic_self_ipi(struct vlapic *vlapic, uint32_t val)
> +{
> +    vlapic_ipi(vlapic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
> +}

I'm not sure a wrapper is really worth it, but if you do care to
continue with the patch, then you want a uint8_t vec here, and drop the
mask.

~Andrew


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-23 20:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-19 14:26 [PATCH] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write Teddy Astie
2026-10-19 21:09 ` Andrew Cooper
2026-10-19 23:29   ` Andrew Cooper
2026-10-23 20:45 ` Andrew Cooper
  -- strict thread matches above, loose matches on Subject: below --
2026-10-19 23:40 Christian Ludloff

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.