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
