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

[PATCH v3 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 makes is_espi() a configuration-neutral architectural range
predicate. Configuration policy and debug checks for unsupported eSPIs are
now explicit at call sites.

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 2 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 3 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 4, introduced in v2, 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 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 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.

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

Mykola Kvach (4):
  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/arm: handle irq_set_type() failures

 xen/arch/arm/gic-v2.c          | 15 +++++++++------
 xen/arch/arm/gic-v3.c          | 15 +++++++++------
 xen/arch/arm/gic.c             |  5 ++++-
 xen/arch/arm/include/asm/irq.h | 11 -----------
 xen/arch/arm/irq.c             | 26 ++++++++++++++++++++++----
 xen/arch/arm/tee/ffa_notif.c   | 11 ++++++++++-
 xen/arch/arm/time.c            | 18 ++++++++++++++----
 xen/arch/arm/vgic.c            | 31 ++++++++++++++++++-------------
 xen/drivers/char/ns16550.c     |  8 ++++++--
 xen/drivers/char/pl011.c       |  4 +++-
 10 files changed, 95 insertions(+), 49 deletions(-)

-- 
2.43.0

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

[Mykola Kvach]

is_espi() currently changes its result according to CONFIG_GICV3_ESPI
and asserts when an eSPI INTID is passed to a build without eSPI
support. This makes a range predicate carry configuration policy and
causes callers to depend on its hidden side effects.

Make is_espi() report only whether an INTID is in the architectural
eSPI range. Gate eSPI handling explicitly at call sites and preserve
the debug checks on paths where an eSPI is invalid without compiled-in
support.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Changes in v3:
- New preparatory cleanup requested during review.
---
 xen/arch/arm/gic.c             |  5 ++++-
 xen/arch/arm/include/asm/irq.h | 11 -----------
 xen/arch/arm/vgic.c            |  4 ++--
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 078049e741..075e1d2c50 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -348,7 +348,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
         /* Reading IRQ will ACK it */
         irq = gic_hw_ops->read_irq();
 
-        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || is_espi(irq) )
+        ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));
+
+        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) ||
+             (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq)) )
         {
             isb();
             do_IRQ(regs, irq, is_fiq);
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..e14123a30a 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -718,8 +718,9 @@ struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
     unsigned int idx;
 
     ASSERT(irq >= NR_LOCAL_IRQS);
+    ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));
 
-    if ( is_espi(irq) )
+    if ( IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq) )
     {
         unsigned int nr_spis = d->arch.vgic.nr_spis;
 
@@ -949,4 +950,3 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, unsigned int rank, uint32_
  * indent-tabs-mode: nil
  * End:
  */
-
-- 
2.43.0

[PATCH v3 2/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.

Assert the regular descriptor bound in __irq_to_desc() so direct callers
cannot silently index the sparse gap in debug builds.

Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
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 | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 73e58a5108..bf14180f97 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);
 
@@ -76,7 +82,6 @@ static int __init init_espi_data(void)
     return 0;
 }
 #else
-
 static int __init init_espi_data(void)
 {
     return 0;
@@ -95,6 +100,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
         return espi_to_desc(irq);
 #endif
 
+    ASSERT(irq < NR_IRQS);
+
     return &irq_desc[irq-NR_LOCAL_IRQS];
 }
 
@@ -416,6 +423,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 +657,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.43.0

[PATCH v3 3/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. 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 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 | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e14123a30a..e541348a5c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -33,6 +33,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
@@ -849,19 +859,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)
@@ -898,7 +900,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 v3 4/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 UART or notification setup when trigger
configuration fails.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
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 | 11 ++++++++++-
 xen/arch/arm/time.c          | 18 ++++++++++++++----
 xen/drivers/char/ns16550.c   |  8 ++++++--
 xen/drivers/char/pl011.c     |  4 +++-
 6 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 43a379fdda..a24d387e7d 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..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..eb608ab8b4 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].
@@ -1959,6 +1960,11 @@ static int __init ns16550_acpi_uart_init(const void *data)
         return -EINVAL;
     }
 
