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
