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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> 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.
    ```
> 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.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>