[PATCH for-4.20 0/2] x86/shutdown: prevent lapic "Receive accept error" errors on AMD

[PATCH for-4.20 0/2] x86/shutdown: prevent lapic "Receive accept error" errors on AMD

[Roger Pau Monne]

Hello,

The following series aims to fix the issues seeing during shutdown on
AMD boxes related to "Receive accept error" local APIC messages.

Fix patch aims to fix the issue, while second patch adjust fixup_irqs()
given it now has a single caller.

I think at least patch 1 should be considered for 4.20, as it fixes a
real issue on AMD boxes that prevents rebooting them.

Thanks, Roger.

Roger Pau Monne (2):
  x86/shutdown: quiesce devices ahead of AP shutdown
  x86/irq: drop fixup_irqs() parameters

 xen/arch/x86/crash.c           |  1 +
 xen/arch/x86/include/asm/irq.h |  4 ++--
 xen/arch/x86/include/asm/msi.h |  1 +
 xen/arch/x86/irq.c             | 30 +++++++++++++-----------------
 xen/arch/x86/msi.c             | 14 ++++++++++++++
 xen/arch/x86/smp.c             | 10 +++++-----
 xen/arch/x86/smpboot.c         |  2 +-
 xen/drivers/passthrough/pci.c  | 32 ++++++++++++++++++++++++++++++++
 xen/include/xen/pci.h          |  2 ++
 9 files changed, 71 insertions(+), 25 deletions(-)

-- 
2.46.0

[PATCH for-4.20 1/2] x86/shutdown: quiesce devices ahead of AP shutdown

[Roger Pau Monne]

The current shutdown logic in smp_send_stop() will first disable the APs,
and then attempt to disable (some) of the interrupt sources.

There are two issues with this approach; the first one being that MSI
interrupt sources are not disabled, the second one is the APs are stopped
before interrupts are disabled.  On AMD systems this can lead to the
triggering of local APIC errors:

APIC error on CPU0: 00(08), Receive accept error

Such error message can be printed in a loop, thus blocking the system from
rebooting.  I assume this loop is created by the error being triggered by
the console interrupt, which is further triggered by the ESR reporting
write to the console.

Intel SDM states:

"Receive Accept Error.

Set when the local APIC detects that the message it received was not
accepted by any APIC on the APIC bus, including itself. Used only on P6
family and Pentium processors."

So the error shouldn't trigger on any Intel CPU supported by Xen.

However AMD doesn't make such claims, and indeed the error is broadcasted
to all local APIC when for example an interrupt targets a CPU that's
offline.

To prevent the error from triggering, move the masking of IO-APIC pins
ahead of stopping the APs.  Also introduce a new function that disables
MSI and MSI-X on all PCI devices.  Remove the call to fixup_irqs() since
there's no point in attempting to move interrupts: all sources will be
either masked or disabled.

For the NMI crash path also call the newly introduced function, with the
hope that disabling MSI and MSI-X will make it easier for the (possible)
crash kernel to boot, as it could otherwise receive the same "Receive
accept error" upon re-enabling interrupts.

Note that this will have the side-effect of preventing further IOMMU
interrupts from being delivered, that's expected and at that point in the
shutdown process no further interaction with the IOMMU should be relevant.
Also note all current callers of smp_send_stop() do so after having called
console_start_sync(), so disabling the console interrupt won't hamper
console output.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/crash.c           |  1 +
 xen/arch/x86/include/asm/msi.h |  1 +
 xen/arch/x86/msi.c             | 14 ++++++++++++++
 xen/arch/x86/smp.c             | 10 +++++-----
 xen/drivers/passthrough/pci.c  | 32 ++++++++++++++++++++++++++++++++
 xen/include/xen/pci.h          |  2 ++
 6 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index a789416ca3ae..55a96d469f47 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -176,6 +176,7 @@ static void nmi_shootdown_cpus(void)
         x2apic_enabled = (current_local_apic_mode() == APIC_MODE_X2APIC);
 
         disable_IO_APIC();
+        pci_disable_msi_all();
         hpet_disable();
     }
 }
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 63adb19820e8..7f9e531f73e6 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -86,6 +86,7 @@ extern int pci_enable_msi(struct pci_dev *pdev, struct msi_info *msi,
 extern void pci_disable_msi(struct msi_desc *msi_desc);
 extern int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off);
 extern void pci_cleanup_msi(struct pci_dev *pdev);
