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
