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