Hi,
On 21/08/2025 16:39, Volodymyr Babchuk wrote:
Leonid Komarianskyi <leonid_komarians...@epam.com> writes:
Introduced two new helper functions: gic_is_valid_irq and
gic_is_shared_irq. The first function helps determine whether an IRQ
number is less than the number of lines supported by hardware. The
second function additionally checks if the IRQ number falls within the
SPI range. Also, updated the appropriate checks to use these new helper
functions.
The current checks for the real GIC are very similar to those for the
vGIC but serve a different purpose. For GIC-related code, the interrupt
numbers should be validated based on whether the hardware can operate
with such interrupts. On the other hand, for the vGIC, the indexes must
also be verified to ensure they are available for a specific domain. The
first reason for introducing these helper functions is to avoid
potential confusion with vGIC-related checks. The second reason is to
consolidate similar code into separate functions, which can be more
easily extended by additional conditions, e.g., when implementing
extended SPI interrupts.
The changes, which replace open-coded checks with the use of the new
helper functions, do not introduce any functional changes, as the helper
functions follow the current IRQ index verification logic.
Signed-off-by: Leonid Komarianskyi <leonid_komarians...@epam.com>
---
Changes in V2:
- introduced this patch
---
xen/arch/arm/gic.c | 2 +-
xen/arch/arm/include/asm/gic.h | 9 +++++++++
xen/arch/arm/irq.c | 2 +-
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e80fe0ca24..eb0346a898 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -111,7 +111,7 @@ static void gic_set_irq_priority(struct irq_desc *desc,
unsigned int priority)
void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
{
ASSERT(priority <= 0xff); /* Only 8 bits of priority */
- ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that
don't exist */
+ ASSERT(gic_is_valid_irq(desc->irq));/* Can't route interrupts that don't
exist */
ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
ASSERT(spin_is_locked(&desc->lock));
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 541f0eeb80..ac0b7b783e 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -306,6 +306,15 @@ extern void gic_dump_vgic_info(struct vcpu *v);
/* Number of interrupt lines */
extern unsigned int gic_number_lines(void);
+static inline bool gic_is_valid_irq(unsigned int irq)
We need to do something about naming, because this function completely
ignores presence of LPIs. What I mean, that it will return "false" for
any LPI, while you can't argue that LPI is a valid interrupt :)
I understand that this is expected behavior by current callers, but the
function name is misleading.
Name like "gic_is_valid_non_lpi()" seems to mouthful, but it is the best
I can come up with.
AFAIU, there is no interrupt lines for LPIs. So what about
gic_is_valid_line()?
+{
+ return irq < gic_number_lines();
+}
+
+static inline bool gic_is_shared_irq(unsigned int irq)
+{
+ return (irq >= NR_LOCAL_IRQS && gic_is_valid_irq(irq));
Again, because of misleading name of gic_is_valid_irq() it may seem that
this function will return "true" for LPIs as well...
Even if we rename gic_is_valid_irq(), the function name would be
misleading because LPIs are shared. I think it would be better named
gic_is_spi(...);
Cheers,
--
Julien Grall
