| Message ID | 20231023124635.44266-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/i8259: do not assume interrupts always target CPU0 | expand |
On 23.10.2023 14:46, Roger Pau Monne wrote: > Sporadically we have seen the following during AP bringup on AMD platforms > only: > > microcode: CPU59 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > microcode: CPU60 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 > CPU60: No irq handler for vector 27 (IRQ -2147483648) > microcode: CPU61 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > > This is similar to the issue raised on Linux commit 36e9e1eab777e, where they > observed i8259 (active) vectors getting delivered to CPUs different than 0. > > On AMD or Hygon platforms adjust the target CPU mask of i8259 interrupt > descriptors to contain all possible CPUs, so that APs will reserve the vector > at startup if any legacy IRQ is still delivered through the i8259. Note that > if the IO-APIC takes over those interrupt descriptors the CPU mask will be > reset. > > Spurious i8259 interrupt vectors however (IRQ7 and IRQ15) can be injected even > when all i8259 pins are masked, and hence would need to be handled on all CPUs. > > Do not reserve the PIC spurious vectors on all CPUs, but do check for such > spurious interrupts on all CPUs if the vendor is AMD or Hygon. The first half of this sentence reads as if it was describing a change, but aiui there's no change to existing behavior in this regard anymore. Maybe use something like "continue to reserve PIC vectors on CPU0 only"? > Note that once > the vectors get used by devices detecting PIC spurious interrupts will no > longer be possible, however the device should be able to cope with spurious > interrupt. Such PIC spurious interrupts occurring when the vector is in use by > a local APIC routed source will lead to an extra EOI, which might > unintentionally clear a different vector from ISR. Note this is already the > current behavior, so assume it's infrequent enough to not cause real issues. > > Finally, adjust the printed message to display the CPU where the spurious > interrupt has been received, so it looks like: > > microcode: CPU1 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > cpu1: spurious 8259A interrupt: IRQ7 > microcode: CPU2 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 > > Fixes: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Do not reserved spurious PIC vectors on APs, but still check for spurious > PIC interrupts. > - Reword commit message. > --- > Not sure if the Fixes tag is the most appropriate here, since AFAICT this is a > hardware glitch, but it makes it easier to see to which versions the fix should > be backported, because Xen previous behavior was to reserve all legacy vectors > on all CPUs. I'm inclined to suggest to (informally) invent an Amends: tag. > --- a/xen/arch/x86/i8259.c > +++ b/xen/arch/x86/i8259.c > @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); > > bool bogus_8259A_irq(unsigned int irq) > { > + if ( smp_processor_id() && > + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) > + /* > + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it > + * has been observed that during unknown circumstances spurious PIC > + * interrupts have been delivered to CPUs different than the BSP. > + */ > + return false; > + > return !_mask_and_ack_8259A_irq(irq); > } I don't think this function should be changed; imo the adjustment belongs at the call site. > @@ -349,7 +359,22 @@ void __init init_IRQ(void) > continue; > desc->handler = &i8259A_irq_type; > per_cpu(vector_irq, cpu)[LEGACY_VECTOR(irq)] = irq; > - cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu)); > + > + /* > + * The interrupt affinity logic never targets interrupts to offline > + * CPUs, hence it's safe to use cpumask_all here. > + * > + * Legacy PIC interrupts are only targeted to CPU0, but depending on > + * the platform they can be distributed to any online CPU in hardware. > + * Note this behavior has only been observed on AMD hardware. In order > + * to cope install all active legacy vectors on all CPUs. > + * > + * IO-APIC will change the destination mask if/when taking ownership of > + * the interrupt. > + */ > + cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & > + (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? > + &cpumask_all : cpumask_of(cpu)); Nit: Imo this would better be wrapped as cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? &cpumask_all : cpumask_of(cpu)); or (considering how we often prefer to wrap conditional operators) cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? &cpumask_all : cpumask_of(cpu)); or cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? &cpumask_all : cpumask_of(cpu)); , possibly with the argument spanning three lines additionally parenthesized as a whole. (Of course an amd_like() construct like we have in the emulator would further help readability here, but the way that works it cannot be easily generalized for use outside of the emulator.) Jan
On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote: > On 23.10.2023 14:46, Roger Pau Monne wrote: > > Sporadically we have seen the following during AP bringup on AMD platforms > > only: > > > > microcode: CPU59 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > > microcode: CPU60 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 > > CPU60: No irq handler for vector 27 (IRQ -2147483648) > > microcode: CPU61 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > > > > This is similar to the issue raised on Linux commit 36e9e1eab777e, where they > > observed i8259 (active) vectors getting delivered to CPUs different than 0. > > > > On AMD or Hygon platforms adjust the target CPU mask of i8259 interrupt > > descriptors to contain all possible CPUs, so that APs will reserve the vector > > at startup if any legacy IRQ is still delivered through the i8259. Note that > > if the IO-APIC takes over those interrupt descriptors the CPU mask will be > > reset. > > > > Spurious i8259 interrupt vectors however (IRQ7 and IRQ15) can be injected even > > when all i8259 pins are masked, and hence would need to be handled on all CPUs. > > > > Do not reserve the PIC spurious vectors on all CPUs, but do check for such > > spurious interrupts on all CPUs if the vendor is AMD or Hygon. > > The first half of this sentence reads as if it was describing a change, > but aiui there's no change to existing behavior in this regard anymore. > Maybe use something like "continue to reserve PIC vectors on CPU0 only"? > > > Note that once > > the vectors get used by devices detecting PIC spurious interrupts will no > > longer be possible, however the device should be able to cope with spurious > > interrupt. Such PIC spurious interrupts occurring when the vector is in use by > > a local APIC routed source will lead to an extra EOI, which might > > unintentionally clear a different vector from ISR. Note this is already the > > current behavior, so assume it's infrequent enough to not cause real issues. > > > > Finally, adjust the printed message to display the CPU where the spurious > > interrupt has been received, so it looks like: > > > > microcode: CPU1 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 > > cpu1: spurious 8259A interrupt: IRQ7 > > microcode: CPU2 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 > > > > Fixes: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - Do not reserved spurious PIC vectors on APs, but still check for spurious > > PIC interrupts. > > - Reword commit message. > > --- > > Not sure if the Fixes tag is the most appropriate here, since AFAICT this is a > > hardware glitch, but it makes it easier to see to which versions the fix should > > be backported, because Xen previous behavior was to reserve all legacy vectors > > on all CPUs. > > I'm inclined to suggest to (informally) invent an Amends: tag. > > > --- a/xen/arch/x86/i8259.c > > +++ b/xen/arch/x86/i8259.c > > @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); > > > > bool bogus_8259A_irq(unsigned int irq) > > { > > + if ( smp_processor_id() && > > + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) > > + /* > > + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it > > + * has been observed that during unknown circumstances spurious PIC > > + * interrupts have been delivered to CPUs different than the BSP. > > + */ > > + return false; > > + > > return !_mask_and_ack_8259A_irq(irq); > > } > > I don't think this function should be changed; imo the adjustment belongs > at the call site. It makes the call site much more complex to follow, in fact I was considering pulling the PIC vector range checks into bogus_8259A_irq(). Would that convince you into placing the check here rather than in the caller context? > > @@ -349,7 +359,22 @@ void __init init_IRQ(void) > > continue; > > desc->handler = &i8259A_irq_type; > > per_cpu(vector_irq, cpu)[LEGACY_VECTOR(irq)] = irq; > > - cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu)); > > + > > + /* > > + * The interrupt affinity logic never targets interrupts to offline > > + * CPUs, hence it's safe to use cpumask_all here. > > + * > > + * Legacy PIC interrupts are only targeted to CPU0, but depending on > > + * the platform they can be distributed to any online CPU in hardware. > > + * Note this behavior has only been observed on AMD hardware. In order > > + * to cope install all active legacy vectors on all CPUs. > > + * > > + * IO-APIC will change the destination mask if/when taking ownership of > > + * the interrupt. > > + */ > > + cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & > > + (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? > > + &cpumask_all : cpumask_of(cpu)); > > Nit: Imo this would better be wrapped as > > cpumask_copy(desc->arch.cpu_mask, > boot_cpu_data.x86_vendor & > (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? > &cpumask_all : cpumask_of(cpu)); > > or (considering how we often prefer to wrap conditional operators) > > cpumask_copy(desc->arch.cpu_mask, > boot_cpu_data.x86_vendor & > (X86_VENDOR_AMD | X86_VENDOR_HYGON) > ? &cpumask_all : cpumask_of(cpu)); > > or > > cpumask_copy(desc->arch.cpu_mask, > boot_cpu_data.x86_vendor & > (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? &cpumask_all > : cpumask_of(cpu)); > > , possibly with the argument spanning three lines additionally > parenthesized as a whole. > > (Of course an amd_like() construct like we have in the emulator would > further help readability here, but the way that works it cannot be > easily generalized for use outside of the emulator.) I think the last one could be my preferred indentation, will adjust to that. Thanks, Roger.
On 24.10.2023 12:14, Roger Pau Monné wrote: > On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote: >> On 23.10.2023 14:46, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/i8259.c >>> +++ b/xen/arch/x86/i8259.c >>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); >>> >>> bool bogus_8259A_irq(unsigned int irq) >>> { >>> + if ( smp_processor_id() && >>> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) >>> + /* >>> + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it >>> + * has been observed that during unknown circumstances spurious PIC >>> + * interrupts have been delivered to CPUs different than the BSP. >>> + */ >>> + return false; >>> + >>> return !_mask_and_ack_8259A_irq(irq); >>> } >> >> I don't think this function should be changed; imo the adjustment belongs >> at the call site. > > It makes the call site much more complex to follow, in fact I was > considering pulling the PIC vector range checks into > bogus_8259A_irq(). Would that convince you into placing the check here > rather than in the caller context? Passing a vector and moving the range check into the function is something that may make sense. But I'm afraid the same does not apply to the smp_processor_id() check, unless the function was also renamed to bogus_8259A_vector(). Which in turn doesn't make much sense, to me at least, as the logic would better be in terms of IRQs (which is what the chip deals with primarily), not vectors (which the chip deals with solely during the INTA cycle with the CPU). Jan
On Tue, Oct 24, 2023 at 12:51:08PM +0200, Jan Beulich wrote: > On 24.10.2023 12:14, Roger Pau Monné wrote: > > On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote: > >> On 23.10.2023 14:46, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/i8259.c > >>> +++ b/xen/arch/x86/i8259.c > >>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); > >>> > >>> bool bogus_8259A_irq(unsigned int irq) > >>> { > >>> + if ( smp_processor_id() && > >>> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) > >>> + /* > >>> + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it > >>> + * has been observed that during unknown circumstances spurious PIC > >>> + * interrupts have been delivered to CPUs different than the BSP. > >>> + */ > >>> + return false; > >>> + > >>> return !_mask_and_ack_8259A_irq(irq); > >>> } > >> > >> I don't think this function should be changed; imo the adjustment belongs > >> at the call site. > > > > It makes the call site much more complex to follow, in fact I was > > considering pulling the PIC vector range checks into > > bogus_8259A_irq(). Would that convince you into placing the check here > > rather than in the caller context? > > Passing a vector and moving the range check into the function is something > that may make sense. But I'm afraid the same does not apply to the > smp_processor_id() check, unless the function was also renamed to > bogus_8259A_vector(). Which in turn doesn't make much sense, to me at > least, as the logic would better be in terms of IRQs (which is what the > chip deals with primarily), not vectors (which the chip deals with solely > during the INTA cycle with the CPU). The alternative is to use: if ( !(vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR && (!smp_processor_id() || /* * For AMD/Hygon do spurious PIC interrupt * detection on all CPUs, as it has been observed * that during unknown circumstances spurious PIC * interrupts have been delivered to CPUs * different than the BSP. */ (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))) && bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) ) { Which I find too complex to read, and prone to mistakes by future modifications. What is your reasoning for wanting the smp_processor_id() check in the caller rather than bogus_8259A_irq()? It does seem fine to me to do such check in bogus_8259A_irq(), as whether the IRQ is bogus also depends on whether it fired on the BSP or any of the APs. Thanks, Roger.
On 24.10.2023 13:36, Roger Pau Monné wrote: > On Tue, Oct 24, 2023 at 12:51:08PM +0200, Jan Beulich wrote: >> On 24.10.2023 12:14, Roger Pau Monné wrote: >>> On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote: >>>> On 23.10.2023 14:46, Roger Pau Monne wrote: >>>>> --- a/xen/arch/x86/i8259.c >>>>> +++ b/xen/arch/x86/i8259.c >>>>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); >>>>> >>>>> bool bogus_8259A_irq(unsigned int irq) >>>>> { >>>>> + if ( smp_processor_id() && >>>>> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) >>>>> + /* >>>>> + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it >>>>> + * has been observed that during unknown circumstances spurious PIC >>>>> + * interrupts have been delivered to CPUs different than the BSP. >>>>> + */ >>>>> + return false; >>>>> + >>>>> return !_mask_and_ack_8259A_irq(irq); >>>>> } >>>> >>>> I don't think this function should be changed; imo the adjustment belongs >>>> at the call site. >>> >>> It makes the call site much more complex to follow, in fact I was >>> considering pulling the PIC vector range checks into >>> bogus_8259A_irq(). Would that convince you into placing the check here >>> rather than in the caller context? >> >> Passing a vector and moving the range check into the function is something >> that may make sense. But I'm afraid the same does not apply to the >> smp_processor_id() check, unless the function was also renamed to >> bogus_8259A_vector(). Which in turn doesn't make much sense, to me at >> least, as the logic would better be in terms of IRQs (which is what the >> chip deals with primarily), not vectors (which the chip deals with solely >> during the INTA cycle with the CPU). > > The alternative is to use: > > if ( !(vector >= FIRST_LEGACY_VECTOR && > vector <= LAST_LEGACY_VECTOR && > (!smp_processor_id() || > /* > * For AMD/Hygon do spurious PIC interrupt > * detection on all CPUs, as it has been observed > * that during unknown circumstances spurious PIC > * interrupts have been delivered to CPUs > * different than the BSP. > */ > (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | > X86_VENDOR_HYGON))) && > bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) ) > { > > Which I find too complex to read, and prone to mistakes by future > modifications. From my pov the main badness with this is pre-existing: The wrapping of a complex expression in !(...). The replacement of the prior plain smp_processor_id() isn't much of a problem imo. > What is your reasoning for wanting the smp_processor_id() check in > the caller rather than bogus_8259A_irq()? It does seem fine to me to > do such check in bogus_8259A_irq(), as whether the IRQ is bogus also > depends on whether it fired on the BSP or any of the APs. bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it should solely deal with 8259A aspects. Jan
On 24.10.2023 14:06, Jan Beulich wrote: > On 24.10.2023 13:36, Roger Pau Monné wrote: >> What is your reasoning for wanting the smp_processor_id() check in >> the caller rather than bogus_8259A_irq()? It does seem fine to me to >> do such check in bogus_8259A_irq(), as whether the IRQ is bogus also >> depends on whether it fired on the BSP or any of the APs. > > bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it > should solely deal with 8259A aspects. Or to put it differently: The function is supposed to tell whether an IRQ is bogus from the pov of the PIC. The caller decides under what conditions to actually invoke this checking. Jan
On Tue, Oct 24, 2023 at 02:08:42PM +0200, Jan Beulich wrote: > On 24.10.2023 14:06, Jan Beulich wrote: > > On 24.10.2023 13:36, Roger Pau Monné wrote: > >> What is your reasoning for wanting the smp_processor_id() check in > >> the caller rather than bogus_8259A_irq()? It does seem fine to me to > >> do such check in bogus_8259A_irq(), as whether the IRQ is bogus also > >> depends on whether it fired on the BSP or any of the APs. > > > > bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it > > should solely deal with 8259A aspects. > > Or to put it differently: The function is supposed to tell whether an > IRQ is bogus from the pov of the PIC. The caller decides under what > conditions to actually invoke this checking. I understand that the PIC itself is agnostic as to which the CPU the irq (vector) has been injected, but the added CPU vendor checks are there to deal with possibly a bogus PIC implementation, and hence doesn't feel that off place IMO. Anyway, will adjust as requested, albeit I think it hampers readability and that's more valuable than whether the check is contextually better fit in do_IRQ() or bogus_8259A_irq(). Thanks, Roger.
diff --git a/xen/arch/x86/i8259.c b/xen/arch/x86/i8259.c index ed9f55abe51e..0935cdf07b65 100644 --- a/xen/arch/x86/i8259.c +++ b/xen/arch/x86/i8259.c @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq); bool bogus_8259A_irq(unsigned int irq) { + if ( smp_processor_id() && + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) + /* + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it + * has been observed that during unknown circumstances spurious PIC + * interrupts have been delivered to CPUs different than the BSP. + */ + return false; + return !_mask_and_ack_8259A_irq(irq); } @@ -222,7 +231,8 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq) is_real_irq = false; /* Report spurious IRQ, once per IRQ line. */ if (!(spurious_irq_mask & irqmask)) { - printk("spurious 8259A interrupt: IRQ%d.\n", irq); + printk("cpu%u: spurious 8259A interrupt: IRQ%u\n", + smp_processor_id(), irq); spurious_irq_mask |= irqmask; } /* @@ -349,7 +359,22 @@ void __init init_IRQ(void) continue; desc->handler = &i8259A_irq_type; per_cpu(vector_irq, cpu)[LEGACY_VECTOR(irq)] = irq; - cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu)); + + /* + * The interrupt affinity logic never targets interrupts to offline + * CPUs, hence it's safe to use cpumask_all here. + * + * Legacy PIC interrupts are only targeted to CPU0, but depending on + * the platform they can be distributed to any online CPU in hardware. + * Note this behavior has only been observed on AMD hardware. In order + * to cope install all active legacy vectors on all CPUs. + * + * IO-APIC will change the destination mask if/when taking ownership of + * the interrupt. + */ + cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor & + (X86_VENDOR_AMD | X86_VENDOR_HYGON) ? + &cpumask_all : cpumask_of(cpu)); desc->arch.vector = LEGACY_VECTOR(irq); } diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index f42ad539dcd5..a2f9374f5deb 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1920,7 +1920,6 @@ void do_IRQ(struct cpu_user_regs *regs) kind = ""; if ( !(vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR && - !smp_processor_id() && bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) ) { printk("CPU%u: No irq handler for vector %02x (IRQ %d%s)\n",
Sporadically we have seen the following during AP bringup on AMD platforms only: microcode: CPU59 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 microcode: CPU60 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 CPU60: No irq handler for vector 27 (IRQ -2147483648) microcode: CPU61 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 This is similar to the issue raised on Linux commit 36e9e1eab777e, where they observed i8259 (active) vectors getting delivered to CPUs different than 0. On AMD or Hygon platforms adjust the target CPU mask of i8259 interrupt descriptors to contain all possible CPUs, so that APs will reserve the vector at startup if any legacy IRQ is still delivered through the i8259. Note that if the IO-APIC takes over those interrupt descriptors the CPU mask will be reset. Spurious i8259 interrupt vectors however (IRQ7 and IRQ15) can be injected even when all i8259 pins are masked, and hence would need to be handled on all CPUs. Do not reserve the PIC spurious vectors on all CPUs, but do check for such spurious interrupts on all CPUs if the vendor is AMD or Hygon. Note that once the vectors get used by devices detecting PIC spurious interrupts will no longer be possible, however the device should be able to cope with spurious interrupt. Such PIC spurious interrupts occurring when the vector is in use by a local APIC routed source will lead to an extra EOI, which might unintentionally clear a different vector from ISR. Note this is already the current behavior, so assume it's infrequent enough to not cause real issues. Finally, adjust the printed message to display the CPU where the spurious interrupt has been received, so it looks like: microcode: CPU1 updated from revision 0x830107a to 0x830107a, date = 2023-05-17 cpu1: spurious 8259A interrupt: IRQ7 microcode: CPU2 updated from revision 0x830104d to 0x830107a, date = 2023-05-17 Fixes: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Do not reserved spurious PIC vectors on APs, but still check for spurious PIC interrupts. - Reword commit message. --- Not sure if the Fixes tag is the most appropriate here, since AFAICT this is a hardware glitch, but it makes it easier to see to which versions the fix should be backported, because Xen previous behavior was to reserve all legacy vectors on all CPUs. --- xen/arch/x86/i8259.c | 29 +++++++++++++++++++++++++++-- xen/arch/x86/irq.c | 1 - 2 files changed, 27 insertions(+), 3 deletions(-)