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 18:26, Leonid Komarianskyi wrote:
> Hi Julien,
>
> Thank you for your review and suggestions.
>
> On 02.09.25 19:55, Julien Grall wrote:
> > Hi Leonid,
> >
> > On 29/08/2025 17:06, Leonid Komarianskyi wrote:
> > > @@ -782,46 +804,81 @@ static int
> > > __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
> > >        {
> > >        case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
> > >        case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
> > > +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
> > > +    case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
> > >            /* We do not implement security extensions for guests, write
> > > ignore */
> > >            goto write_ignore_32;
> > >        case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
> > > +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
> > >            if ( dabt.size != DABT_WORD ) goto bad_width;
> > > -        rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
> > > +        if ( reg >= GICD_ISENABLERnE )
> > > +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
> > > +                                        DABT_WORD);
> > > +        else
> > > +            rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER,
> > > DABT_WORD);
> > While I understand the desire to try to avoid code duplication. I feel
> > this is making the code a lot more complicating and in fact riskier
> > because...
> >
> > >            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.
An alternative would be to process the 3rd parameters separately.

The next big one to takle is:

if ( reg >= ... )
   vgic_ext_rank_...(...)
else
   vgic_rank(...)

Ideally I would like to provide an helper that can figure out whether 
this is an eSPI or not. Looking at the pattern, I think we would 
introduce a new helper which rather than taking a relative offset take 
the reg offset, the base for SPIs and the base for eSPIs.
Cheers,

--
Julien Grall