[PATCH v6 0/4] xen/arm: Fix eSPI IRQ handling

[PATCH v6 0/4] xen/arm: Fix eSPI IRQ handling

[Mykola Kvach]

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

Patch 1 checks irq_set_type() failures in the GTDT, MADT, SPCR, and FF-A
paths. GTDT and MADT could retain rejected timer or maintenance INTIDs
and later use them in direct descriptor lookups. This patch also fixes
MISRA C Rule 17.7 violations. It is now first in the series, as requested
during review, so callers handle errors before stricter IRQ validation
is introduced.

Patch 2 makes is_espi() check the architectural INTID range regardless of
CONFIG_GICV3_ESPI. If the GIC reports an eSPI without compiled-in support,
Xen stops with BUG_ON(), as there is no descriptor or pending_irq storage
for it. Virtual eSPI pending lookups without support are treated as
unreachable: the stub asserts in debug builds and retains a NULL return.

Xen has IRQ descriptors for INTIDs below NR_IRQS and, with eSPI support,
for eSPIs starting at 4096. It has no descriptors for INTIDs 1024 through
4095. Patch 3 checks INTIDs in setup_irq() and irq_set_spi_type() before
descriptor lookup. irq_set_spi_type() checks descriptor-backed ranges
because it can run before the GIC line counts are known. setup_irq()
checks implemented lines using those counts.

Patch 4 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. Both vGIC implementations now
use vgic_is_valid_line() when reserving and freeing vIRQs.

Testing of the v6 UART changes, before the last FF-A formatting change:

- Arm64 debug build passed with CONFIG_ACPI=y, CONFIG_FFA=y,
  CONFIG_GICV3_ESPI=y, CONFIG_HAS_NS16550=y and CONFIG_HAS_PL011=y.
- Tested PL011 and NS16550 on QEMU with GICv3 and UEFI/ACPI.
  Tested each driver with a valid IRQ and with IRQ 1024 in SPCR.
- All four tests passed. With IRQ 1024, irq_set_type() returned
  -EINVAL, the driver used IRQ 0 and printed a warning.
  Console output kept working through Dom0 startup.

Testing of the v5 review changes, before the latest rebase and reordering:

- Arm64 build with CONFIG_NEW_VGIC=y, CONFIG_GICV2=y, and CONFIG_DEBUG=y.
- QEMU GICv2 guest tests: 10 checks passed, covering SPI delivery,
  enable/disable behavior, affinity changes, SGIs, and the virtual timer.

Testing from earlier revisions, before the final v5 edits:

- QEMU and FVP tests with CONFIG_GICV3_ESPI, CONFIG_HAS_ITS, and
  CONFIG_DEBUG enabled and disabled.
- Physical and virtual eSPI delivery on FVP, including delivery to both
  vCPUs and retriggering an active eSPI.
- Injected an unsupported physical eSPI and confirmed BUG_ON() in both
  debug and release builds.
- Arm64 builds with CONFIG_GICV3_ESPI and CONFIG_DEBUG enabled and disabled.
- 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, covering the GTDT,
  GICv3 MADT, and PL011 SPCR paths.

Changes in v6:

- Keep the NS16550 and PL011 ACPI consoles available when
  irq_set_type() fails: emit a warning and fall back to polling.
- Adjust the FF-A error message formatting.

Changes in v5:

- Move the irq_set_type() error-handling patch to the beginning.
- Explain why an eSPI cannot be handled without compiled-in support.
- Make both espi_to_pending() helpers static inline and constify d.
- Add ASSERT_UNREACHABLE() to the disabled-eSPI stub.
- Preserve the unmapped LPI comment and clarify that eSPI lookup without
  support must not occur.
- Add the vgic_is_valid_line() guard to vgic_free_virq() in the new vGIC
  implementation and use the same helper in vgic_reserve_virq().

Changes in v4:

- Use BUG_ON() when the GIC reports an eSPI without compiled-in support.
- Return NULL for virtual eSPI pending lookups when support is disabled.
- Remove the redundant CONFIG_GICV3_ESPI check in IRQ dispatch.
- Share irq_has_desc() with the assertion in __irq_to_desc().
- Log invalid IRQs rejected by setup_irq().
- Document the compressed vIRQ allocation bitmap above the conversion
  helpers, with an ASCII diagram and a reference to struct vgic_dist.
- Clarify the is_espi() commit message and drop unrelated blank-line
  removals.
- Add Reviewed-by tags.

Changes in v3:

- Add a preparatory patch making is_espi() a pure range predicate and move
  configuration policy and debug checks to callers.
