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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,

On 02.09.25 23:20, Julien Grall wrote:
> 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.
>

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

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

Hm, that's a good idea. I will try to implement this. I agree that it 
will reduce the boilerplate code.

Best regards,
Leonid