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

On 03.09.25 14:26, Julien Grall wrote:
> 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,
> 

Yesterday evening, I realized the same :) I fixed this while preparing 
the next version of the patch series. Also, I found a way to remove many 
ifs in this patch by introducing just 2 helper functions. I will push v6 
soon with these updates. I hope it will look much better.

Best regards,
Leonid