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 2:57 PM, Jan Beulich wrote:
> On 10.09.2026 14:44, Oleksii Kurochko wrote:
> > 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.
> But desc->irq and NR_IRQS have to represent both. Which then puts the
> BUILD_BUG_ON() above under question.
Agreed. Right now desc->irq, the APLIC source number and the IMSIC EIID
are all the same number: aplic_set_irq_affinity() writes desc->irq into
EIID, and aplic_handle_interrupt() hands the IMSIC identity from stopei
straight to do_IRQ(). That only works because there's no MSI support on
RISC-V yet, so NR_IRQS covers wired interrupts only.

Once device MSIs are supported, that identity mapping can't stay anyway.
IMSIC IDs are per interrupt file and shared between wired and MSI
interrupts, so we'll need to allocate EIIDs and map desc->irq to
(hart, EIID). target[] will then get the allocated EIID rather than
desc->irq, and NR_IRQS will grow beyond 1024.

So for this patch I'll tie the check to the APLIC source range instead 
of NR_IRQS:
BUILD_BUG_ON(ARRAY_SIZE(aplic.regs->target) >
             MASK_EXTR(~0U, APLIC_TARGET_EIID));
ASSERT(desc->irq && desc->irq <= aplic_info.num_irqs);

The ASSERT also covers the target[desc->irq - 1] indexing.

~ Oleksii