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

[Julien Grall <julien@xxxxxxx>]

Hi Leonid,

On 02/09/2025 21:55, Leonid Komarianskyi wrote:
> > > > >             if ( rank == NULL ) goto write_ignore;
> > > > >             vgic_lock_rank(v, rank, flags);
> > > > >             tr = rank->ienable;
> > > > >             vreg_reg32_setbits(&rank->ienable, r, info);
> > > > > -        vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
> > > > > +        if ( reg >= GICD_ISENABLERnE )
> > > > > +            vgic_enable_irqs(v, (rank->ienable) & (~tr),
> > > > > +                             EXT_RANK_IDX2NUM(rank->index));
> > > > > +        else
> > > > > +            vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
> > > > ... you now have to keep both "if" the same. Unless we can have a
> > > > version to avoid "ifs" everywhere (maybe using macros), I would rather
> > > > create a separate funciton to handle eSPIs.
> > > >
> > > > Cheers,
> > > The main idea of V5 of this patch is to consolidate similar code and
> > > keep the pairs of regular SPIs and their eSPI counterparts in the same
> > > place to simplify any future modifications of these pairs. If
> > > eSPI-specific registers are moved to a separate function, it would
> > > result in code duplication. Additionally, I think it would be easier to
> > > miss updating the code for one register of the SPI/eSPI pair while
> > > modifying the code for the other..
> > I understand your reasoning, but in this case, we need to try to keep
> > the code as common as possible and reduce the number of ifs. Looking at
> > the patch again, we seem to often use "EXT_RANK_IDX2NUM(rank->index)"
> > and this is why we have the second "if", for instance here. I think it
> > would be preferable if "index" store the proper value.
> Actually, there are 4 specific cases where I need to use EXT_RANK_IDX2NUM:
> vgic_enable_irqs, vgic_disable_irqs, vgic_set_irqs_pending, and
> vgic_check_inflight_irqs_pending. The reason I made this transformation
> is that these functions are generic and calculate the virq based on the
> rank number, e.g. vgic_check_inflight_irqs_pending():
>
> unsigned int irq = i + 32 * rank;
>
> As a result, I decided to introduce EXT_RANK_IDX2NUM with ifs rather
> than modifying very generic code that would also affect vGICv2 and be
> more difficult to upstream..
I wasn't asking to modify the code in vgic_enable_irqs() & co. But 
rather how it is called.
Right now, rank->index for eSPI, will be starting at 0. But what if 
instead, it is starting at 128 (i.e. EXT_RANK_MIN)?
Effectively, rather than initializating the eSPI ranks with:

    for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
        vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i], i, 0);

You could do:

    for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
        vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i], 
EXT_RANK_IDX2NUM(i), 0);
This would remove all the "if"s and the "EXT_RANK_IDX2NUM(rank->index)".

Cheers,

--
Julien Grall