Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> imsic_init() is introduced to parse device tree node, which has the following
> bindings [2], and based on the parsed information update IMSIC configuration
> which is stored in imsic_cfg.
> 
> The following helpers are introduces for imsic_init() usage:
>   - imsic_parse_node() parses IMSIC node from DTS
>   - imsic_get_parent_cpuid() returns the hart ( CPU ) ID of the given device
>     tree node.
> 
> This patch is based on the code from [1].
> 
> Since Microchip originally developed imsic.{c,h}, an internal discussion with
> them led to the decision to use the MIT license.
> 
> [1] 
> https://gitlab.com/xen-project/people/olkur/xen/-/commit/0b1a94f2bc3bb1a81cd26bb75f0bf578f84cb4d4
> [2] 
> https://elixir.bootlin.com/linux/v6.12/source/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> 
> Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V2:
>  - Drop years in copyrights.

Did you, really?

>  - s/riscv_of_processor_hartid/dt_processor_cpuid.
>  - s/imsic_get_parent_hartid/imsic_get_parent_cpuid.
>    Rename argument hartid to cpuid.
>    Make node argument const.
>    Return res instead of -EINVAL for the failure case of dt_processor_cpuid().
>    Drop local variable hart and use cpuid argument instead.
>    Drop useless return res;
>  - imsic_parse_node() changes:
>    - Make node argument const.
>    - Check the return value of dt_property_read_u32() directly instead of
>      saving it to rc variable.
>    - Update tmp usage, use short form "-=".
>    - Update a check (imsic_cfg.nr_ids >= IMSIC_MAX_ID) to (imsic_cfg.nr_ids > 
> IMSIC_MAX_ID)
>      as IMSIC_MAX_ID is changed to maximum valid value, not just the firsr 
> out-of-range.
>    - Use `rc` to return value instead of explicitly use -EINVAL.
>    - Use do {} while() to find number of MMIO register sets.
>  - Set IMSIC_MAX_ID to 2047 (maximum possible IRQ number).
>  - imsic_init() changes:
>    - Use unsigned int in for's expression1.
>    - s/xfree/XFEE.
>    - Allocate msi and cpus array dynamically.
>  - Drop forward declaration before declaration of imsic_get_config() in 
> asm/imsic.h
>    as it is not used as parameter type.
>  - Align declaration of imisic_init with defintion.
>  - s/harts/cpus in imisic_mmios.
>    Also, change type from bool harts[NR_CPUS] to unsigned long *cpus.
>  - Allocate msi member of imsic_config dynamically to save some memory.
>  - Code style fixes.
>  - Update the commit message.
> ---
>  xen/arch/riscv/Makefile            |   1 +
>  xen/arch/riscv/imsic.c             | 286 +++++++++++++++++++++++++++++
>  xen/arch/riscv/include/asm/imsic.h |  65 +++++++
>  3 files changed, 352 insertions(+)
>  create mode 100644 xen/arch/riscv/imsic.c
>  create mode 100644 xen/arch/riscv/include/asm/imsic.h
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index a1c145c506..e2b8aa42c8 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -2,6 +2,7 @@ obj-y += aplic.o
>  obj-y += cpufeature.o
>  obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-y += entry.o
> +obj-y += imsic.o
>  obj-y += intc.o
>  obj-y += irq.o
>  obj-y += mm.o
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> new file mode 100644
> index 0000000000..43d0c92cbd
> --- /dev/null
> +++ b/xen/arch/riscv/imsic.c
> @@ -0,0 +1,286 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/imsic.c
> + *
> + * RISC-V Incoming MSI Controller support
> + *
> + * (c) Microchip Technology Inc.
> + * (c) Vates
> + */
> +
> +#include <xen/const.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/macros.h>
> +#include <xen/xmalloc.h>
> +
> +#include <asm/imsic.h>
> +
> +static struct imsic_config imsic_cfg;
> +
> +/* Callers aren't expected to changed imsic_cfg so return const. */
> +const struct imsic_config *imsic_get_config(void)
> +{
> +    return &imsic_cfg;
> +}
> +
> +static int __init imsic_get_parent_cpuid(const struct dt_device_node *node,
> +                                         unsigned int index,
> +                                         unsigned long *cpuid)
> +{
> +    int res;
> +    struct dt_phandle_args args;
> +
> +    res = dt_parse_phandle_with_args(node, "interrupts-extended",
> +                                     "#interrupt-cells", index, &args);
> +    if ( !res )
> +        res = dt_processor_cpuid(args.np->parent, cpuid);
> +
> +    return res;
> +}
> +
> +static int imsic_parse_node(const struct dt_device_node *node,
> +                            unsigned int *nr_parent_irqs)
> +{
> +    int rc;
> +    unsigned int tmp;
> +    paddr_t base_addr;
> +
> +    /* Find number of parent interrupts */
> +    *nr_parent_irqs = dt_number_of_irq(node);
> +    if ( !*nr_parent_irqs )
> +    {
> +        printk(XENLOG_ERR "%s: no parent irqs available\n", node->name);
> +        return -ENOENT;
> +    }
> +
> +    if ( !dt_property_read_u32(node, "riscv,guest-index-bits",
> +                               &imsic_cfg.guest_index_bits) )
> +        imsic_cfg.guest_index_bits = 0;
> +    tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT;
> +    if ( tmp < imsic_cfg.guest_index_bits )
> +    {
> +        printk(XENLOG_ERR "%s: guest index bits too big\n", node->name);
> +        return -ENOENT;
> +    }
> +
> +    /* Find number of HART index bits */
> +    if ( !dt_property_read_u32(node, "riscv,hart-index-bits",
> +                               &imsic_cfg.hart_index_bits) )
> +    {
> +        /* Assume default value */
> +        imsic_cfg.hart_index_bits = fls(*nr_parent_irqs);
> +        if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs )
> +            imsic_cfg.hart_index_bits++;
> +    }
> +    tmp -= imsic_cfg.guest_index_bits;
> +    if ( tmp < imsic_cfg.hart_index_bits )
> +    {
> +        printk(XENLOG_ERR "%s: HART index bits too big\n", node->name);
> +        return -ENOENT;
> +    }
> +
> +    /* Find number of group index bits */
> +    if ( !dt_property_read_u32(node, "riscv,group-index-bits",
> +                               &imsic_cfg.group_index_bits) )
> +        imsic_cfg.group_index_bits = 0;
> +    tmp -= imsic_cfg.hart_index_bits;
> +    if ( tmp < imsic_cfg.group_index_bits )
> +    {
> +        printk(XENLOG_ERR "%s: group index bits too big\n", node->name);
> +        return -ENOENT;
> +    }
> +
> +    /* Find first bit position of group index */
> +    tmp = IMSIC_MMIO_PAGE_SHIFT * 2;
> +    if ( !dt_property_read_u32(node, "riscv,group-index-shift",
> +                               &imsic_cfg.group_index_shift) )
> +        imsic_cfg.group_index_shift = tmp;
> +    if ( imsic_cfg.group_index_shift < tmp )
> +    {
> +        printk(XENLOG_ERR "%s: group index shift too small\n", node->name);
> +        return -ENOENT;
> +    }
> +    tmp = imsic_cfg.group_index_bits + imsic_cfg.group_index_shift - 1;
> +    if ( tmp >= BITS_PER_LONG )
> +    {
> +        printk(XENLOG_ERR "%s: group index shift too big\n", node->name);
> +        return -EINVAL;
> +    }
> +
> +    /* Find number of interrupt identities */
> +    if ( !dt_property_read_u32(node, "riscv,num-ids", &imsic_cfg.nr_ids) )
> +    {
> +        printk(XENLOG_ERR "%s: number of interrupt identities not found\n",
> +               node->name);
> +        return -ENOENT;
> +    }
> +
> +    if ( (imsic_cfg.nr_ids < IMSIC_MIN_ID) ||
> +         (imsic_cfg.nr_ids > IMSIC_MAX_ID) ||
> +         ((imsic_cfg.nr_ids & IMSIC_MIN_ID) != IMSIC_MIN_ID) )
> +    {
> +        printk(XENLOG_ERR "%s: invalid number of interrupt identities\n",
> +               node->name);
> +        return -EINVAL;
> +    }
> +
> +    /* Compute base address */
> +    imsic_cfg.nr_mmios = 0;
> +    rc = dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "%s: first MMIO resource not found\n", node->name);
> +        return rc;
> +    }
> +
> +    imsic_cfg.base_addr = base_addr;
> +    imsic_cfg.base_addr &= ~(BIT(imsic_cfg.guest_index_bits +
> +                           imsic_cfg.hart_index_bits +
> +                           IMSIC_MMIO_PAGE_SHIFT, UL) - 1);

