[PATCH v2 0/3] xen/arm: Fix eSPI IRQ handling

[PATCH v2 0/3] xen/arm: Fix eSPI IRQ handling

[Mykola Kvach]

This series fixes sparse eSPI INTID handling and checks errors returned by
irq_set_type().

Xen has IRQ descriptors for INTIDs below NR_IRQS and for eSPIs starting at
4096. It has no descriptors for INTIDs 1024 through 4095. Patch 1 checks
INTIDs in setup_irq() and irq_set_spi_type() before these functions look up
a descriptor. irq_set_spi_type() checks descriptor ranges because the GIC
line counts are not known yet. setup_irq() uses the line counts once they
are available.

Patch 2 fixes the vGIC allocation bitmap. Reserving an eSPI used a compact
bitmap index, but freeing it used the raw virtual INTID. This could write
past the bitmap and leave the eSPI reserved.

Patch 3 is new in v2. It checks errors from irq_set_type() in the GTDT,
MADT, SPCR, and FF-A paths. GTDT and MADT could keep a rejected timer or
maintenance INTID and later use it in a direct descriptor lookup. This
patch also fixes MISRA C Rule 17.7 violations.

Tested with:
- Arm64 debug builds with CONFIG_ACPI=y and CONFIG_FFA=y, both with and
  without CONFIG_GICV3_ESPI
- FVP Device Tree boot with 64 eSPIs; Linux dom0 started
- QEMU virt UEFI/ACPI boot to a dom0 initramfs shell; this covered the GTDT,
  GICv3 MADT, and PL011 SPCR paths

Changes in v2:
- Check descriptor ranges in irq_set_spi_type() and implemented GIC lines
  in setup_irq().
- Keep the is_espi() debug check when CONFIG_GICV3_ESPI is disabled.
- Remove a redundant CONFIG_GICV3_ESPI guard from the vGIC code.
- Add patch 3 to check irq_set_type() errors in the GTDT, MADT, SPCR, and
  FF-A paths.
- Target master instead of the 4.22 release.

v1: https://patchew.org/Xen/cover.1783671887.git.mykola._5Fkvach@epam.com/

Mykola Kvach (3):
  xen/arm: validate IRQs before descriptor lookup
  xen/arm: vgic: free eSPIs using the bitmap index
  xen/arm: handle irq_set_type() failures

 xen/arch/arm/gic-v2.c        |  8 ++++++--
 xen/arch/arm/gic-v3.c        |  8 ++++++--
 xen/arch/arm/irq.c           | 29 ++++++++++++++++++++++++-----
 xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
 xen/arch/arm/time.c          | 18 ++++++++++++++----
 xen/arch/arm/vgic.c          | 25 ++++++++++++++-----------
 xen/drivers/char/ns16550.c   |  5 ++++-
 xen/drivers/char/pl011.c     |  4 +++-
 8 files changed, 81 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] xen/arm: validate IRQs before descriptor lookup

[Mykola Kvach]

GICv3 eSPI support makes nr_irqs span the architectural INTID namespace
through ESPI_MAX_INTID, but descriptor storage is sparse. local_irq_desc[]
and irq_desc[] cover INTIDs below NR_IRQS, while espi_desc[] covers eSPIs.
INTIDs 1024 through 4095 have no backing descriptors.

Validation based only on nr_irqs accepts an INTID in this gap.
__irq_to_desc() then indexes beyond irq_desc[], and callers may lock or
update unrelated Xen memory.

Reject INTIDs that the GIC reports as unimplemented in setup_irq() before
looking up a descriptor. irq_set_spi_type() can run before the implemented
GIC line counts are available, so validate descriptor-backed ranges there
before looking up a descriptor.

Call is_espi() unconditionally in __irq_to_desc() and provide an
espi_to_desc() stub when eSPI support is disabled. This preserves the
is_espi() debug check for eSPI-range INTIDs when support is disabled.

Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Changes in v2:
- Validate descriptor-backed ranges in irq_set_spi_type().
- Validate implemented GIC lines in setup_irq().
- Preserve is_espi() validation with CONFIG_GICV3_ESPI disabled.
---
 xen/arch/arm/irq.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 73e58a5108..0f5d3496bf 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -23,6 +23,12 @@ const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
                                         (ESPI_MAX_INTID + 1) :
                                         NR_IRQS;
 