+    /* The trigger/polarity information is not available in spcr. */
+    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    if ( rc )
+        return rc;
+
     ns16550_init_common(uart);
 
     /*
@@ -1975,8 +1981,6 @@ static int __init ns16550_acpi_uart_init(const void *data)
     uart->reg_shift = spcr->serial_port.bit_offset;
     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;
 
     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 v3 1/4] xen/arm: make is_espi() a pure range predicate

[Volodymyr Babchuk]

Hi Mykola,

Mykola Kvach <mykola_kvach@epam.com> writes:

> is_espi() currently changes its result according to CONFIG_GICV3_ESPI
> and asserts when an eSPI INTID is passed to a build without eSPI
> support.

Probably you want to reword this part of the commit message. I think you
wanted to say that "assertion fails when an eSPI INTID is passed to a
build without eSPI support".

> This makes a range predicate carry configuration policy and
> causes callers to depend on its hidden side effects.

I'm not sure that I got this.

>
> Make is_espi() report only whether an INTID is in the architectural
> eSPI range. Gate eSPI handling explicitly at call sites and preserve
> the debug checks on paths where an eSPI is invalid without compiled-in
> support.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> Changes in v3:
> - New preparatory cleanup requested during review.
> ---
>  xen/arch/arm/gic.c             |  5 ++++-
>  xen/arch/arm/include/asm/irq.h | 11 -----------
>  xen/arch/arm/vgic.c            |  4 ++--
>  3 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 078049e741..075e1d2c50 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -348,7 +348,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>          /* Reading IRQ will ACK it */
>          irq = gic_hw_ops->read_irq();
>  
> -        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || is_espi(irq) )
> +        ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));

I am not sure that it is a good idea to put ASSERT on value that we got
from external source. What if Xen is build without CONFIG_GICV3_ESPI but
hardware really reports an eSPI?

> +
> +        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) ||
> +             (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq)) )
>          {
>              isb();
>              do_IRQ(regs, irq, is_fiq);
> 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..e14123a30a 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -718,8 +718,9 @@ struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
>      unsigned int idx;
>  
>      ASSERT(irq >= NR_LOCAL_IRQS);
> +    ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));
>  
> -    if ( is_espi(irq) )
> +    if ( IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq) )
>      {
>          unsigned int nr_spis = d->arch.vgic.nr_spis;
>  
> @@ -949,4 +950,3 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, unsigned int rank, uint32_
>   * indent-tabs-mode: nil
>   * End:
>   */
> -

Please refrain from unneeded changes.



-- 
WBR, Volodymyr

Re: [PATCH v3 2/4] xen/arm: validate IRQs before descriptor lookup

[Volodymyr Babchuk]

Hi,

Mykola Kvach <mykola_kvach@epam.com> writes:

> 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.
>
> Assert the regular descriptor bound in __irq_to_desc() so direct callers
> cannot silently index the sparse gap in debug builds.
>
> Fixes: 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in the eSPI range")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> 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 | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 73e58a5108..bf14180f97 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)

You are using this function only in one place, where you are actually
testing for SPI. So, maybe introduce irq_is_spi() helper instead? And
use it below?

> +{
> +    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);
>  
> @@ -76,7 +82,6 @@ static int __init init_espi_data(void)
>      return 0;
>  }
>  #else
> -

Please, no unnecessary changes

>  static int __init init_espi_data(void)
>  {
>      return 0;
> @@ -95,6 +100,8 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
>          return espi_to_desc(irq);
>  #endif
>  
> +    ASSERT(irq < NR_IRQS);
> +
>      return &irq_desc[irq-NR_LOCAL_IRQS];
>  }
>  
> @@ -416,6 +423,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 +657,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) )

So here you can just call if ( !irq_is_spi(spi) )

>          return -EINVAL;
>  
> +    desc = irq_to_desc(spi);
> +
>      spin_lock_irqsave(&desc->lock, flags);
>  
>      if ( !irq_validate_new_type(desc->arch.type, type) )

-- 
WBR, Volodymyr

Re: [PATCH v3 3/4] xen/arm: vgic: free eSPIs using the bitmap index

[Volodymyr Babchuk]

Hi,


I have only one small question to this patch. Please see below.

Mykola Kvach <mykola_kvach@epam.com> writes:

> 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 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 | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e14123a30a..e541348a5c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -33,6 +33,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
> @@ -849,19 +859,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)
> @@ -898,7 +900,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) )

Is this really can happen during normal runtime?

> +        return;
> +
> +    clear_bit(virq_to_idx(d, virq), d->arch.vgic.allocated_irqs);
>  }
>  
>  unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)

-- 
WBR, Volodymyr

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

[Volodymyr Babchuk]

Hi,

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 UART or notification setup when trigger
> configuration fails.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>

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

> ---
> 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 | 11 ++++++++++-
>  xen/arch/arm/time.c          | 18 ++++++++++++++----
>  xen/drivers/char/ns16550.c   |  8 ++++++--
>  xen/drivers/char/pl011.c     |  4 +++-
>  6 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 43a379fdda..a24d387e7d 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..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..eb608ab8b4 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].
> @@ -1959,6 +1960,11 @@ static int __init ns16550_acpi_uart_init(const void *data)
>          return -EINVAL;
>      }
>  
> +    /* The trigger/polarity information is not available in spcr. */
> +    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    if ( rc )
> +        return rc;
> +
>      ns16550_init_common(uart);
>  
>      /*
> @@ -1975,8 +1981,6 @@ static int __init ns16550_acpi_uart_init(const void *data)
>      uart->reg_shift = spcr->serial_port.bit_offset;
>      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;
>  
>      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,

-- 
WBR, Volodymyr