Re: [PATCH 04/10] xen/arm/irq: allow assignment/releasing of eSPI interrupts

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,

On 29.07.25 16:56, Julien Grall wrote:
> Hi,
> 
> On 24/07/2025 15:57, Leonid Komarianskyi wrote:
>> The current checks don't allow us to assign or release interrupts
>> with INTID greater than 1024. This patch adds an additional condition
>> to check whether the IRQ number is in the eSPI range and allows it to
>> be assigned to Xen and domains if it is.
>  > > Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>> ---
>>   xen/arch/arm/irq.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 3f68257fde..8c47eeb7c3 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -444,8 +444,8 @@ 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());
>> +    /* For now, we can only route SPIs and eSPIs to the guest */
>> +    return (((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())) || 
>> is_espi(irq));
>>   }
> 
> is_assignable_irq() is called by route_irq_to_guest() which first check 
> 'virq >= vgic_num_irqs()'. AFAICT, if we apply only up to this patch, 
> 'virq' would still require to < 1024. So ...

That is my mistake when splitting the changes into separate patches. I 
will at least reorder the patches to make the changes more logical, or, 
based on your comments for the 6th patch in the series, I will try to 
refactor it with a helper function instead.

>>   be
>>   /*
>> @@ -589,8 +589,8 @@ int release_guest_irq(struct domain *d, unsigned 
>> int virq)
>>       unsigned long flags;
>>       int ret;
>> -    /* Only SPIs are supported */
>> -    if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
>> +    /* Only SPIs and eSPIs are supported */
>> +    if ( (virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d)) && ! 
>> is_espi(virq) )
> 
> ... I don't quite understand why this (yet?) need a change. Can you 
> clarify? ...

This is necessary to allow releasing eSPI IRQs when destroying the 
domain. And yes, this is the same mistake on my part - the patches are 
in the wrong order, and I will refactor them to fix that and make 
changes more logical and straightforward.

Thank you for your comments.

Cheers,
Leonid