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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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


-- 
WBR, Volodymyr