+static bool irq_has_desc(unsigned int irq)
+{
+    return irq < NR_IRQS ||
+           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
+}
+
 static unsigned int local_irqs_type[NR_LOCAL_IRQS];
 static DEFINE_SPINLOCK(local_irqs_type_lock);
 
@@ -77,6 +83,12 @@ static int __init init_espi_data(void)
 }
 #else
 
+static struct irq_desc *espi_to_desc(unsigned int irq)
+{
+    ASSERT_UNREACHABLE();
+    return NULL;
+}
+
 static int __init init_espi_data(void)
 {
     return 0;
@@ -90,10 +102,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
     if ( irq < NR_LOCAL_IRQS )
         return &this_cpu(local_irq_desc)[irq];
 
-#ifdef CONFIG_GICV3_ESPI
     if ( is_espi(irq) )
         return espi_to_desc(irq);
-#endif
 
     return &irq_desc[irq-NR_LOCAL_IRQS];
 }
@@ -416,6 +426,9 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
     struct irq_desc *desc;
     bool disabled;
 
+    if ( !gic_is_valid_line(irq) )
+        return -EINVAL;
+
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock, flags);
@@ -647,13 +660,19 @@ static bool irq_validate_new_type(unsigned int curr, unsigned int new)
 int irq_set_spi_type(unsigned int spi, unsigned int type)
 {
     unsigned long flags;
-    struct irq_desc *desc = irq_to_desc(spi);
+    struct irq_desc *desc;
     int ret = -EBUSY;
 
-    /* This function should not be used for other than SPIs */
-    if ( spi < NR_LOCAL_IRQS )
+    /*
+     * The implemented GIC line counts are not available when early
+     * callers configure IRQ types. Check descriptor storage here; setup_irq()
+     * validates the implemented line before the interrupt is used.
+     */
+    if ( spi < NR_LOCAL_IRQS || !irq_has_desc(spi) )
         return -EINVAL;
 
+    desc = irq_to_desc(spi);
+
     spin_lock_irqsave(&desc->lock, flags);
 
     if ( !irq_validate_new_type(desc->arch.type, type) )
-- 
2.43.0

Re: [PATCH v2 1/3] xen/arm: validate IRQs before descriptor lookup

[Orzel, Michal]

On 10-Aug-26 20:38, Mykola Kvach wrote:
> GICv3 eSPI support makes nr_irqs span the architectural INTID namespace
> through ESPI_MAX_INTID, but descriptor storage is sparse. local_irq_desc[]
> and irq_desc[] cover INTIDs below NR_IRQS, while espi_desc[] covers eSPIs.
> INTIDs 1024 through 4095 have no backing descriptors.
> 
> Validation based only on nr_irqs accepts an INTID in this gap.
> __irq_to_desc() then indexes beyond irq_desc[], and callers may lock or
> update unrelated Xen memory.
> 
> Reject INTIDs that the GIC reports as unimplemented in setup_irq() before
> looking up a descriptor. irq_set_spi_type() can run before the implemented
> GIC line counts are available, so validate descriptor-backed ranges there
> before looking up a descriptor.
> 
> Call is_espi() unconditionally in __irq_to_desc() and provide an
> espi_to_desc() stub when eSPI support is disabled. This preserves the
> is_espi() debug check for eSPI-range INTIDs when support is disabled.
> 
> Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> Changes in v2:
> - Validate descriptor-backed ranges in irq_set_spi_type().
> - Validate implemented GIC lines in setup_irq().
> - Preserve is_espi() validation with CONFIG_GICV3_ESPI disabled.
> ---
>  xen/arch/arm/irq.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 73e58a5108..0f5d3496bf 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -23,6 +23,12 @@ const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
>                                          (ESPI_MAX_INTID + 1) :
>                                          NR_IRQS;
>  
> +static bool irq_has_desc(unsigned int irq)
> +{
> +    return irq < NR_IRQS ||
> +           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
This IS_ENABLED reads redundant because is_espi() contains #ifdef
CONFIG_GICV3_ESPI inside. AFAICT you added it here to prevent the !ESPI build
from reaching ASSERT inside is_espi() when the irq is in ESPI range. I don't
like the ASSERT inside is_espi(). I think it does not make much sense in a
helper that should really just tell us whether the IRQ is in ESPI range or not.
It should be up to the caller to decide what to do based on whether ESPI is
compiled in or not. I think this cleanup would be best to be done first. If you
don't want to do that, at least document this in the commit msg because others
may be tempted to drop this IS_ENABLED.

> +}
> +
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>  
> @@ -77,6 +83,12 @@ static int __init init_espi_data(void)
>  }
>  #else
>  
> +static struct irq_desc *espi_to_desc(unsigned int irq)
> +{
> +    ASSERT_UNREACHABLE();
> +    return NULL;
> +}
> +
>  static int __init init_espi_data(void)
>  {
>      return 0;
> @@ -90,10 +102,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
>      if ( irq < NR_LOCAL_IRQS )
>          return &this_cpu(local_irq_desc)[irq];
>  
> -#ifdef CONFIG_GICV3_ESPI
>      if ( is_espi(irq) )
>          return espi_to_desc(irq);
> -#endif
>  
>      return &irq_desc[irq-NR_LOCAL_IRQS];
Nothing here covers 1024..4095. I think we should add at least:
ASSERT(irq < NR_IRQS) like we discussed some time ago.

>  }
> @@ -416,6 +426,9 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
>      struct irq_desc *desc;
>      bool disabled;
>  
> +    if ( !gic_is_valid_line(irq) )
> +        return -EINVAL;
> +
>      desc = irq_to_desc(irq);
>  
>      spin_lock_irqsave(&desc->lock, flags);
> @@ -647,13 +660,19 @@ static bool irq_validate_new_type(unsigned int curr, unsigned int new)
>  int irq_set_spi_type(unsigned int spi, unsigned int type)
>  {
>      unsigned long flags;
> -    struct irq_desc *desc = irq_to_desc(spi);
> +    struct irq_desc *desc;
>      int ret = -EBUSY;
>  
> -    /* This function should not be used for other than SPIs */
This is an important line that you should keep.

> -    if ( spi < NR_LOCAL_IRQS )
> +    /*
> +     * The implemented GIC line counts are not available when early
> +     * callers configure IRQ types. Check descriptor storage here; setup_irq()
> +     * validates the implemented line before the interrupt is used.
> +     */
> +    if ( spi < NR_LOCAL_IRQS || !irq_has_desc(spi) )
>          return -EINVAL;
>  
> +    desc = irq_to_desc(spi);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( !irq_validate_new_type(desc->arch.type, type) )

~Michal

Re: [PATCH v2 1/3] xen/arm: validate IRQs before descriptor lookup

[Mykola Kvach]

Hi Michal,

Thank you for the review.

On Tue, Aug 11, 2026 at 10:51:47AM +0200, Orzel, Michal wrote:
> 
> 
> On 10-Aug-26 20:38, Mykola Kvach wrote:
> > GICv3 eSPI support makes nr_irqs span the architectural INTID namespace
> > through ESPI_MAX_INTID, but descriptor storage is sparse. local_irq_desc[]
> > and irq_desc[] cover INTIDs below NR_IRQS, while espi_desc[] covers eSPIs.
> > INTIDs 1024 through 4095 have no backing descriptors.
> > 
> > Validation based only on nr_irqs accepts an INTID in this gap.
> > __irq_to_desc() then indexes beyond irq_desc[], and callers may lock or
> > update unrelated Xen memory.
> > 
> > Reject INTIDs that the GIC reports as unimplemented in setup_irq() before
> > looking up a descriptor. irq_set_spi_type() can run before the implemented
> > GIC line counts are available, so validate descriptor-backed ranges there
> > before looking up a descriptor.
> > 
> > Call is_espi() unconditionally in __irq_to_desc() and provide an
> > espi_to_desc() stub when eSPI support is disabled. This preserves the
> > is_espi() debug check for eSPI-range INTIDs when support is disabled.
> > 
> > Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > Changes in v2:
> > - Validate descriptor-backed ranges in irq_set_spi_type().
> > - Validate implemented GIC lines in setup_irq().
> > - Preserve is_espi() validation with CONFIG_GICV3_ESPI disabled.
> > ---
> >  xen/arch/arm/irq.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 73e58a5108..0f5d3496bf 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -23,6 +23,12 @@ const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
> >                                          (ESPI_MAX_INTID + 1) :
> >                                          NR_IRQS;
> >  
> > +static bool irq_has_desc(unsigned int irq)
> > +{
> > +    return irq < NR_IRQS ||
> > +           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
> This IS_ENABLED reads redundant because is_espi() contains #ifdef
> CONFIG_GICV3_ESPI inside. AFAICT you added it here to prevent the !ESPI build
> from reaching ASSERT inside is_espi() when the irq is in ESPI range. I don't
> like the ASSERT inside is_espi(). I think it does not make much sense in a
> helper that should really just tell us whether the IRQ is in ESPI range or not.
> It should be up to the caller to decide what to do based on whether ESPI is
> compiled in or not. I think this cleanup would be best to be done first. If you
> don't want to do that, at least document this in the commit msg because others
> may be tempted to drop this IS_ENABLED.

Ack. I’ll add a preparatory cleanup patch making is_espi() a pure
range predicate and keep the configuration handling at the call sites.

> 
> > +}
> > +
> >  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> >  static DEFINE_SPINLOCK(local_irqs_type_lock);
> >  
> > @@ -77,6 +83,12 @@ static int __init init_espi_data(void)
> >  }
> >  #else
> >  
> > +static struct irq_desc *espi_to_desc(unsigned int irq)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return NULL;
> > +}
> > +
> >  static int __init init_espi_data(void)
> >  {
> >      return 0;
> > @@ -90,10 +102,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
> >      if ( irq < NR_LOCAL_IRQS )
> >          return &this_cpu(local_irq_desc)[irq];
> >  
> > -#ifdef CONFIG_GICV3_ESPI
> >      if ( is_espi(irq) )
> >          return espi_to_desc(irq);
> > -#endif
> >  
> >      return &irq_desc[irq-NR_LOCAL_IRQS];
> Nothing here covers 1024..4095. I think we should add at least:
> ASSERT(irq < NR_IRQS) like we discussed some time ago.

Ack, I’ll restore the assertion before indexing irq_desc[].

> 
> >  }
> > @@ -416,6 +426,9 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
> >      struct irq_desc *desc;
> >      bool disabled;
> >  
> > +    if ( !gic_is_valid_line(irq) )
> > +        return -EINVAL;
> > +
> >      desc = irq_to_desc(irq);
> >  
> >      spin_lock_irqsave(&desc->lock, flags);
> > @@ -647,13 +660,19 @@ static bool irq_validate_new_type(unsigned int curr, unsigned int new)
> >  int irq_set_spi_type(unsigned int spi, unsigned int type)
> >  {
> >      unsigned long flags;
> > -    struct irq_desc *desc = irq_to_desc(spi);
> > +    struct irq_desc *desc;
> >      int ret = -EBUSY;
> >  
> > -    /* This function should not be used for other than SPIs */
> This is an important line that you should keep.

Ack.

Best regards,
Mykola

[PATCH v2 2/3] xen/arm: vgic: free eSPIs using the bitmap index

[Mykola Kvach]

The allocated_irqs bitmap in the existing vGIC implementation stores eSPI
allocation bits immediately after the regular vIRQ bits.
vgic_reserve_virq() converts an eSPI INTID to this compressed bitmap index,
but vgic_free_virq() used the raw INTID.

Freeing INTID 4096 therefore clears bit 4096 instead of the first eSPI bit.
This writes beyond allocated_irqs and leaves the intended eSPI bit set.
Valid eSPIs reach this path during DOMCTL bind failure cleanup and unbind,
and during vPL011 teardown.

Add virq_to_idx(), the inverse of idx_to_virq(), and use it when reserving
and freeing vIRQs. Validate a vIRQ before clearing its allocation bit.

Fixes: bdde400c6e1b ("xen/arm: vgic: add resource management for extended SPIs")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Changes in v2:
- Call is_espi() without a configuration guard.
---
 xen/arch/arm/vgic.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e5aca17dcb..c095866709 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -33,6 +33,14 @@ static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
     return idx;
 }
 