- Add the requested assertion before regular descriptor lookup.
- Avoid partial MADT and UART state updates after irq_set_type() failures.
- Apply cosmetic cleanups from review.

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 a patch checking irq_set_type() errors in the GTDT, MADT, SPCR, and
  FF-A paths.
- Target master instead of the 4.22 release.

Mykola Kvach (4):
  xen/arm: handle irq_set_type() failures
  xen/arm: make is_espi() a pure range predicate
  xen/arm: validate IRQs before descriptor lookup
  xen/arm: vgic: free eSPIs using the bitmap index

 xen/arch/arm/gic-v2.c          | 15 ++++---
 xen/arch/arm/gic-v3.c          | 15 ++++---
 xen/arch/arm/gic.c             |  6 +++
 xen/arch/arm/include/asm/irq.h | 11 ------
 xen/arch/arm/irq.c             | 28 +++++++++++--
 xen/arch/arm/tee/ffa_notif.c   | 10 ++++-
 xen/arch/arm/time.c            | 18 +++++++--
 xen/arch/arm/vgic.c            | 72 +++++++++++++++++++++++-----------
 xen/arch/arm/vgic/vgic.c       |  5 ++-
 xen/drivers/char/ns16550.c     |  8 +++-
 xen/drivers/char/pl011.c       | 12 +++++-
 11 files changed, 142 insertions(+), 58 deletions(-)

-- 
2.53.0

[PATCH v6 1/4] 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.

GTDT and MADT retain rejected timer and maintenance INTIDs.
check_timer_irq_cfg() and release_irq() later perform unconditional
descriptor lookups on those values. Xen has no backing descriptors for
INTIDs 1024 through 4095, so retaining one can cause an out-of-bounds
access.

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 notification setup when trigger configuration
fails. Keep the NS16550 and PL011 consoles available by falling back to
polling, with a warning, if their IRQ trigger configuration fails.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v6:
- Fall back to polling with a warning when ACPI IRQ configuration fails
  for NS16550 or PL011, preserving the console.

Changes in v3:
- Avoid partial state updates and simplify maintenance IRQ setup.

Changes in v2:
- New patch.
---
 xen/arch/arm/gic-v2.c        | 15 +++++++++------
 xen/arch/arm/gic-v3.c        | 15 +++++++++------
 xen/arch/arm/tee/ffa_notif.c | 10 +++++++++-
 xen/arch/arm/time.c          | 18 ++++++++++++++----
 xen/drivers/char/ns16550.c   |  8 +++++++-
 xen/drivers/char/pl011.c     | 12 ++++++++++--
 6 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index d05c574d88..87eda9d322 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1166,17 +1166,20 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
     /* Read from APIC table and fill up the GIC variables */
     if ( cpu_base_assigned == 0 )
     {
+        int rc;
+
+        rc = irq_set_type(processor->vgic_interrupt,
+                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
+                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
+
+        if ( rc )
+            return rc;
+
         cbase = processor->base_address;
         csize = SZ_8K;
         hbase = processor->gich_base_address;
         vbase = processor->gicv_base_address;
         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);
-        else
-            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
-
         cpu_base_assigned = 1;
     }
     else
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index acdac22953..b32a9b5009 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1743,15 +1743,18 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
     /* Read from APIC table and fill up the GIC variables */
     if ( !cpu_base_assigned )
     {
+        int rc;
+
+        rc = irq_set_type(processor->vgic_interrupt,
+                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
+                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
+
+        if ( rc )
+            return rc;
+
         cbase = processor->base_address;
         vbase = processor->gicv_base_address;
         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);
-        else
-            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
-
         cpu_base_assigned = 1;
     }
     else
diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
index 186e726412..c64bc39f03 100644
--- a/xen/arch/arm/tee/ffa_notif.c
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -407,7 +407,15 @@ 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 be54b87438..ccfb76e20f 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 593d208483..cdda28879e 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1990,8 +1990,14 @@ 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);
     uart->irq = spcr->interrupt;
+    if ( irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH) )
+    {
+        printk(XENLOG_WARNING
+               "ns16550: unable to configure IRQ %u, using polling\n",
+               spcr->interrupt);
+        uart->irq = 0;
+    }
 
     uart->vuart.base_addr = uart->io_base;
     uart->vuart.size = uart->io_size;
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index a336241033..612d457fe0 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -347,6 +347,7 @@ static int __init pl011_acpi_uart_init(const void *data)
 {
     acpi_status status;
     struct acpi_table_spcr *spcr = NULL;
+    unsigned int irq;
     int res;
     bool sbsa;
 
@@ -363,10 +364,17 @@ 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);
+    irq = spcr->interrupt;
+    if ( irq_set_type(irq, IRQ_TYPE_LEVEL_HIGH) )
+    {
+        printk(XENLOG_WARNING
+               "pl011: unable to configure IRQ %u, using polling\n",
+               irq);
+        irq = 0;
+    }
 
     /* TODO - mmio32 proper handling (for now set to true) */
