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

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

On 04.09.25 00:37, Volodymyr Babchuk wrote:
> Hi Oleksandr,

Hello Volodymyr

> 
> Oleksandr Tyshchenko <olekstysh@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.

I got it

  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)

 From my PoV, the idea looks good

> 
> 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.
> 
>