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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 04.09.25 08:52, Leonid Komarianskyi wrote:
> Hi Julien, Volodymyr and Oleksandr,

Hello all

> 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.
I was leaning more towards the macro, but would be happy with static 
inline helper (my R-b will stay).
I guess, the suggested ASSERT() for the static inline
helper vgic_get_reg_offset would be also applicable for helper 
vgic_get_rank (or whatever name it will gain).
> Best regards,
> Leonid