Re: [PATCH v5 04/12] xen/arm/irq: add handling for IRQs in the eSPI range

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Julien, Volodymyr and Oleksandr,

On 01.09.25 19:16, Julien Grall wrote:
> Hi,
> 
> On 01/09/2025 15:42, Leonid Komarianskyi wrote:
>>>> Taking into account that with CONFIG_GICV3_ESPI=n we should never have
>>>> "irq" in eSPI range, do you really need this #ifdef? I think that
>>>> ASSERT_UNREACHABLE in espi_to_desc() is sufficient guard.
>>>>
>>>> Also, IRQ line number belongs to eSPI range regardless of
>>>> CONFIG_GICV3_ESPI,
>>>> value, so in my opinion is_espi() should always return correct value 
>>>> for
>>>> a given "irq".
>>>
>>>    ... I agree with Volodymyr's suggestion for is_espi() to always 
>>> return
>>> correct value for a given "irq".
>>>
>>>
>>
>> I will fix that in V6.
> 
> I am not sure about this. If is_espi() is not returning false with 
> CONFIG_GICV3_EPSI, then the compiler would not be able to optimize code 
> like:
> 
> if (is_espi(...)) {
>     return espi_to_desc(irq);
> }
> 
> return &irq_desc[irq-NR_LOCAL_IRQS];
> 
> irq_to_desc() is called fairly often, so I would like to keep the code 
> fairly optimized. An alternative would be to use #ifdef CONFIG_*. I 
> don't like it, but it could be a compromise if Oleksandr and Volodymyr 
> wants to push to remove #ifdef from CONFIG_IS_ESPI.
> 
> Cheers,
> 

I am just thinking about a possible compromise between writing logical 
code where the function name reflects its actual functionality and 
allowing for compiler optimization. Perhaps it would be better to keep 
the #ifdef but rename the function to `is_valid_espi()` or something 
similar.

In that case, I think there would be less confusion, as it seems 
reasonable for a function with such a name to return false when 
CONFIG_GICV3_ESPI=n, and also the compiler would be able to optimize the 
code.

What do you think about this approach?

Best regards,
Leonid