+static inline unsigned int virq_to_idx(struct domain *d, unsigned int virq)
+{
+    if ( is_espi(virq) )
+        return espi_intid_to_idx(virq) + vgic_num_irqs(d);
+
+    return virq;
+}
+
 bool vgic_is_valid_line(struct domain *d, unsigned int virq)
 {
 #ifdef CONFIG_GICV3_ESPI
@@ -848,19 +856,11 @@ bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
 
 bool vgic_reserve_virq(struct domain *d, unsigned int virq)
 {
-    unsigned int idx = virq;
-
     if ( !vgic_is_valid_line(d, virq) )
         return false;
 
-    if ( is_espi(virq) )
-    {
-        unsigned int num_regular_irqs = vgic_num_irqs(d);
-
-        idx = espi_intid_to_idx(virq) + num_regular_irqs;
-    }
-
-    return !test_and_set_bit(idx, d->arch.vgic.allocated_irqs);
+    return !test_and_set_bit(virq_to_idx(d, virq),
+                             d->arch.vgic.allocated_irqs);
 }
 
 int vgic_allocate_virq(struct domain *d, bool spi)
@@ -897,7 +897,10 @@ int vgic_allocate_virq(struct domain *d, bool spi)
 
 void vgic_free_virq(struct domain *d, unsigned int virq)
 {
-    clear_bit(virq, d->arch.vgic.allocated_irqs);
+    if ( !vgic_is_valid_line(d, virq) )
+        return;
+
+    clear_bit(virq_to_idx(d, virq), d->arch.vgic.allocated_irqs);
 }
 
 unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
-- 
2.43.0

[PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Mykola Kvach]

Several Arm firmware initialization paths discard irq_set_type()'s return
value, violating MISRA C Rule 17.7. If trigger configuration fails,
initialization continues with an IRQ that was not configured as requested.

Check the return value in the GTDT, MADT, SPCR, and FF-A paths. Store
timer INTIDs only after successful trigger configuration, make GTDT
parsing failure fatal, and stop UART or notification setup when trigger
configuration fails.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Changes in v2:
- new patch.
---
 xen/arch/arm/gic-v2.c        |  8 ++++++--
 xen/arch/arm/gic-v3.c        |  8 ++++++--
 xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
 xen/arch/arm/time.c          | 18 ++++++++++++++----
 xen/drivers/char/ns16550.c   |  5 ++++-
 xen/drivers/char/pl011.c     |  4 +++-
 6 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 43a379fdda..b8dcbb0bb4 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1157,6 +1157,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
 {
     static int cpu_base_assigned = 0;
+    int rc;
     struct acpi_madt_generic_interrupt *processor =
                container_of(header, struct acpi_madt_generic_interrupt, header);
 
@@ -1173,9 +1174,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
         gicv2_info.maintenance_irq = processor->vgic_interrupt;
 
         if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
-            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
+            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
         else
-            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
+            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
+
+        if ( rc )
+            return rc;
 
         cpu_base_assigned = 1;
     }
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index acdac22953..d92d0b9b3c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1734,6 +1734,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
 {
     static int cpu_base_assigned = 0;
+    int rc;
     struct acpi_madt_generic_interrupt *processor =
                container_of(header, struct acpi_madt_generic_interrupt, header);
 
@@ -1748,9 +1749,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
         gicv3_info.maintenance_irq = processor->vgic_interrupt;
 
         if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
-            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
+            rc = irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
         else
-            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
+            rc = irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
+
+        if ( rc )
+            return rc;
 
         cpu_base_assigned = 1;
     }
diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
index 186e726412..d08d0a3366 100644
--- a/xen/arch/arm/tee/ffa_notif.c
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -407,7 +407,16 @@ void ffa_notif_init(void)
         irq = resp.a2;
         notif_sri_irq = irq;
         if ( irq >= NR_GIC_SGI )
-            irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
+        {
+            ret = irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
+            if ( ret )
+            {
+                printk(XENLOG_ERR
+                       "ffa: irq_set_type irq %u failed: error %d\n",
+                       irq, ret);
+                return;
+            }
+        }
         ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
         if ( ret )
         {
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 6955b2788f..39b5eabe7c 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -60,20 +60,27 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *header)
 {
     u32 irq_type;
     struct acpi_table_gtdt *gtdt;
+    int rc;
 
     gtdt = container_of(header, struct acpi_table_gtdt, header);
 
     /* Initialize all the generic timer IRQ variable from GTDT table */
     irq_type = acpi_get_timer_irq_type(gtdt->non_secure_el1_flags);
-    irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
+    rc = irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
+    if ( rc )
+        return rc;
     timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
 
     irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
-    irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
+    rc = irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
+    if ( rc )
+        return rc;
     timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
 
     irq_type = acpi_get_timer_irq_type(gtdt->non_secure_el2_flags);
-    irq_set_type(gtdt->non_secure_el2_interrupt, irq_type);
+    rc = irq_set_type(gtdt->non_secure_el2_interrupt, irq_type);
+    if ( rc )
+        return rc;
     timer_irq[TIMER_HYP_PPI] = gtdt->non_secure_el2_interrupt;
 
     return 0;
@@ -81,7 +88,10 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *header)
 
 static void __init preinit_acpi_xen_time(void)
 {
-    acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
+    int rc = acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
+
+    if ( rc )
+        panic("Timer: Failed to configure interrupts from GTDT: %d\n", rc);
 }
 #else
 static void __init preinit_acpi_xen_time(void) { }
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 120ac09d23..4bfdcfebd7 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1928,6 +1928,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
     struct acpi_table_header *table;
     struct acpi_table_spcr *spcr;
     acpi_status status;
+    int rc;
     /*
      * Same as the DT part.
      * Only support one UART on ARM which happen to be ns16550_com[0].
@@ -1976,7 +1977,9 @@ static int __init ns16550_acpi_uart_init(const void *data)
     uart->reg_width = spcr->serial_port.access_width;
 
     /* The trigger/polarity information is not available in spcr. */
-    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    if ( rc )
+        return rc;
     uart->irq = spcr->interrupt;
 
     uart->vuart.base_addr = uart->io_base;
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index a336241033..97c53c11e0 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -363,7 +363,9 @@ static int __init pl011_acpi_uart_init(const void *data)
             spcr->interface_type == ACPI_DBG2_SBSA_32);
 
     /* trigger/polarity information is not available in spcr */
