Hi Leonid,
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.
> +{
> + 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...
> +}
>
> /* IRQ translation function for the device tree */
> int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 03fbb90c6c..12c70d02cc 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -415,7 +415,7 @@ err:
> bool is_assignable_irq(unsigned int irq)
> {
> /* For now, we can only route SPIs to the guest */
> - return (irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines());
> + return gic_is_shared_irq(irq);
> }
>
> /*
--
WBR, Volodymyr
