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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall