Re: [PATCH v3 09/39] xen/riscv: build the target hart index via aplic_hart_index()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/6/26 11:02 AM, Baptiste Le Duc wrote:
> > aplic_set_irq_affinity() open-coded the packing of the group and hart
> > indices into the target register, and got two things wrong along the
> > way:
> >
> >   - imsic_config.msi[] is indexed by logical CPU id, but the index was
> >     run through cpuid_to_hartid() first. On any platform where the two
> >     spaces differ this picks another CPU's interrupt file, or reads past
> >     the array;
> >
> >   - the same hart id was then used verbatim as the low hart index, and
> >     the group index was derived from msi[].base_addr alone. The hart
> >     index bits live in msi[].offset, the base address only covers the
> >     MMIO regset, which may hold the files of several harts. Both indices
> >     have to come out of base_addr + offset.
> >
> > aplic_hart_index() already extracts them that way, and is what the vAPLIC
> > target path uses, so call it here as well and insert the result with
> > MASK_INSR(APLIC_TARGET_HART_IDX) instead of a bare shift, which keeps the
> > value from spilling out of the 14-bit field. This drops the last in-tree
> > duplicate of the AIA hart index formula, and with it the last user of
> > APLIC_TARGET_HART_IDX_SHIFT, which is removed.
> >
> > desc->irq is used as the EIID as is. Assert that it is a valid APLIC
> > source, which also guarantees that it fits the EIID field.
> >
> > No functional change on a single-group platform whose hart ids match
> > their CPU ids and whose IMSIC regset holds one file per hart.
> Nit: The message is hard to follow because it describes the old code and
> its bugs in the same sentences, so you never see a plain picture of what
> the code did before. I would propose this:
>      ```
>      aplic_set_irq_affinity() computes the target register value by hand:
>
>          cpu         = cpuid_to_hartid(<cpu picked from mask>);
>          group_index = <bits taken from msi[cpu].base_addr>;
>          hart_index  = cpu;
>
>      This is wrong in three ways:
>
>      - msi[] is indexed by logical CPU id, not by hart id. When the two
>        differ, the wrong CPU's interrupt file is used, or msi[] is read
>        out of bounds.
>
>      - The hart id is used as the low hart index. The AIA spec defines
>        the low hart index as bits of the IMSIC interrupt file address,
>        and the two are not guaranteed to be equal.
>
>      - The group index is taken from msi[].base_addr only. base_addr is
>        the start of the IMSIC MMIO region, which may hold the interrupt
>        files of several harts; the file of this CPU is at base_addr +
>        msi[].offset, and both indexes must be extracted from that
>        address.
>
>      aplic_hart_index() already does this correctly and is used by the
>      vAPLIC target path. Use it here as well, and insert the result with
>      MASK_INSR(APLIC_TARGET_HART_IDX) rather than a plain shift, so it
>      cannot overflow the 14-bit field. APLIC_TARGET_HART_IDX_SHIFT has no
>      users left and is removed.
>
>      desc->irq is written as the EIID unchanged. Assert that it is a
>      valid APLIC source number, which also ensures it fits in the EIID
>      field.
>
>      No functional change on platforms with a single group, hart ids
>      equal to CPU ids, and one interrupt file per IMSIC MMIO region.
>      ```
Thanks, that reads much better. I'll take it.

> > Fixes: d4676a1398bc ("xen/riscv: implementation of aplic and imsic operations")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v3:
> >   - s/aplic_hart_field/aplic_hart_index/ (in the subject too) following the
> >     rename of the helper in "xen/riscv: implement virtual APLIC MMIO
> >     emulation".
> >   - Don't mask desc->irq with APLIC_TARGET_EIID. Instead, bound it by
> >     the APLIC source range rather than NR_IRQS, which will also have to
> >     cover device MSIs: add a BUILD_BUG_ON() that ARRAY_SIZE(target) fits
> >     the EIID field and an ASSERT() that desc->irq is a valid APLIC source
> >     (1..num_irqs), which also covers the target[desc->irq - 1] index.
> > ---
> > Changes in v2:
> >   - New patch.
> > ---
> > ---
> >   xen/arch/riscv/aplic.c             | 33 ++++++++++--------------------
> >   xen/arch/riscv/include/asm/aplic.h |  1 -
> >   2 files changed, 11 insertions(+), 23 deletions(-)
> >
> > diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
> > index febc451760..62bbcc9afa 100644
> > --- a/xen/arch/riscv/aplic.c
> > +++ b/xen/arch/riscv/aplic.c
> > @@ -335,9 +335,7 @@ static unsigned int aplic_get_cpu_from_mask(const cpumask_t 
> > *cpumask)
> >   static void cf_check aplic_set_irq_affinity(struct irq_desc *desc, const 
> > cpumask_t *mask)
> >   {
> >       unsigned int cpu;
> > -    uint64_t group_index, base_ppn;
> > -    uint32_t hhxw, lhxw, hhxs, value;
> > -    const struct imsic_config *imsic = aplic.imsic_cfg;
> > +    uint32_t value;
> >   
> >       /*
> >        * TODO: Currently, APLIC is supported only with MSI interrupts.
> > @@ -350,27 +348,18 @@ 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;
> > +    cpu = aplic_get_cpu_from_mask(mask);
> > +
> >       /*
> > -     * 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.
> > +     * desc->irq is used as the EIID as-is, which is only valid for APLIC
> > +     * sources: they are numbered 1..num_irqs, with at most ARRAY_SIZE(target).
> >        */
> > -    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;
> > -    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
> > -    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT);
> > +    BUILD_BUG_ON(ARRAY_SIZE(aplic.regs->target) >
> > +                 MASK_EXTR(~0U, APLIC_TARGET_EIID));
> Shouldn't this function belong to aplic_init() or at least in a __init
> function? Moreover, this check is only relevant in case of MSI delivery
> mode but I agree it worth checking as we couldn't know at build time in
> which delivery we would be.
BUILD_BUG_ON() is evaluated at build time only, so there's no run-time
cost wherever it is placed, and an __init function wouldn't buy anything.
I'd rather keep it here, next to the ASSERT() (ASSERT(desc->irq && 
desc->irq <= aplic_info.num_irqs);) and the code relying on
both: together they are what allows desc->irq to be used as the EIID
without masking it. This function is MSI-only anyway (see the ASSERT()
on domaincfg.DM above), so the check already sits in an MSI-only
context, whereas moving it, for example, after the msi-parent check in 
aplic_init() would make it look conditional on the delivery mode, which 
it can't be.
Thanks.

~ Oleksii