On 21/08/2025 17:59, Volodymyr Babchuk wrote:
Julien Grall <jul...@xen.org> writes:
Hi,
On 21/08/2025 16:59, Volodymyr Babchuk wrote:
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.
I haven't seen many board with ESPI available. So I think it would be
better if this is under a Kconfig because not everyone may want to
have the code.
Probably, we can expect more in the future...
Well yes. But I was under the impression this the preferred approach.
See the discussion about disabling 32-bit support on 64-bit:
20250723075835.3993182-1-grygorii_stras...@epam.com
Anyways, after reviewing
all patches in the series, I can see that code will be littered with
#ifdef CONFIG_GICV3_ESPI, which, probably, not a good thing.
The solution is to provide wrappers to reduce the number of #ifdef. I
haven't checked all the places.
[...]
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];
By how much will this increase the Xen binary?
I am wondering this also. The struct is cache aligned, so it will take
at least 128 bytes. The whole array will take at least 128kb. Not
great, not terrible. As this should go to .bss, it should not increase
the binary itself.
I guess "binary" was the wrong word. I was referring to the size of the
Xen in memory. On my setup Xen is 1448kb. Here you would increase ~9% of
resident size. This seems quite steep for a feature that is not often used.
Maybe it is better to allocate this dynamically? I do understand that we
want to get rid of as many dynamic allocs as possible, but maybe in this
case it will be okay.
This is up to Leonid. I don't think this is strictly necessary in order
to get the eSPI support. However, until this is solved CONFIG_GICV3_EPSI
*must not* be on by default as this is done in this patch.
As a bonus point, we can't leave this pointer
present even if CONFIG_GICV3_ESPI=n, which will simplify some code in
latter patches.
Did you intend to say "We can leave" rather than "we can't leave"?
Cheers,
--
Julien Grall
