Hi Julien,
Thank you for your comments.
On 04.09.25 17:06, Julien Grall wrote:
> Hi Leonid,
>
> On 04/09/2025 14:09, Leonid Komarianskyi wrote:
>> On 04.09.25 15:27, Julien Grall wrote:
>>> Hi Leonid,
>>>
>>> On 03/09/2025 15:29, Leonid Komarianskyi wrote:
>>>> ---
>>>> xen/arch/arm/Kconfig | 8 +++++
>>>> xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
>>>> xen/arch/arm/irq.c | 53 +++++++++++++++++++++++++++++
>>>> +++--
>>>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 17df147b25..43b05533b1 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -135,6 +135,14 @@ 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
>>>> + 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..f4d0997651 100644
>>>> --- a/xen/arch/arm/include/asm/irq.h
>>>> +++ b/xen/arch/arm/include/asm/irq.h
>>>> @@ -32,6 +32,10 @@ struct arch_irq_desc {
>>>> #define SPI_MAX_INTID 1019
>>>> #define LPI_OFFSET 8192
>>>> +#define ESPI_BASE_INTID 4096
>>>> +#define ESPI_MAX_INTID 5119
>>>> +#define NR_ESPI_IRQS 1024
>>>> +
>>>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid
>>>> case. */
>>>> #define INVALID_LPI 0
>>>> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>>>> #define INVALID_IRQ 1023
>>>> extern const unsigned int nr_irqs;
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +/* This will cover the eSPI range, to allow asignmant of eSPIs to
>>>> domains. */
>>>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>>>> +#else
>>>> #define nr_static_irqs NR_IRQS
>>>> +#endif
>>>> struct irq_desc;
>>>> struct irqaction;
>>>> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>>>> return irq >= LPI_OFFSET;
>>>> }
>>>> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
>>>> +{
>>>> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
>>>
>>> Can we use is_espi()?
>>>
>>
>> Yes, sure. I just need to change the function declaration order and then
>> I can use is_espi() here. I will do this in V7.
>>
>>>> + return intid - ESPI_BASE_INTID;
>>>> +}
>>>> +
>>>> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
>>>> +{
>>>> + ASSERT(idx <= NR_ESPI_IRQS);
>>>> + return idx + ESPI_BASE_INTID;
>>>> +}
>>>> +
>>>> +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, e.g., in __irq_to_desc().
>>>> + */
>>>> + ASSERT(irq >= ESPI_BASE_INTID);
>>>
>>> Regardless what Volodymyr mentioned about the assert!(), I am a bit
>>> unsure where we guarantee is_espi() will not be called with an irq <=
>>> ESPI_BASE_INTID. In fact, we could have the following code in Xen:
>>>
>>> if (is_espi(irq))
>>> {
>>> }
>>> else if (is_lpi(irq))
>>> {
>>> }
>>> else
>>> {
>>> }
>>>
>>> We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <=
>>> ESPI_MAX_INTID)". But I would actually prefer if there is no check
>>> because I don't see the value.
>>>
>>
>> The main reason to add ASSERT here is to trigger it if the config is
>> disabled but an eSPI INTID is defined in Xen DTS.
>
> I will not insist on remove the ASSERT(). However, it could correct and
> we should avoid relying on ASSERT() to catch DTS bugs. Because...
>
Yes, I agree with that, but I just checked something else - I tried
using mainline Xen (without eSPI patches) and defined an invalid IRQ
(0x110a00) for a device in the Xen DTS:
interrupts = <0x00 0x110a00 0x04>;
And Xen crashed with a data abort while starting Dom0:
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading d0 kernel from boot module @ 000000007a000000
(XEN) Loading ramdisk from boot module @ 0000000055964000
(XEN) Grant table range: 0x00000078200000-0x00000078240000
(XEN) Allocating 1:1 mappings totalling 512MB for dom0:
(XEN) BANK[0] 0x00000068000000-0x00000078000000 (256MB)
(XEN) BANK[1] 0x000010d0000000-0x000010e0000000 (256MB)
(XEN) Data Abort Trap. Syndrome=0x6
(XEN) Walking Hypervisor VA 0xa0008b991a4 on CPU0 via TTBR
0x0000000078348000
(XEN) 0TH[0x014] = 0x78347f7f
(XEN) 1ST[0x000] = 0x78346f7f
(XEN) 2ND[0x045] = 0x0
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.21-unstable arm64 debug=y Not tainted ]----
(XEN) CPU: 0
(XEN) PC: 00000a00002285c8 _spin_lock+0x40/0xa4
(XEN) LR: 00000a00002285b0
(XEN) SP: 00000a0000326210
(XEN) CPSR: 00000000600002c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN) X0: 00000a0000330058 X1: 0000000000000001 X2: 0000000000000000
(XEN) X3: 0000000000000000 X4: 0000000000000000 X5: 00000a0000330130
(XEN) X6: 0000000000000000 X7: 0000800fbffdf9b0 X8: 7f7f7f7f7f7f7f7f
(XEN) X9: 0000000000000080 X10: 0101010101010101 X11: 0000000000000030
(XEN) X12: 0000000000000028 X13: ff00000000000000 X14: 0000000004000000
(XEN) X15: 0080000000000000 X16: 00000000000fffff X17: 0000000000000000
(XEN) X18: 00000000bbfefd20 X19: 00000a0008b991a4 X20: 0000000000010000
(XEN) X21: 00000a0008b991a8 X22: 0000000000000004 X23: 0000000000000000
(XEN) X24: 0000000000000000 X25: 0000800fbffcc980 X26: 0000000000000001
(XEN) X27: 0000000000173000 X28: 00000000481a8060 FP: 00000a0000326210
(XEN)
(XEN) VTCR_EL2: 00000000800d3590
(XEN) VTTBR_EL2: 0000000000000000
(XEN)
(XEN) SCTLR_EL2: 0000000030cd183d
(XEN) HCR_EL2: 0000000080000038
(XEN) TTBR0_EL2: 0000000078348000
(XEN)
(XEN) ESR_EL2: 0000000096000006
(XEN) HPFAR_EL2: 0000000000000000
(XEN) FAR_EL2: 00000a0008b991a4
(XEN)
....
(XEN) Xen call trace:
(XEN) [<00000a00002285c8>] _spin_lock+0x40/0xa4 (PC)
(XEN) [<00000a00002285b0>] _spin_lock+0x28/0xa4 (LR)
(XEN) [<00000a000022872c>] _spin_lock_irqsave+0x18/0x28
(XEN) [<00000a0000278e9c>] irq_set_spi_type+0x34/0x78
(XEN) [<00000a0000279034>] irq_set_type+0x154/0x16c
(XEN) [<00000a0000279074>] platform_get_irq+0x28/0x44
(XEN) [<00000a00002e188c>] domain_build.c#handle_node+0x100/0x7b0
(XEN) [<00000a00002e1dac>] domain_build.c#handle_node+0x620/0x7b0
(XEN) [<00000a00002e1dac>] domain_build.c#handle_node+0x620/0x7b0
(XEN) [<00000a00002e24e8>] construct_hwdom+0x3f4/0x4bc
(XEN) [<00000a00002e2650>] domain_build.c#construct_dom0+0xa0/0xb4
(XEN) [<00000a00002e273c>] create_dom0+0xd8/0x11c
(XEN) [<00000a00002e87e8>] start_xen+0x8bc/0x98c
(XEN) [<00000a00002001a4>] head.o#primary_switched+0x4/0x24
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ****************************************
Currently, Xen does not verify the validity of interrupt numbers defined
in the DTS file. This should definitely be addressed elsewhere and not
just for the eSPI range, but at least the ASSERT for eSPIs will not make
things worse. Perhaps the issue with IRQ number validation should be
fixed in a separate patch series. I will try to look into this issue
after eSPI and dynamic allocation for irq_desc_t array.
>> In this case, instead
>> of triggering an ASSERT (as proposed), the following will occur in
>> __irq_to_desc:
>>
>> // Assume we have irq = 4096
>> struct irq_desc *__irq_to_desc(unsigned int irq)
>> {
>> // This check will return false
>> if ( irq < NR_LOCAL_IRQS )
>> return &this_cpu(local_irq_desc)[irq];
>>
>> /*
>> * This check will also return false because is_espi()
>> * will always return false when CONFIG_GICV3_ESPI=n.
>> */
>> if ( is_espi(irq) )
>> return espi_to_desc(irq);
>>
>> /*
>> * We will fall through to this point and attempt to access 4064,
>> * which does not exist
>> */
>> return &irq_desc[irq-NR_LOCAL_IRQS];
>> }
>>
>> So, I think it's better to use ASSERT to simplify error detection in
>> debug builds.
>
> ... no everyone will use debug build. So if this is the purpose of the
> ASSERT() then we need to have another runtime check during the parsing
> of the DTS.
>
> Cheers,
>
Best regards,
Leonid