Nit: indentation, similarly ...

> +    imsic_cfg.base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
> +                           imsic_cfg.group_index_shift);

... here.

> +    /* Find number of MMIO register sets */
> +    do {
> +        imsic_cfg.nr_mmios++;
> +    } while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, 
> NULL) );
> +
> +    return 0;
> +}
> +
> +int __init imsic_init(const struct dt_device_node *node)
> +{
> +    int rc;
> +    unsigned long reloff, cpuid;
> +    uint32_t nr_parent_irqs, index, nr_handlers = 0;

I can't spot why unsigned int wouldn't be suitable here. In fact e.g. ...

> +    paddr_t base_addr;
> +    unsigned int nr_mmios;
> +
> +    /* Parse IMSIC node */
> +    rc = imsic_parse_node(node, &nr_parent_irqs);

... this one wants to yield unsigned int * according to the function parameter's
type.

> +    if ( rc )
> +        return rc;
> +
> +    nr_mmios = imsic_cfg.nr_mmios;
> +
> +    /* Allocate MMIO resource array */
> +    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios);
> +    if ( !imsic_cfg.mmios )
> +        return -ENOMEM;
> +
> +    imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs);
> +    if ( !imsic_cfg.msi )
> +        return -ENOMEM;

Leaking the earlier successful allocation?