-    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    res = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    if ( res )
+        return res;
 
     /* TODO - mmio32 proper handling (for now set to true) */
     res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
-- 
2.43.0

Re: [PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Andrew Cooper]

On 10/08/2026 7:38 pm, Mykola Kvach wrote:
> Several Arm firmware initialization paths discard irq_set_type()'s return
> value, violating MISRA C Rule 17.7. If trigger configuration fails,
> initialization continues with an IRQ that was not configured as requested.
>
> Check the return value in the GTDT, MADT, SPCR, and FF-A paths. Store
> timer INTIDs only after successful trigger configuration, make GTDT
> parsing failure fatal, and stop UART or notification setup when trigger
> configuration fails.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> Changes in v2:
> - new patch.
> ---
>  xen/arch/arm/gic-v2.c        |  8 ++++++--
>  xen/arch/arm/gic-v3.c        |  8 ++++++--
>  xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
>  xen/arch/arm/time.c          | 18 ++++++++++++++----
>  xen/drivers/char/ns16550.c   |  5 ++++-
>  xen/drivers/char/pl011.c     |  4 +++-
>  6 files changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 43a379fdda..b8dcbb0bb4 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1157,6 +1157,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                          const unsigned long end)
>  {
>      static int cpu_base_assigned = 0;
> +    int rc;
>      struct acpi_madt_generic_interrupt *processor =
>                 container_of(header, struct acpi_madt_generic_interrupt, header);
>  
> @@ -1173,9 +1174,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>          gicv2_info.maintenance_irq = processor->vgic_interrupt;
>  
>          if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> +            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
>          else
> -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> +            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);

