Re: [RFC PATCH] x86/apic: Fix lost IRQ during forced vector migration on Hyper-V

[Thomas Gleixner <tglx@kernel.org>]

On Fri, Jul 10 2026 at 05:40, Naman Jain wrote:
>
> Fixes: e84cf6aa501c5 ("x86/apic/vector: Handle vector release on CPU unplug correctly")

Errm. This does not fix anything in the common code. It fixes a hyperv
PCI/MSI problem.

>  	} else {
> +		/*
> +		 * The outgoing CPU cannot use the deferred cleanup-vector
> +		 * mechanism, so its vector is freed inline below. On Hyper-V the
> +		 * MSI retarget hypercall is asynchronous, so an interrupt raised
> +		 * inside the stop_machine window can be posted to the outgoing
> +		 * CPU's old vIRR after the free. Two complementary steps handle
> +		 * that (see also the retrigger at the end of the function):
> +		 *
> +		 *  - Retrigger on the new target so a raced completion is drained
> +		 *    there rather than lost. The retarget is asynchronous, so the
> +		 *    outgoing IRR is not authoritative and the IPI is issued
> +		 *    unconditionally; a spurious ISR is harmless to
> +		 *    completion-draining handlers (they find an empty queue).
> +		 *  - Mark the freed slot VECTOR_RETRIGGERED so a late stray is
> +		 *    absorbed by reevaluate_vector() instead of logging "No irq
> +		 *    handler" while the CPU still takes interrupts during
> +		 *    teardown; __setup_vector_irq() resets it on re-online.
> +		 *
> +		 * This mirrors msi_set_affinity()'s protection, which the Hyper-V
> +		 * MSI chip bypasses via IRQCHIP_MOVE_DEFERRED.

What has msi_set_affinity() and IRQCHIP_MOVE_DEFERRED to do with this?

  - msi_set_affinity() solves a completely different problem. It handles
    the case when the PCI/MSI device does not provide masking of the MSI
    interrupt across the message change.

    Q: What exactly is mirrored here?
    A: Nothing

  - In the context of CPU hot-unplug IRQCHIP_MOVE_DEFERRED is not
    deferring anything simply because there is no way to do the deferred
    (in interrupt context) move on the original target CPU.

    fixup_irqs()
       irq_migrate_all_off_this_cpu()
          migrate_one_irq()
            irq_do_set_affinity()

    The only check there which is related to IRQCHIP_MOVE_DEFERRED is
    the masking decision in migrate_one_irq(), but that has nothing to
    do with the semantics of IRQCHIP_MOVE_DEFERRED.

    That flag tells the interrupt core that regular affinity changes
    have to be delayed into the context of the next device interrupt,
    which is obviously not possible for interrupt startup and forced
    migration on CPU hot-unplug, which both ignore that restriction. The
    only effect in CPU hot-unplug migrations is that the interrupt is
    masked at the outermost domain level across the affinity change
    operation because that's an requirement for msi_set_affinity() and
    similar workarounds (loongarch) to avoid the multi vector dance.

    The fact that hyper-V invokes the retarget hypercall in context of
    the unmask() callback has nothing to with that flag and due to the
    lack of explanation why the flag is needed in the first place.

I'm all for commenting complex issues, but comments have to be
technically correct, which excludes both LLM and human hallucinations.
The same applies to the change log which carries the same fairy tale.

Aside of that, doing the re-targeting in the unmask() callback
unconditionally is pointless overhead. Why?

    disable_irq($IRQ);             // Sets the disabled flag, but does not mask

    --> $IRQ is raised
    handle_edge_irq($IRQ)
    if (disabled($IRQ))
      mask_irq($IRQ);              // Masks at the PCI/MSI level

    enable_irq($IRQ)
    if (masked($IRQ))
      unmask_irq($IRQ)
        issue_hypercall(RETARGET)  // Pointless exercise when affinity
                                   // has not changed
        unmask_pci_msi();          // Unmasks at the PCI/MSI level

Not that I care at all, but I have no idea why pci-hyperv has to do the
affinity change in the unmask() callback and why it needs to take the
overhead which comes with the deferred affinity changes in general.

> +		 * restricted to edge MSI vectors on Hyper-V (msi_desc present):
> +		 * only the MSI retarget hypercall is asynchronous, so edge IOAPIC
> +		 * lines (retargeted synchronously via the RTE) and level-triggered
> +		 * lines never see this race and must not be force-injected.
> +		 */
> +		if (hypervisor_is_type(X86_HYPER_MS_HYPERV) &&
> +		    apicd->vector >= FIRST_EXTERNAL_VECTOR &&
> +		    !irqd_is_level_type(irqd) && irq_data_get_msi_desc(irqd))

TBH. This is a tasteless layering violation. We are not cluttering
common code with hypervisor specific MSI workarounds. The non-remapped
MSI workaround is not implemented here either.

> +			hv_retrigger = true;
>  		apic_free_vector(apicd->cpu, apicd->vector, managed);
> +		/*
> +		 * apic_free_vector() releases the matrix bit but leaves the
> +		 * outgoing CPU's vector_irq[] slot pointing at the stale desc, so
> +		 * the marker is written unconditionally here.

What's exactly unconditional about

       if (cond)
          write()

?

> +              						(Unlike
> +		 * msi_set_affinity(), which marks a genuinely unused slot and
> +		 * therefore guards with IS_ERR_OR_NULL, the slot here still holds
> +		 * this irq's old desc.)

What has msi_set_affinity() to do with this? It sets RETRIGGER for a
very different reason. These references are confusing at best. Simply
explain WHY you need to do this and explain it correctly.

> +		 */
> +		if (hv_retrigger)
> +			per_cpu(vector_irq, apicd->cpu)[apicd->vector] = VECTOR_RETRIGGERED;
>  	}
>  
>  setnew:
> @@ -190,6 +231,13 @@ static void chip_data_update(struct irq_data *irqd, unsigned int newvec, unsigne
>  	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
>  	per_cpu(vector_irq, newcpu)[newvec] = desc;
>  	apic_update_irq_cfg(irqd, newvec, newcpu);
> +	/*
> +	 * Drain any completion that raced onto the freed vIRR by retriggering

What's a freed vIRR and which completion? This code deals with vector
management and not with hyperV MSI implementation specific completions
and it does not care whether it operates on a physical or a virtual
APIC.

> +	 * on the new target (see the else-branch above). Issued after the new

'See the else-branch above' is as useful as 'See some random explanation
elsewhere'. It's obvious where to look for that information as the
hv_retrigger condition must be set by something to make the condition true.

> +	 * mapping is installed so the handler is present when it is serviced.
> +	 */
> +	if (hv_retrigger)
> +		__apic_send_IPI(newcpu, newvec);

You are claiming above that the MSI retarget hypercall is asynchronous,
but you fail to specify in which way.

  - If you refer to the retarget invocation in the unmask() callback,
    then that happens still before the new target can handle it.

  - If it is truly asynchronous, i.e. the hypervisor handles it at some
    arbitrary point in time, then the retriggered vector on the new
    target CPU can be handled _before_ the retarget is in effect. In
    that case a subsequent interrupt might still end up on the dead CPU,
    no?

IOW, your or your LLM's elaborate comment is useless word salad as it
does not explain the problem in a coherent and comprehensible way. Quite
to the contrary it is more confusing than helpful.

But as I told you already, this is not a problem of incoherent comments,
this is simply the wrong place to address that.

Looking at the pci-hyperv implementation made me immediately notice a
bug, which exists since the driver was merged:

  The driver does not set the irq_retrigger() callback for the
  interrupt chip.

That means that fixup_irqs() can't do anything even when it observes the
IRR bit set. And that matches the incoherent problem description as far
as I can tell from trying to oracle something out of the word salad.

Great that your AI ass-istant did not figure that out and you blindly
trusted the nonsense it hallucinated.

The uncompiled and therefore untested below should exactly do what you
want to achieve without having hyperV specific hackery in the common
code.

If it does not, then you need to provide a coherent technical
explanation why it is not sufficient.

Thanks,

        tglx

---
Subject: TBD

  Instead of adding the missing callback to the hyperv driver, which
  would be the trivial "fix", it changes the x86 fixup_irqs()
  implementation. Why?

  That makes the code more resilient and allows to remove quite a bit of
  initializations of interrupt chips all over the place.

Not-Signed-off-yet-by: Thomas Gleixner <tglx@kernel.org>
Assisted-by: Human Intelligence
---
 arch/x86/kernel/irq.c |   23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -466,11 +466,6 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_posted_msi
 /* A cpu has been removed from cpu_online_mask.  Reset irq affinities. */
 void fixup_irqs(void)
 {
-	unsigned int vector;
-	struct irq_desc *desc;
-	struct irq_data *data;
-	struct irq_chip *chip;
-
 	irq_migrate_all_off_this_cpu();
 
 	/*
@@ -489,22 +484,18 @@ void fixup_irqs(void)
 	 * vector_lock because the cpu is already marked !online, so
 	 * nothing else will touch it.
 	 */
-	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
-		if (IS_ERR_OR_NULL(__this_cpu_read(vector_irq[vector])))
+	for (unsigned int vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
+		struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
+
+		if (IS_ERR_OR_NULL(desc))
 			continue;
 
 		if (is_vector_pending(vector)) {
-			desc = __this_cpu_read(vector_irq[vector]);
-
-			raw_spin_lock(&desc->lock);
-			data = irq_desc_get_irq_data(desc);
-			chip = irq_data_get_irq_chip(data);
-			if (chip->irq_retrigger) {
-				chip->irq_retrigger(data);
+			guard(raw_spinlock)(&desc->lock);
+			if (irq_chip_retrigger_hierarchy(&desc->irq_data))
 				__this_cpu_write(vector_irq[vector], VECTOR_RETRIGGERED);
-			}
-			raw_spin_unlock(&desc->lock);
 		}
+
 		if (__this_cpu_read(vector_irq[vector]) != VECTOR_RETRIGGERED)
 			__this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
 	}