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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

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..

So, I will revise the code and try to avoid ifs where possible.

Best regards,
Leonid