I know it was pre-existing, but this is an overly verbose way of writing:

rc = irq_set_type(gicv2_info.maintenance_irq,
          (processor->flags & ACPI_MADT_VGIC_IRQ_MODE)
          ? IRQ_TYPE_EDGE_BOTH
          : IRQ_TYPE_LEVEL_MASK);

I expect the optimiser can transform behind the scenes, but it's better
to make the C simpler for humans too.

~Andrew

Re: [PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Mykola Kvach]

Hi Andrew,

Thank you for the review.

On Tue, Aug 11, 2026 at 02:01:17PM +0100, Andrew Cooper wrote:
> On 10/08/2026 7:38 pm, Mykola Kvach wrote:
> > Several Arm firmware initialization paths discard irq_set_type()'s return
> > value, violating MISRA C Rule 17.7. If trigger configuration fails,
> > initialization continues with an IRQ that was not configured as requested.
> >
> > Check the return value in the GTDT, MADT, SPCR, and FF-A paths. Store
> > timer INTIDs only after successful trigger configuration, make GTDT
> > parsing failure fatal, and stop UART or notification setup when trigger
> > configuration fails.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > Changes in v2:
> > - new patch.
> > ---
> >  xen/arch/arm/gic-v2.c        |  8 ++++++--
> >  xen/arch/arm/gic-v3.c        |  8 ++++++--
> >  xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
> >  xen/arch/arm/time.c          | 18 ++++++++++++++----
> >  xen/drivers/char/ns16550.c   |  5 ++++-
> >  xen/drivers/char/pl011.c     |  4 +++-
> >  6 files changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 43a379fdda..b8dcbb0bb4 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1157,6 +1157,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> >                          const unsigned long end)
> >  {
> >      static int cpu_base_assigned = 0;
> > +    int rc;
> >      struct acpi_madt_generic_interrupt *processor =
> >                 container_of(header, struct acpi_madt_generic_interrupt, header);
> >  
> > @@ -1173,9 +1174,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> >          gicv2_info.maintenance_irq = processor->vgic_interrupt;
> >  
> >          if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> > -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> > +            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> >          else
> > -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> > +            rc = irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> 
> I know it was pre-existing, but this is an overly verbose way of writing:
> 
> rc = irq_set_type(gicv2_info.maintenance_irq,
>           (processor->flags & ACPI_MADT_VGIC_IRQ_MODE)
>           ? IRQ_TYPE_EDGE_BOTH
>           : IRQ_TYPE_LEVEL_MASK);
> 
> I expect the optimiser can transform behind the scenes, but it's better
> to make the C simpler for humans too.

Agreed. I'll use a single irq_set_type() call with a conditional
trigger type in both the GICv2 and GICv3 MADT paths.

Best regards,
Mykola

Re: [PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Jan Beulich]

On 10.08.2026 20:38, Mykola Kvach wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1928,6 +1928,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
>      struct acpi_table_header *table;
>      struct acpi_table_spcr *spcr;
>      acpi_status status;
> +    int rc;
>      /*
>       * Same as the DT part.
>       * Only support one UART on ARM which happen to be ns16550_com[0].
> @@ -1976,7 +1977,9 @@ static int __init ns16550_acpi_uart_init(const void *data)
>      uart->reg_width = spcr->serial_port.access_width;
>  
>      /* The trigger/polarity information is not available in spcr. */
> -    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    if ( rc )
> +        return rc;

Is erroring out still appropriate when part of ns16550_com[] was already
modified? I.e. doesn't the call need to move up then?

Jan

Re: [PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Mykola Kvach]

Hi Jan,

Thank you for the review.

On Wed, Aug 12, 2026 at 09:39:55AM +0200, Jan Beulich wrote:
> On 10.08.2026 20:38, Mykola Kvach wrote:
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -1928,6 +1928,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
> >      struct acpi_table_header *table;
> >      struct acpi_table_spcr *spcr;
> >      acpi_status status;
> > +    int rc;
> >      /*
> >       * Same as the DT part.
> >       * Only support one UART on ARM which happen to be ns16550_com[0].
> > @@ -1976,7 +1977,9 @@ static int __init ns16550_acpi_uart_init(const void *data)
> >      uart->reg_width = spcr->serial_port.access_width;
> >  
> >      /* The trigger/polarity information is not available in spcr. */
> > -    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> > +    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> > +    if ( rc )
> > +        return rc;
> 
> Is erroring out still appropriate when part of ns16550_com[] was already
> modified? I.e. doesn't the call need to move up then?

Good point. Returning there can leave ns16550_com[0] partially
initialized. I'll move irq_set_type() before ns16550_init_common()
and before modifying the UART state.

Best regards,
Mykola