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 <olekst...@gmail.com> 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