+extern void pci_disable_msi_all(void);
 extern int setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc);
 extern int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
                            hw_irq_controller *handler);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index e2360579deda..f53b50c98f2a 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1248,6 +1248,20 @@ void pci_cleanup_msi(struct pci_dev *pdev)
     msi_free_irqs(pdev);
 }
 
+static int cf_check disable_msi(struct pci_dev *pdev, void *arg)
+{
+    msi_set_enable(pdev, 0);
+    msix_set_enable(pdev, 0);
+
+    return 0;
+}
+
+void pci_disable_msi_all(void)
+{
+    /* Disable MSI and/or MSI-X on all devices. */
+    pci_iterate_devices(disable_msi, NULL);
+}
+
 int pci_reset_msix_state(struct pci_dev *pdev)
 {
     unsigned int pos = pdev->msix_pos;
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 02a6ed7593f3..0cf03660214d 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -358,14 +358,15 @@ void smp_send_stop(void)
 {
     unsigned int cpu = smp_processor_id();
 
+    local_irq_disable();
+    disable_IO_APIC();
+    pci_disable_msi_all();
+    local_irq_enable();
+
     if ( num_online_cpus() > 1 )
     {
         int timeout = 10;
 
-        local_irq_disable();
-        fixup_irqs(cpumask_of(cpu), 0);
-        local_irq_enable();
-
         smp_call_function(stop_this_cpu, NULL, 0);
 
         /* Wait 10ms for all other CPUs to go offline. */
@@ -376,7 +377,6 @@ void smp_send_stop(void)
     if ( cpu_online(cpu) )
     {
         local_irq_disable();
-        disable_IO_APIC();
         hpet_disable();
         __stop_this_cpu();
         x2apic_enabled = (current_local_apic_mode() == APIC_MODE_X2APIC);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 777c6b1a7fdc..9782750f7902 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1803,6 +1803,38 @@ int iommu_do_pci_domctl(
     return ret;
 }
 
+struct segment_iter {
+    int (*handler)(struct pci_dev *pdev, void *arg);
+    void *arg;
+};
+
+static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
+{
+    const struct segment_iter *iter = arg;
+    struct pci_dev *pdev;
+
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+    {
+        int rc = iter->handler(pdev, iter->arg);
+
+        if ( rc )
+            return rc;
+    }
+
+    return 0;
+}
+
+int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
+                        void *arg)
+{
+    struct segment_iter iter = {
+        .handler = handler,
+        .arg = arg,
+    };
+
+    return pci_segments_iterate(iterate_all, &iter);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index f784e9116059..d4c9837af722 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -225,6 +225,8 @@ int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
 struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf);
 struct pci_dev *pci_get_real_pdev(pci_sbdf_t sbdf);
 void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
+int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
+                        void *arg);
 
 uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
 uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg);
-- 
2.46.0

Re: [PATCH for-4.20 1/2] x86/shutdown: quiesce devices ahead of AP shutdown

[Jan Beulich]

On 28.01.2025 17:27, Roger Pau Monne wrote:
> The current shutdown logic in smp_send_stop() will first disable the APs,
> and then attempt to disable (some) of the interrupt sources.
> 
> There are two issues with this approach; the first one being that MSI
> interrupt sources are not disabled, the second one is the APs are stopped
> before interrupts are disabled.  On AMD systems this can lead to the
> triggering of local APIC errors:
> 
> APIC error on CPU0: 00(08), Receive accept error
> 
> Such error message can be printed in a loop, thus blocking the system from
> rebooting.  I assume this loop is created by the error being triggered by
> the console interrupt, which is further triggered by the ESR reporting
> write to the console.
> 
> Intel SDM states:
> 
> "Receive Accept Error.
> 
> Set when the local APIC detects that the message it received was not
> accepted by any APIC on the APIC bus, including itself. Used only on P6
> family and Pentium processors."
> 
> So the error shouldn't trigger on any Intel CPU supported by Xen.
> 
> However AMD doesn't make such claims, and indeed the error is broadcasted
> to all local APIC when for example an interrupt targets a CPU that's
> offline.
> 
> To prevent the error from triggering, move the masking of IO-APIC pins
> ahead of stopping the APs.  Also introduce a new function that disables
> MSI and MSI-X on all PCI devices.  Remove the call to fixup_irqs() since
> there's no point in attempting to move interrupts: all sources will be
> either masked or disabled.
> 
> For the NMI crash path also call the newly introduced function, with the
> hope that disabling MSI and MSI-X will make it easier for the (possible)
> crash kernel to boot, as it could otherwise receive the same "Receive
> accept error" upon re-enabling interrupts.
> 
> Note that this will have the side-effect of preventing further IOMMU
> interrupts from being delivered, that's expected and at that point in the
> shutdown process no further interaction with the IOMMU should be relevant.

This is at most for AMD only. Shouldn't we similarly disable VT-d's
interrupt(s)? (It's only one right now, as we still don't use the QI
completion one.) Even for AMD I'm uncertain: It has separate
hw_irq_controller instances, and its set_iommu_interrupt_handler() is
custom as well. Will pci_disable_msi_all() really affect it? (Hmm,
yes, from amd_iommu_msi_enable() it looks like it will.)

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1248,6 +1248,20 @@ void pci_cleanup_msi(struct pci_dev *pdev)
>      msi_free_irqs(pdev);
>  }
>  
> +static int cf_check disable_msi(struct pci_dev *pdev, void *arg)
> +{
> +    msi_set_enable(pdev, 0);
> +    msix_set_enable(pdev, 0);
> +
> +    return 0;
> +}
> +
> +void pci_disable_msi_all(void)
> +{
> +    /* Disable MSI and/or MSI-X on all devices. */
> +    pci_iterate_devices(disable_msi, NULL);
> +}