-    res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
+    res = pl011_uart_init(irq, spcr->serial_port.address,
                           PAGE_SIZE, sbsa, true);
     if ( res < 0 )
     {
-- 
2.53.0

[PATCH v6 2/4] xen/arm: make is_espi() a pure range predicate

[Mykola Kvach]

is_espi() currently changes its result according to CONFIG_GICV3_ESPI.
Without eSPI support, it returns false, and its assertion fails if
an eSPI INTID is passed. Callers therefore use it both to identify
eSPIs and to exclude eSPI handling when support is disabled.

Make is_espi() report only whether an INTID is in the architectural
eSPI range. Check for eSPI support at the call sites.

Use BUG_ON() if the GIC reports an eSPI without compiled-in support,
matching the handling of unsupported LPIs. Treat virtual eSPI lookup
without support as unreachable, with a NULL return from the
espi_to_pending() stub.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v5:
- Explain why an eSPI cannot be handled without compiled-in support.
- Make both espi_to_pending() helpers static inline and constify d.
- Add ASSERT_UNREACHABLE() to the espi_to_pending() stub.
- Preserve the unmapped LPI comment and clarify that an eSPI lookup
  without support must not occur.

Changes in v4:
- Clarify the existing is_espi() behavior in the commit message.
- Drop the unrelated blank-line removal in vgic.c.
- Use BUG_ON() if the GIC reports an eSPI without eSPI support.
- Drop the redundant CONFIG_GICV3_ESPI check in IRQ dispatch.
- Return NULL for virtual eSPI lookup when eSPI support is disabled.

Changes in v3:
- New preparatory cleanup requested during review.
---
 xen/arch/arm/gic.c             |  6 ++++++
 xen/arch/arm/include/asm/irq.h | 11 -----------
 xen/arch/arm/vgic.c            | 30 ++++++++++++++++++------------
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6a132c64e1..0e96def2d0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -353,6 +353,12 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
         /* Reading IRQ will ACK it */
         irq = gic_hw_ops->read_irq();
 
+        /*
+         * Without CONFIG_GICV3_ESPI, there is no IRQ descriptor or
+         * pending_irq storage for eSPIs, so we cannot handle them.
+         */
+        BUG_ON(!IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
+
         if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || is_espi(irq) )
         {
             isb();
diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index 09788dbfeb..c29f3d04a3 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -66,18 +66,7 @@ static inline bool is_lpi(unsigned int irq)
 
 static inline bool is_espi(unsigned int irq)
 {
-#ifdef CONFIG_GICV3_ESPI
     return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
-#else
-    /*
-     * The function should not be called for eSPIs when CONFIG_GICV3_ESPI is
-     * disabled. Returning false allows the compiler to optimize the code
-     * when the config is disabled, while the assert ensures that out-of-range
-     * array resources are not accessed.
-     */
-    ASSERT(!(irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID));
-    return false;
-#endif
 }
 
 static inline unsigned int espi_intid_to_idx(unsigned int intid)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e5aca17dcb..0ba13e18da 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -62,6 +62,14 @@ static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
     return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
 }
 
+static inline struct pending_irq *espi_to_pending(const struct domain *d,
+                                                  unsigned int irq)
+{
+    unsigned int idx = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
+
+    return &d->arch.vgic.pending_irqs[idx];
+}
+
 #else
 static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
 {
@@ -78,6 +86,13 @@ static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
     ASSERT_UNREACHABLE();
     return NULL;
 }
+
+static inline struct pending_irq *espi_to_pending(const struct domain *d,
+                                                  unsigned int irq)
+{
+    ASSERT_UNREACHABLE();
+    return NULL;
+}
 #endif
 
 static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
@@ -698,6 +713,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
  * interrupt.
  * This can return NULL if called for an LPI which has been unmapped
  * meanwhile.
+ * This must not be called for an eSPI when CONFIG_GICV3_ESPI is disabled.
  */
 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
 {
@@ -715,22 +731,12 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
 
 struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
 {
-    unsigned int idx;
-
     ASSERT(irq >= NR_LOCAL_IRQS);
 
     if ( is_espi(irq) )
-    {
-        unsigned int nr_spis = d->arch.vgic.nr_spis;
+        return espi_to_pending(d, irq);
 
-        idx = espi_intid_to_idx(irq) + nr_spis;
-    }
-    else
-    {
-        idx = irq - NR_LOCAL_IRQS;
-    }
-
-    return &d->arch.vgic.pending_irqs[idx];
+    return &d->arch.vgic.pending_irqs[irq - NR_LOCAL_IRQS];
 }
 
 void vgic_clear_pending_irqs(struct vcpu *v)
-- 
2.53.0

[PATCH v6 3/4] 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.

Use the same descriptor range check in irq_set_spi_type() and the
assertion in __irq_to_desc() to keep them in sync. Log the IRQ number
when setup_irq() rejects an invalid line.

Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v4:
- Share irq_has_desc() with the assertion in __irq_to_desc().
- Log invalid IRQs rejected by setup_irq().
- Drop the unrelated blank line removal.

Changes in v3:
- Add the requested bound assertion and retain the SPI-only comment.

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 | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 73e58a5108..8d3517e761 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -85,6 +85,12 @@ static int __init init_espi_data(void)
 
 static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 
+static bool irq_has_desc(unsigned int irq)
+{
+    return irq < NR_IRQS ||
+           (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
+}
+
 struct irq_desc *__irq_to_desc(unsigned int irq)
 {
     if ( irq < NR_LOCAL_IRQS )
@@ -95,6 +101,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
         return espi_to_desc(irq);
 #endif
 
+    ASSERT(irq_has_desc(irq));
+
     return &irq_desc[irq-NR_LOCAL_IRQS];
 }
 
@@ -416,6 +424,12 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
     struct irq_desc *desc;
     bool disabled;
 
+    if ( !gic_is_valid_line(irq) )
+    {
+        printk(XENLOG_ERR "Cannot set up IRQ %u: invalid GIC interrupt\n", irq);
+        return -EINVAL;
+    }
+
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock, flags);
@@ -647,13 +661,21 @@ 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 )
+    /*
+     * This function should not be used for other than SPIs.
+     *
+     * 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.53.0

[PATCH v6 4/4] 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. Use vgic_is_valid_line() for the validity checks when
reserving and freeing vIRQs in both vGIC implementations.

Fixes: bdde400c6e1b ("xen/arm: vgic: add resource management for extended SPIs")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v5:
- Add the vgic_is_valid_line() guard to vgic_free_virq() in the new
  vGIC implementation and use the same helper in vgic_reserve_virq().

Changes in v4:
- Document the compressed bitmap layout with an ASCII diagram above the
  conversion helpers and refer to struct vgic_dist for the full layout.

Changes in v3:
- Adapt virq_to_idx() to the configuration-neutral is_espi() helper.

Changes in v2:
- Call is_espi() without a configuration guard.
---
 xen/arch/arm/vgic.c      | 42 +++++++++++++++++++++++++++++-----------
 xen/arch/arm/vgic/vgic.c |  5 ++++-
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 0ba13e18da..acded6a40f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -25,6 +25,21 @@
 #include <asm/vgic.h>
 
 
+/*
+ * The allocated_irqs bitmap is compressed: eSPI bits immediately follow
+ * regular IRQ bits, skipping the gap in the INTID space.
+ *
+ *   +-----------+-----------+-------------------+-------------------+
+ *   |   SGIs    |   PPIs    |       SPIs        |       eSPIs       |
+ *   +-----------+-----------+-------------------+-------------------+
+ *   0           16          32                  vgic_num_irqs(d)
+ *
+ * INTID ESPI_BASE_INTID maps to bitmap index vgic_num_irqs(d).
+ * The following idx_to_virq() and virq_to_idx() convert between INTIDs
+ * and bitmap indexes.
+ *
+ * See also the allocated_irqs comment in struct vgic_dist.
+ */
 static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
 {
     if ( idx >= vgic_num_irqs(d) )
@@ -33,6 +48,16 @@ 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)
+{
+    ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(virq));
+
+    if ( IS_ENABLED(CONFIG_GICV3_ESPI) && 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
@@ -854,19 +879,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)
@@ -903,7 +920,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)
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index ba029b8a3b..84212bbefe 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -712,7 +712,7 @@ bool vgic_evtchn_irq_pending(struct vcpu *v)
 
 bool vgic_reserve_virq(struct domain *d, unsigned int virq)
 {
-    if ( virq >= vgic_num_irqs(d) )
+    if ( !vgic_is_valid_line(d, virq) )
         return false;
 
     return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
@@ -756,6 +756,9 @@ int vgic_allocate_virq(struct domain *d, bool spi)
 
 void vgic_free_virq(struct domain *d, unsigned int virq)
 {
+    if ( !vgic_is_valid_line(d, virq) )
+        return;
+
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
-- 
2.53.0

Re: [PATCH v6 1/4] xen/arm: handle irq_set_type() failures

[Volodymyr Babchuk]

Hi Mykola,

Mykola Kvach <mykola_kvach@epam.com> writes:

> 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.
>
> GTDT and MADT retain rejected timer and maintenance INTIDs.
> check_timer_irq_cfg() and release_irq() later perform unconditional
> descriptor lookups on those values. Xen has no backing descriptors for
> INTIDs 1024 through 4095, so retaining one can cause an out-of-bounds
> access.
>
> 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 notification setup when trigger configuration
> fails. Keep the NS16550 and PL011 consoles available by falling back to
> polling, with a warning, if their IRQ trigger configuration fails.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Not sure that this is a good idea to leave R-b tags if you are doing
functional changes to the patches.

You can have my R-b though:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
> Changes in v6:
> - Fall back to polling with a warning when ACPI IRQ configuration fails
>   for NS16550 or PL011, preserving the console.
>
> Changes in v3:
> - Avoid partial state updates and simplify maintenance IRQ setup.
>
> Changes in v2:
> - New patch.
> ---
>  xen/arch/arm/gic-v2.c        | 15 +++++++++------
>  xen/arch/arm/gic-v3.c        | 15 +++++++++------
>  xen/arch/arm/tee/ffa_notif.c | 10 +++++++++-
>  xen/arch/arm/time.c          | 18 ++++++++++++++----
>  xen/drivers/char/ns16550.c   |  8 +++++++-
>  xen/drivers/char/pl011.c     | 12 ++++++++++--
>  6 files changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index d05c574d88..87eda9d322 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1166,17 +1166,20 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>      /* Read from APIC table and fill up the GIC variables */
>      if ( cpu_base_assigned == 0 )
>      {
> +        int rc;
> +
> +        rc = irq_set_type(processor->vgic_interrupt,
> +                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
> +                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
> +
> +        if ( rc )
> +            return rc;
> +
>          cbase = processor->base_address;
>          csize = SZ_8K;
>          hbase = processor->gich_base_address;
>          vbase = processor->gicv_base_address;
>          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);
> -        else
> -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> -
>          cpu_base_assigned = 1;
>      }
>      else
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index acdac22953..b32a9b5009 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1743,15 +1743,18 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>      /* Read from APIC table and fill up the GIC variables */
>      if ( !cpu_base_assigned )
>      {
> +        int rc;
> +
> +        rc = irq_set_type(processor->vgic_interrupt,
> +                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
> +                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
> +
> +        if ( rc )
> +            return rc;
> +
>          cbase = processor->base_address;
>          vbase = processor->gicv_base_address;
>          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);
> -        else
> -            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> -
>          cpu_base_assigned = 1;
>      }
>      else
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> index 186e726412..c64bc39f03 100644
> --- a/xen/arch/arm/tee/ffa_notif.c
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -407,7 +407,15 @@ 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 be54b87438..ccfb76e20f 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 593d208483..cdda28879e 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1990,8 +1990,14 @@ 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);
>      uart->irq = spcr->interrupt;
> +    if ( irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH) )
> +    {
> +        printk(XENLOG_WARNING
> +               "ns16550: unable to configure IRQ %u, using polling\n",
> +               spcr->interrupt);
> +        uart->irq = 0;
> +    }
>  
>      uart->vuart.base_addr = uart->io_base;
>      uart->vuart.size = uart->io_size;
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index a336241033..612d457fe0 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -347,6 +347,7 @@ static int __init pl011_acpi_uart_init(const void *data)
>  {
>      acpi_status status;
>      struct acpi_table_spcr *spcr = NULL;
> +    unsigned int irq;
>      int res;
>      bool sbsa;
>  
> @@ -363,10 +364,17 @@ 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);
> +    irq = spcr->interrupt;
> +    if ( irq_set_type(irq, IRQ_TYPE_LEVEL_HIGH) )
> +    {
> +        printk(XENLOG_WARNING
> +               "pl011: unable to configure IRQ %u, using polling\n",
> +               irq);
> +        irq = 0;
> +    }
>  
>      /* TODO - mmio32 proper handling (for now set to true) */
> -    res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
> +    res = pl011_uart_init(irq, spcr->serial_port.address,
>                            PAGE_SIZE, sbsa, true);
>      if ( res < 0 )
>      {

-- 
WBR, Volodymyr