> +    /* Check MMIO register sets */
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +    {
> +        imsic_cfg.mmios[i].cpus = xzalloc_array(unsigned long,
> +                                                
> BITS_TO_LONGS(nr_parent_irqs));
> +        if ( !imsic_cfg.mmios[i].cpus )
> +            return -ENOMEM;

Leaking all earlier successful allocations?

> +        rc = dt_device_get_address(node, i, &imsic_cfg.mmios[i].base_addr,
> +                                   &imsic_cfg.mmios[i].size);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "%s:  unable to parse MMIO regset %d\n",

Nit: Excess blank.

> +                   node->name, i);
> +            goto imsic_init_err;
> +        }
> +
> +        base_addr = imsic_cfg.mmios[i].base_addr;
> +        base_addr &= ~(BIT(imsic_cfg.guest_index_bits +
> +                     imsic_cfg.hart_index_bits +
> +                     IMSIC_MMIO_PAGE_SHIFT, UL) - 1);
> +        base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
> +                     imsic_cfg.group_index_shift);

As above, indentation again.

> +        if ( base_addr != imsic_cfg.base_addr )
> +        {
> +            rc = -EINVAL;
> +            printk(XENLOG_ERR "%s: address mismatch for regset %d\n",
> +                   node->name, i);
> +            goto imsic_init_err;
> +        }
> +    }
> +
> +    /* Configure handlers for target CPUs */
> +    for ( unsigned int i = 0; i < nr_parent_irqs; i++ )
> +    {
> +        rc = imsic_get_parent_cpuid(node, i, &cpuid);

Coming back to a comment I gave on the respective earlier patch: What you get 
back
here is a DT value, aiui. There's no translation to Xen CPU numbering space, as
would be required for e.g. ...

> +        if ( rc )
> +        {
> +            printk(XENLOG_WARNING "%s: cpu ID for parent irq%d not found\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( cpuid >= num_possible_cpus() )

... this. Are you using DT numbering without any translation, no matter that it
(I suppose) could be very sparse?

> +        {
> +            printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent 
> irq%d\n",
> +                   node->name, cpuid, i);

Nit: i is unsigned int, so wants formatting with %u (also applicable elsewhere).

> +            continue;
> +        }
> +
> +        /* Find MMIO location of MSI page */
> +        index = nr_mmios;
> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * 
> IMSIC_MMIO_PAGE_SZ;
> +        for ( unsigned int j = 0; nr_mmios; j++ )

DYM "j < nr_mmios"?

> +        {
> +            if ( reloff < imsic_cfg.mmios[j].size )
> +            {
> +                index = j;
> +                break;
> +            }
> +
> +            /*
> +             * MMIO region size may not be aligned to
> +             * BIT(global->guest_index_bits) * IMSIC_MMIO_PAGE_SZ
> +             * if holes are present.
> +             */
> +            reloff -= ROUNDUP(imsic_cfg.mmios[j].size,
> +                BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ);
> +        }
> +
> +        if ( index >= nr_mmios )

Why is it that you need both "index" and "j"?

> +        {
> +            printk(XENLOG_WARNING "%s: MMIO not found for parent irq%d\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( !IS_ALIGNED(imsic_cfg.msi[cpuid].base_addr + reloff, PAGE_SIZE) 
> )
> +        {
> +            printk(XENLOG_WARNING "%s: MMIO address 0x%lx is not aligned on 
> a page\n",

Please prefer to use %#lx, as we do elsewhere.

> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
> +            imsic_cfg.msi[cpuid].offset = 0;
> +            imsic_cfg.msi[cpuid].base_addr = 0;
> +            continue;
> +        }
> +
> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);

Depending on clarification on the number space used, this may want to be
cpumask_set_cpu() instead. Else I think this is simply __set_bit().

> +        imsic_cfg.msi[cpuid].base_addr = imsic_cfg.mmios[index].base_addr;
> +        imsic_cfg.msi[cpuid].offset = reloff;

How come it's cpuid that's used as array index here? Shouldn't this be i,
seeing that the array allocation is using nr_parent_irqs?

> +        nr_handlers++;
> +    }
> +
> +    if ( !nr_handlers )
> +    {
> +        printk(XENLOG_ERR "%s: No CPU handlers found\n", node->name);
> +        rc = -ENODEV;
> +        goto imsic_init_err;
> +    }
> +
> +    return 0;
> +
> + imsic_init_err:
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +        XFREE(imsic_cfg.mmios[i].cpus);

This can be just xfree(), as the array itself ...

> +    XFREE(imsic_cfg.mmios);

... is then also freed.

> +    XFREE(imsic_cfg.msi);
> +
> +    return rc;
> +}
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/imsic.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/imsic.h
> + *
> + * RISC-V Incoming MSI Controller support
> + *
> + * (c) 2023 Microchip Technology Inc.
> + */
> +
> +#ifndef ASM__RISCV__IMSIC_H
> +#define ASM__RISCV__IMSIC_H
> +
> +#include <xen/types.h>
> +
> +#define IMSIC_MMIO_PAGE_SHIFT   12
> +#define IMSIC_MMIO_PAGE_SZ      (1UL << IMSIC_MMIO_PAGE_SHIFT)
> +
> +#define IMSIC_MIN_ID            63

This isn't the "minimum ID", but the "minimum permissible number of IDs" as per
its first use in imsic_parse_node(). Further uses there consider it a mask,
which makes me wonder whether the imsic_cfg.nr_ids field name is actually
correct: Isn't this the maximum valid ID rather than their count/number?

Jan