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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/10/26 1:23 PM, Jan Beulich wrote:
> On 10.09.2026 12:59, Oleksii Kurochko wrote:
> > 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);
> If the question is whether to prefer BUILD_BUG_ON() over BUG_ON(), then:
> Yes please. However, APLIC_TARGET_EIID is a mask (despite its name not
> indicating that), which only happens to start at bit 0. This kind of
> assumption would better be avoided.
Then BUILD_BUG_on should be updated to:

 /* desc->irq < NR_IRQS, so it always fits the EIID field */
    BUILD_BUG_ON(NR_IRQS - 1 > MASK_EXTR(~0U, APLIC_TARGET_EIID));


> This raises another question though: No matter how big a RISC-V system
> is, it can only ever have 1k IRQs? How does that work with a single
> MSI-X device having up to 2k MSIs?
Device MSIs never pass through the APLIC. They are written straight into 
an IMSIC interrupt file. The ID space belongs to each file, so each hart 
has up to 2047 IDs (IMSIC_MAX_ID).
1k it is limitation for wired interrupts (which could be delivered in 
MSI mode where APLIC + IMSIC is needed) which are going through APLIC.
~ Oleksii