Leonid Komarianskyi <leonid_komarians...@epam.com> writes:
> Currently, Xen does not support eSPI interrupts, leading
> to a data abort when such interrupts are defined in the DTS.
>
> This patch introduces a separate array to initialize up to
> 1024 interrupt descriptors in the eSPI range and adds the
> necessary defines and helper function. These changes lay the
> groundwork for future implementation of full eSPI interrupt
> support. As this GICv3.1 feature is not required by all vendors,
> all changes are guarded by ifdefs, depending on the corresponding
> Kconfig option.
I don't think that it is a good idea to hide this feature under Kconfig
option, as this will increase number of different build variants.
I believe that runtime check for GICD_TYPER.ESPI should be sufficient,
but maintainers can correct me there.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarians...@epam.com>
>
> ---
> Changes in V2:
> - use (ESPI_MAX_INTID + 1) instead of (ESPI_BASE_INTID + NR_IRQS)
> - remove unnecessary comment for nr_irqs initialization
> ---
> xen/arch/arm/Kconfig | 9 +++++++++
> xen/arch/arm/include/asm/irq.h | 25 +++++++++++++++++++++++++
> xen/arch/arm/irq.c | 26 ++++++++++++++++++++++++++
> 3 files changed, 60 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 17df147b25..08073ece1f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -135,6 +135,15 @@ config GICV3
> Driver for the ARM Generic Interrupt Controller v3.
> If unsure, use the default setting.
>
> +config GICV3_ESPI
> + bool "Extended SPI range support"
> + depends on GICV3 && !NEW_VGIC
> + default y
> + help
> + Allow Xen and domains to use interrupt numbers from the extended SPI
> + range, from 4096 to 5119. This feature is introduced in GICv3.1
> + architecture.
> +
> config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC && !ARM_32
> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> index 5bc6475eb4..acebc3d42f 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -32,6 +32,14 @@ struct arch_irq_desc {
> #define SPI_MAX_INTID 1019
> #define LPI_OFFSET 8192
>
> +#ifdef CONFIG_GICV3_ESPI
> +#define ESPI_BASE_INTID 4096
> +#define ESPI_MAX_INTID 5119
> +
> +#define ESPI_INTID2IDX(intid) ((intid) - ESPI_BASE_INTID)
> +#define ESPI_IDX2INTID(idx) ((idx) + ESPI_BASE_INTID)
> +#endif
> +
> /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
> #define INVALID_LPI 0
>
> @@ -39,7 +47,15 @@ struct arch_irq_desc {
> #define INVALID_IRQ 1023
>
> extern const unsigned int nr_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> +/*
> + * This will also cover the eSPI range, as some critical devices
> + * for booting Xen (e.g., serial) may use this type of interrupts.
> + */
> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
> +#else
> #define nr_static_irqs NR_IRQS
> +#endif
Don't introduce defines that look like variables. I am sure that MISRA
team will be unhappy about that. But what you can really do is to
introduce variable nr_static_irqs, which value will depend on
GICD_TYPER.ESPI and GICD_TYPER.ESPI_range
>
> struct irq_desc;
> struct irqaction;
> @@ -55,6 +71,15 @@ static inline bool is_lpi(unsigned int irq)
> return irq >= LPI_OFFSET;
> }
>
> +static inline bool is_espi(unsigned int irq)
> +{
> +#ifdef CONFIG_GICV3_ESPI
> + return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
> +#else
> + return false;
> +#endif
> +}
> +
> #define domain_pirq_to_irq(d, pirq) (pirq)
>
> bool is_assignable_irq(unsigned int irq);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 50e57aaea7..9bc72fbbc9 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -19,7 +19,11 @@
> #include <asm/gic.h>
> #include <asm/vgic.h>
>
> +#ifdef CONFIG_GICV3_ESPI
> +const unsigned int nr_irqs = ESPI_MAX_INTID + 1;
> +#else
> const unsigned int nr_irqs = NR_IRQS;
> +#endif
>
> static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -46,6 +50,9 @@ void irq_end_none(struct irq_desc *irq)
> }
>
> static irq_desc_t irq_desc[NR_IRQS - NR_LOCAL_IRQS];
> +#ifdef CONFIG_GICV3_ESPI
> +static irq_desc_t espi_desc[NR_IRQS];
This is really confusing. Should it be something like espi_desc[NR_ESPI_IRQS]?
> +#endif
> static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>
> struct irq_desc *__irq_to_desc(unsigned int irq)
> @@ -53,6 +60,11 @@ 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_desc[ESPI_INTID2IDX(irq)];
> +#endif
> +
> return &irq_desc[irq-NR_LOCAL_IRQS];
> }
>
> @@ -79,6 +91,20 @@ static int __init init_irq_data(void)
> desc->action = NULL;
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> + for ( irq = ESPI_BASE_INTID; irq <= ESPI_MAX_INTID; irq++ )
> + {
> + struct irq_desc *desc = irq_to_desc(irq);
> + int rc = init_one_irq_desc(desc);
> +
> + if ( rc )
> + return rc;
> +
> + desc->irq = irq;
> + desc->action = NULL;
> + }
> +#endif
> +
> return 0;
> }
--
WBR, Volodymyr