That's going to be all devices we know of. I.e. best effort only. Maybe
the comment should be adjusted to this effect.

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -358,14 +358,15 @@ void smp_send_stop(void)
>  {
>      unsigned int cpu = smp_processor_id();
>  
> +    local_irq_disable();
> +    disable_IO_APIC();
> +    pci_disable_msi_all();
> +    local_irq_enable();
> +
>      if ( num_online_cpus() > 1 )
>      {
>          int timeout = 10;
>  
> -        local_irq_disable();
> -        fixup_irqs(cpumask_of(cpu), 0);
> -        local_irq_enable();
> -
>          smp_call_function(stop_this_cpu, NULL, 0);
>  
>          /* Wait 10ms for all other CPUs to go offline. */
> @@ -376,7 +377,6 @@ void smp_send_stop(void)
>      if ( cpu_online(cpu) )
>      {
>          local_irq_disable();
> -        disable_IO_APIC();
>          hpet_disable();

Like IOMMUs, HPET also has custom interrupt management. I think this
call needs pulling up, too (much like it is also there in
nmi_shootdown_cpus()).

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1803,6 +1803,38 @@ int iommu_do_pci_domctl(
>      return ret;
>  }
>  
> +struct segment_iter {
> +    int (*handler)(struct pci_dev *pdev, void *arg);
> +    void *arg;
> +};
> +
> +static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
> +{
> +    const struct segment_iter *iter = arg;
> +    struct pci_dev *pdev;
> +
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        int rc = iter->handler(pdev, iter->arg);
> +
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
> +                        void *arg)
> +{
> +    struct segment_iter iter = {
> +        .handler = handler,
> +        .arg = arg,
> +    };
> +
> +    return pci_segments_iterate(iterate_all, &iter);
> +}

For the specific purpose during shutdown it may be okay to do all of this
without locking (but see below) and without preemption checks. Yet then a
warning will want putting here to indicate that from other environments
this isn't okay to use as-is.

This use then also requires that msi{,x}_set_enable() paths never gain
lock-related assertions.

Talking of the lack of locking: Since you invoke the disabling before
bringing down APs, we're ending up in kind of a chicken and egg problem
here: Without APs quiesced, there may be operations in progress there
which conflict with the disabling done here. Hence why so far we brought
down APs first.

With this special-purpose use I further wonder whether iterate_all()
wouldn't better continue despite an error coming back from a callback
(and also arrange for pci_segments_iterate() to continue, by merely
recording any possible error in struct segment_iter), and only accumulate
the error code to eventually return. The more devices we manage to
quiesce, the better our chances of rebooting cleanly.

Jan

Re: [PATCH for-4.20 1/2] x86/shutdown: quiesce devices ahead of AP shutdown

[Jan Beulich]

On 29.01.2025 11:13, Jan Beulich wrote:
> On 28.01.2025 17:27, Roger Pau Monne wrote:
>> The current shutdown logic in smp_send_stop() will first disable the APs,
>> and then attempt to disable (some) of the interrupt sources.
>>
>> There are two issues with this approach; the first one being that MSI
>> interrupt sources are not disabled, the second one is the APs are stopped
>> before interrupts are disabled.  On AMD systems this can lead to the
>> triggering of local APIC errors:
>>
>> APIC error on CPU0: 00(08), Receive accept error
>>
>> Such error message can be printed in a loop, thus blocking the system from
>> rebooting.  I assume this loop is created by the error being triggered by
>> the console interrupt, which is further triggered by the ESR reporting
>> write to the console.
>>
>> Intel SDM states:
>>
>> "Receive Accept Error.
>>
>> Set when the local APIC detects that the message it received was not
>> accepted by any APIC on the APIC bus, including itself. Used only on P6
>> family and Pentium processors."
>>
>> So the error shouldn't trigger on any Intel CPU supported by Xen.
>>
>> However AMD doesn't make such claims, and indeed the error is broadcasted
>> to all local APIC when for example an interrupt targets a CPU that's
>> offline.
>>
>> To prevent the error from triggering, move the masking of IO-APIC pins
>> ahead of stopping the APs.  Also introduce a new function that disables
>> MSI and MSI-X on all PCI devices.  Remove the call to fixup_irqs() since
>> there's no point in attempting to move interrupts: all sources will be
>> either masked or disabled.
>>
>> For the NMI crash path also call the newly introduced function, with the
>> hope that disabling MSI and MSI-X will make it easier for the (possible)
>> crash kernel to boot, as it could otherwise receive the same "Receive
>> accept error" upon re-enabling interrupts.
>>
>> Note that this will have the side-effect of preventing further IOMMU
>> interrupts from being delivered, that's expected and at that point in the
>> shutdown process no further interaction with the IOMMU should be relevant.
> 
> This is at most for AMD only. Shouldn't we similarly disable VT-d's
> interrupt(s)? (It's only one right now, as we still don't use the QI
> completion one.) Even for AMD I'm uncertain: It has separate
> hw_irq_controller instances, and its set_iommu_interrupt_handler() is
> custom as well. Will pci_disable_msi_all() really affect it? (Hmm,
> yes, from amd_iommu_msi_enable() it looks like it will.)

Oh, no - not for the x2APIC case. There it's solely iommu->ctrl.int_cap_xt_en
which controls whether the interrupt is enabled, iirc. Btw, one of the two
calls from enable_iommu() to amd_iommu_msi_enable() is actually wrong (commit
d9e49d1afe2e should have moved it instead of adding another call); I'll make
a patch for that.

Jan

Re: [PATCH for-4.20 1/2] x86/shutdown: quiesce devices ahead of AP shutdown

[Roger Pau Monné]

On Wed, Jan 29, 2025 at 11:13:09AM +0100, Jan Beulich wrote:
> On 28.01.2025 17:27, Roger Pau Monne wrote:
> > The current shutdown logic in smp_send_stop() will first disable the APs,
> > and then attempt to disable (some) of the interrupt sources.
> > 
> > There are two issues with this approach; the first one being that MSI
> > interrupt sources are not disabled, the second one is the APs are stopped
> > before interrupts are disabled.  On AMD systems this can lead to the
> > triggering of local APIC errors:
> > 
> > APIC error on CPU0: 00(08), Receive accept error
> > 
> > Such error message can be printed in a loop, thus blocking the system from
> > rebooting.  I assume this loop is created by the error being triggered by
> > the console interrupt, which is further triggered by the ESR reporting
> > write to the console.
> > 
> > Intel SDM states:
> > 
> > "Receive Accept Error.
> > 
> > Set when the local APIC detects that the message it received was not
> > accepted by any APIC on the APIC bus, including itself. Used only on P6
> > family and Pentium processors."
> > 
> > So the error shouldn't trigger on any Intel CPU supported by Xen.
> > 
> > However AMD doesn't make such claims, and indeed the error is broadcasted
> > to all local APIC when for example an interrupt targets a CPU that's
> > offline.
> > 
> > To prevent the error from triggering, move the masking of IO-APIC pins
> > ahead of stopping the APs.  Also introduce a new function that disables
> > MSI and MSI-X on all PCI devices.  Remove the call to fixup_irqs() since
> > there's no point in attempting to move interrupts: all sources will be
> > either masked or disabled.
> > 
> > For the NMI crash path also call the newly introduced function, with the
> > hope that disabling MSI and MSI-X will make it easier for the (possible)
> > crash kernel to boot, as it could otherwise receive the same "Receive
> > accept error" upon re-enabling interrupts.
> > 
> > Note that this will have the side-effect of preventing further IOMMU
> > interrupts from being delivered, that's expected and at that point in the
> > shutdown process no further interaction with the IOMMU should be relevant.
> 
> This is at most for AMD only. Shouldn't we similarly disable VT-d's
> interrupt(s)? (It's only one right now, as we still don't use the QI
> completion one.) Even for AMD I'm uncertain: It has separate
> hw_irq_controller instances, and its set_iommu_interrupt_handler() is
> custom as well. Will pci_disable_msi_all() really affect it? (Hmm,
> yes, from amd_iommu_msi_enable() it looks like it will.)

I was only partly right, the XT interrupt type will still need to be
disabled in a custom way, as there's no associated MSI(-X) capability
in that case.

> 
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -1248,6 +1248,20 @@ void pci_cleanup_msi(struct pci_dev *pdev)
> >      msi_free_irqs(pdev);
> >  }
> >  
> > +static int cf_check disable_msi(struct pci_dev *pdev, void *arg)
> > +{
> > +    msi_set_enable(pdev, 0);
> > +    msix_set_enable(pdev, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +void pci_disable_msi_all(void)
> > +{
> > +    /* Disable MSI and/or MSI-X on all devices. */
> > +    pci_iterate_devices(disable_msi, NULL);
> > +}
> 
> That's going to be all devices we know of. I.e. best effort only. Maybe
> the comment should be adjusted to this effect.

Sure.

> > --- a/xen/arch/x86/smp.c
> > +++ b/xen/arch/x86/smp.c
> > @@ -358,14 +358,15 @@ void smp_send_stop(void)
> >  {
> >      unsigned int cpu = smp_processor_id();
> >  
> > +    local_irq_disable();
> > +    disable_IO_APIC();
> > +    pci_disable_msi_all();
> > +    local_irq_enable();
> > +
> >      if ( num_online_cpus() > 1 )
> >      {
> >          int timeout = 10;
> >  
> > -        local_irq_disable();
> > -        fixup_irqs(cpumask_of(cpu), 0);
> > -        local_irq_enable();
> > -
> >          smp_call_function(stop_this_cpu, NULL, 0);
> >  
> >          /* Wait 10ms for all other CPUs to go offline. */
> > @@ -376,7 +377,6 @@ void smp_send_stop(void)
> >      if ( cpu_online(cpu) )
> >      {
> >          local_irq_disable();
> > -        disable_IO_APIC();
> >          hpet_disable();
> 
> Like IOMMUs, HPET also has custom interrupt management. I think this
> call needs pulling up, too (much like it is also there in
> nmi_shootdown_cpus()).

Indeed, I wasn't taking into account the FSB capability.

> 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1803,6 +1803,38 @@ int iommu_do_pci_domctl(
> >      return ret;
> >  }
> >  
> > +struct segment_iter {
> > +    int (*handler)(struct pci_dev *pdev, void *arg);
> > +    void *arg;
> > +};
> > +
> > +static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
> > +{
> > +    const struct segment_iter *iter = arg;
> > +    struct pci_dev *pdev;
> > +
> > +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > +    {
> > +        int rc = iter->handler(pdev, iter->arg);
> > +
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
> > +                        void *arg)
> > +{
> > +    struct segment_iter iter = {
> > +        .handler = handler,
> > +        .arg = arg,
> > +    };
> > +
> > +    return pci_segments_iterate(iterate_all, &iter);
> > +}
> 
> For the specific purpose during shutdown it may be okay to do all of this
> without locking (but see below) and without preemption checks. Yet then a
> warning will want putting here to indicate that from other environments
> this isn't okay to use as-is.
> 
> This use then also requires that msi{,x}_set_enable() paths never gain
> lock-related assertions.

Good point.  It might be better to just wrap the code in
pci_iterate_devices() with pcidevs_{,un}lock().

> Talking of the lack of locking: Since you invoke the disabling before
> bringing down APs, we're ending up in kind of a chicken and egg problem
> here: Without APs quiesced, there may be operations in progress there
> which conflict with the disabling done here. Hence why so far we brought
> down APs first.

I could implement a synchronized approach with the BSP only doing the
interrupt disabling after the APs are in stop_this_cpu() with
interrupts disabled, but before the calling __stop_this_cpu().

My thinking for doing it the proposed way is that MSI(-X) capability
enable is not toggled by Xen as part of interrupt handling, and hence
there should be no intent to enable the capabilities back after being
disabled by the BSP.

> With this special-purpose use I further wonder whether iterate_all()
> wouldn't better continue despite an error coming back from a callback
> (and also arrange for pci_segments_iterate() to continue, by merely
> recording any possible error in struct segment_iter), and only accumulate
> the error code to eventually return. The more devices we manage to
> quiesce, the better our chances of rebooting cleanly.

Fair enough, will do.

Thanks, Roger.

Re: [PATCH for-4.20 1/2] x86/shutdown: quiesce devices ahead of AP shutdown

[Jan Beulich]

On 04.02.2025 12:46, Roger Pau Monné wrote:
> On Wed, Jan 29, 2025 at 11:13:09AM +0100, Jan Beulich wrote:
>> On 28.01.2025 17:27, Roger Pau Monne wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1803,6 +1803,38 @@ int iommu_do_pci_domctl(
>>>      return ret;
>>>  }
>>>  
>>> +struct segment_iter {
>>> +    int (*handler)(struct pci_dev *pdev, void *arg);
>>> +    void *arg;
>>> +};
>>> +
>>> +static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
>>> +{
>>> +    const struct segment_iter *iter = arg;
>>> +    struct pci_dev *pdev;
>>> +
>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> +    {
>>> +        int rc = iter->handler(pdev, iter->arg);
>>> +
>>> +        if ( rc )
>>> +            return rc;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
>>> +                        void *arg)
>>> +{
>>> +    struct segment_iter iter = {
>>> +        .handler = handler,
>>> +        .arg = arg,
>>> +    };
>>> +
>>> +    return pci_segments_iterate(iterate_all, &iter);
>>> +}
>>
>> For the specific purpose during shutdown it may be okay to do all of this
>> without locking (but see below) and without preemption checks. Yet then a
>> warning will want putting here to indicate that from other environments
>> this isn't okay to use as-is.
>>
>> This use then also requires that msi{,x}_set_enable() paths never gain
>> lock-related assertions.
> 
> Good point.  It might be better to just wrap the code in
> pci_iterate_devices() with pcidevs_{,un}lock().

I'd recommend against doing so. If this was just for ordinary reboot or
shutdown, then yes. But we can crash with this (or any other) lock held.
And this particular lock we hold for sometimes pretty long sequences of
code.

There's anyway the wider issue of how much code we want to involve in
rebooting (or kexec-ing) after a crash: The more we do, the more likely
that we run into a knock-on issue from the earlier crash.

Jan

[PATCH for-4.20 2/2] x86/irq: drop fixup_irqs() parameters

[Roger Pau Monne]

The solely remaining caller always passes the same globally available
parameters.  Drop the parameters and modify fixup_irqs() to use
cpu_online_map in place of the input mask parameter, and always be verbose
in its output printing.

While there remove some of the checks given the single context where
fixup_irqs() is now called, which should always be in the CPU offline path,
after the CPU going offline has been removed from cpu_online_map.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
There's more cleanup that can likely be done here, but it's best if such
cleanup is done after the cpu_mask and old_cpu_mask irq_desc fields are
converted from cpu masks to integers, as logic delivery mode should never
be used for external interrupts now.
---
 xen/arch/x86/include/asm/irq.h |  4 ++--
 xen/arch/x86/irq.c             | 30 +++++++++++++-----------------
 xen/arch/x86/smpboot.c         |  2 +-
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index d3bc76806808..354868ba31ab 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -168,8 +168,8 @@ void free_domain_pirqs(struct domain *d);
 int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq);
 int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
 
-/* Evacuate interrupts assigned to CPUs not present in the input CPU mask. */
-void fixup_irqs(const cpumask_t *mask, bool verbose);
+/* Evacuate interrupts assigned to CPUs not present in the CPU online map. */
+void fixup_irqs(void);
 void fixup_eoi(void);
 
 int  init_irq_data(void);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index e56bacc88d84..ff3ac832f4b9 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2590,17 +2590,21 @@ static int __init cf_check setup_dump_irqs(void)
 }
 __initcall(setup_dump_irqs);
 
-/* Evacuate interrupts assigned to CPUs not present in the input CPU mask. */
-void fixup_irqs(const cpumask_t *mask, bool verbose)
+/* Evacuate interrupts assigned to CPUs not present in the CPU online map. */
+void fixup_irqs(void)
 {
+    const unsigned int cpu = smp_processor_id();
     unsigned int irq;
     static int warned;
     struct irq_desc *desc;
 
+    /* Only to be called from the context of a CPU going offline. */
+    ASSERT(!cpu_online(cpu));
+
     for ( irq = 0; irq < nr_irqs; irq++ )
     {
         bool break_affinity = false, set_affinity = true, check_irr = false;
-        unsigned int vector, cpu = smp_processor_id();
+        unsigned int vector;
         cpumask_t *affinity = this_cpu(scratch_cpumask);
 
         if ( irq == 2 )
@@ -2644,12 +2648,6 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
         }
 
         if ( desc->arch.move_in_progress &&
-             /*
-              * Only attempt to adjust the mask if the current CPU is going
-              * offline, otherwise the whole system is going down and leaving
-              * stale data in the masks is fine.
-              */
-             !cpu_online(cpu) &&
              cpumask_test_cpu(cpu, desc->arch.old_cpu_mask) )
         {
             /*
@@ -2691,16 +2689,17 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
 
         /*
          * Avoid shuffling the interrupt around as long as current target CPUs
-         * are a subset of the input mask.  What fixup_irqs() cares about is
-         * evacuating interrupts from CPUs not in the input mask.
+         * are a subset of the online mask.  What fixup_irqs() cares about is
+         * evacuating interrupts from CPUs not in the online mask.
          */
-        if ( !desc->action || cpumask_subset(desc->arch.cpu_mask, mask) )
+        if ( !desc->action || cpumask_subset(desc->arch.cpu_mask,
+                                             &cpu_online_map) )
         {
             spin_unlock(&desc->lock);
             continue;
         }
 
-        if ( !cpumask_intersects(mask, desc->affinity) )
+        if ( !cpumask_intersects(&cpu_online_map, desc->affinity) )
         {
             break_affinity = true;
             cpumask_setall(affinity);
@@ -2716,7 +2715,7 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
          * the interrupt, signal to check whether there are any pending vectors
          * to be handled in the local APIC after the interrupt has been moved.
          */
-        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
+        if ( cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
             check_irr = true;
 
         if ( desc->handler->set_affinity )
@@ -2743,9 +2742,6 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
 
         spin_unlock(&desc->lock);
 
-        if ( !verbose )
-            continue;
-
         if ( !set_affinity )
             printk("Cannot set affinity for IRQ%u\n", irq);
         else if ( break_affinity )
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 79a79c54c304..891a29fca146 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1282,7 +1282,7 @@ void __cpu_disable(void)
 
     /* It's now safe to remove this processor from the online map */
     cpumask_clear_cpu(cpu, &cpu_online_map);
-    fixup_irqs(&cpu_online_map, 1);
+    fixup_irqs();
     fixup_eoi();
 }
 
-- 
2.46.0

Re: [PATCH for-4.20 2/2] x86/irq: drop fixup_irqs() parameters

[Jan Beulich]

On 28.01.2025 17:27, Roger Pau Monne wrote:
> The solely remaining caller always passes the same globally available
> parameters.  Drop the parameters and modify fixup_irqs() to use
> cpu_online_map in place of the input mask parameter, and always be verbose
> in its output printing.
> 
> While there remove some of the checks given the single context where
> fixup_irqs() is now called, which should always be in the CPU offline path,
> after the CPU going offline has been removed from cpu_online_map.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>