Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien, Volodymyr and Oleksandr,

Thank you for your comments.

On 04.09.25 02:04, Julien Grall wrote:
> Hi,
> 
> On 03/09/2025 22:37, Volodymyr Babchuk wrote:
>> Hi Oleksandr,
>>
>> Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> writes:
>>
>>
>> [...]
>>
>>>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t 
>>>> spi_base,
>>>> +                                           uint32_t espi_base)
>>>> +{
>>>> +    if ( reg < espi_base )
>>>> +        return reg - spi_base;
>>>> +    else
>>>> +        return reg - espi_base;
>>>> +}
>>>
>>> I am wondering (I do not request a change) whether
>>> vgic_get_reg_offset() is really helpfull,
>>> e.g. is
>>>   offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
>>> much better than:
>>>   offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
>>>   GICD_IPRIORITYRnE;
>  >>>
>> IMO, it is easy to make a mistake, because you need to write register
>> name 3 times. Can cause errors during copy-pasting.
> 
> +1.
> 
>   But I saw clever
>> trick by Mykola Kvach, something like this:
>>
>> #define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
>>   addr - reg_name : addr - reg_name##nE )
>>
>> And then you can just use this as
>>
>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
>>
>> I don't know what maintainers think about this type of preprocessor
>> trickery, but in my opinion it is justified in this case, because it
>> leaves less room for a mistake.
> 
> I don't have a strong opinion between the macro version or the static 
> inline helper. However:
>    * for the macro version, you want to store 'addr' in a local variable 
> to ensure it is only evaluated once.
>    * for both case, I would prefer if we assert (for the static inline 
> helper) or use BUILD_BUG_ON() to confirm that spi_base < espi_base
> 
> Cheers,
> 

I was considering introducing this kind of macro, but I think it may 
lead to issues in the future because it requires us to always maintain 
the pattern reg_name/reg_name##nE for all registers. I understand that 
the names of the defines are unlikely to change, but I would prefer to 
use an inline function along with the suggested ASSERT(), as it seems 
more versatile to me.

Best regards,
Leonid