Re: [PATCH v2 10/39] xen/riscv: build the target hart index via aplic_hart_field()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/9/26 4:52 PM, Jan Beulich wrote:
> On 27.08.2026 17:20, Oleksii Kurochko wrote:
> > @@ -340,27 +338,11 @@ static void cf_check aplic_set_irq_affinity(struct 
> > irq_desc *desc, const cpumask
> >   
> >       ASSERT(spin_is_locked(&desc->lock));
> >   
> > -    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
> > -    hhxw = imsic->group_index_bits;
> > -    lhxw = imsic->hart_index_bits;
> > -    /*
> > -     * Although this variable is used only once in the calculation of
> > -     * group_index, and it might seem that hhxs could be defined as:
> > -     *   hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT;
> > -     * and then the addition of IMSIC_MMIO_PAGE_SHIFT could be omitted
> > -     * when calculating the group index.
> > -     * It was done intentionally this way to follow the formula from
> > -     * the AIA specification for calculating the MSI address.
> > -     */
> > -    hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2;
> > -    base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT;
> > -
> > -    /* Update hart and EEID in the target register */
> > -    group_index = (base_ppn >> (hhxs + IMSIC_MMIO_PAGE_SHIFT)) &
> > -                  (BIT(hhxw, UL) - 1);
> > -    value = desc->irq;
> Hmm, only after sending the ack I noticed that there's no masking here, ...
>
> > -    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
> > -    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT);
> > +    cpu = aplic_get_cpu_from_mask(mask);
> > +
> > +    /* Update hart index and EIID in the target register */
> > +    value = MASK_INSR(aplic_hart_field(cpu), APLIC_TARGET_HART_IDX) |
> > +            (desc->irq & APLIC_TARGET_EIID);
> ... but there is masking here. Chopping off bits doesn't look as if it can
> lead to anything good. What's the deal here?
The mask is a no-op: desc->irq < NR_IRQS (1024) always fits the 11-bit 
EIID field, so I'll drop it and add a BUILD_BUG_ON() instead.
Would it be better to:

+    /* desc->irq < NR_IRQS, so it always fits the EIID field */
+    BUILD_BUG_ON(NR_IRQS - 1 > APLIC_TARGET_EIID);
+
+    value = MASK_INSR(aplic_hart_index(cpu), APLIC_TARGET_HART_IDX) | 
desc->irq;
?

Thanks.

~ Oleksii