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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall