Re: [PATCH v1 08/14] xen/riscv: imsic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> --- /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) 2023 Microchip Technology Inc.
> + * (c) 2024 Vates

No 2025 here (if already the years matter)?

> + */
> +
> +#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;
> +
> +const struct imsic_config *imsic_get_config(void)

Does this need to return a pointer to non-const?

> +{
> +    return &imsic_cfg;
> +}
> +
> +static int __init imsic_get_parent_hartid(struct dt_device_node *node,
> +                                          unsigned int index,
> +                                          unsigned long *hartid)
> +{
> +    int res;
> +    unsigned long hart;
> +    struct dt_phandle_args args;
> +
> +    /* Try the new-style interrupts-extended first */

The comment says "first", but then ...

> +    res = dt_parse_phandle_with_args(node, "interrupts-extended",
> +                                     "#interrupt-cells", index, &args);
> +    if ( !res )
> +    {
> +        res = riscv_of_processor_hartid(args.np->parent, &hart);
> +        if ( res < 0 )
> +            return -EINVAL;
> +
> +        *hartid = hart;
> +    }
> +    return res;
> +}

... nothing else is being tried.

Also, nit: Blank line please ahead of the main "return" of a function.

Further - any particular reason to discard riscv_of_processor_hartid()'s
error code on the error path?

> +
> +

Nit: No double blank lines please (and I wish I wouldn't need to repeat
this any further).

> +static int imsic_parse_node(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;
> +    }
> +
> +    /* Find number of guest index bits in MSI address */
> +    rc = dt_property_read_u32(node, "riscv,guest-index-bits",
> +                              &imsic_cfg.guest_index_bits);
> +    if ( !rc )

It is confusing to store a bool return value in a local "int" variable,
just to then use it as boolean. Is the local var needed at all here?

> +        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 */
> +    rc = dt_property_read_u32(node, "riscv,hart-index-bits",
> +                              &imsic_cfg.hart_index_bits);
> +    if ( !rc )
> +    {
> +        /* 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 = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT -
> +          imsic_cfg.guest_index_bits;

    tmp -= imsic_cfg.guest_index_bits;

? (And then similarly further down.)

> +    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 */
> +    rc = dt_property_read_u32(node, "riscv,group-index-bits",
> +                              &imsic_cfg.group_index_bits);
> +    if ( !rc )
> +        imsic_cfg.group_index_bits = 0;
> +    tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT -
> +          imsic_cfg.guest_index_bits - 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;
> +    rc = dt_property_read_u32(node, "riscv,group-index-shift",
> +                              &imsic_cfg.group_index_shift);
> +    if ( !rc )
> +        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 */
> +    rc = dt_property_read_u32(node, "riscv,num-ids", &imsic_cfg.nr_ids);
> +    if ( !rc )
> +    {
> +        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) ||

Something named "max" normally wants to decribe the highest valid value,
not the first out-of-range one.

> +         ((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)

Nit: Style.

> +    {
> +        printk(XENLOG_ERR "%s: first MMIO resource not found\n", node->name);
> +        return -EINVAL;

Discarding "rc" again?

> +    }
> +
> +    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);
> +    imsic_cfg.base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
> +                   imsic_cfg.group_index_shift);

Besides indentation being bogus here, why is it that you need to mask bits
off of the value read from DT? Wouldn't the expectation be that you get back
the true base address?

> +    /* Find number of MMIO register sets */
> +    imsic_cfg.nr_mmios++;
> +    while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, 
> NULL) )
> +        imsic_cfg.nr_mmios++;

And the base addresses of these aren't of interest? Oh, I see they're
fetched again further down.

Also - use do-while here?

> +    return 0;
> +}
> +
> +int __init imsic_init(struct dt_device_node *node)
> +{
> +    int rc;
> +    unsigned long reloff, hartid;
> +    uint32_t nr_parent_irqs, index, nr_handlers = 0;
> +    paddr_t base_addr;
> +
> +    /* Parse IMSIC node */
> +    rc = imsic_parse_node(node, &nr_parent_irqs);
> +    if ( rc )
> +        return rc;
> +
> +    /* Allocate MMIO resource array */
> +    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, imsic_cfg.nr_mmios);
> +    if ( !imsic_cfg.mmios )
> +        return -ENOMEM;
> +
> +    /* check MMIO register sets */
> +    for ( int i = 0; i < imsic_cfg.nr_mmios; i++ )

No plain int please for anything that can only be non-negative.

> +    {
> +        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",
> +                   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);

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;
> +        }

Oh, all of the addresses need to (sufficiently) match.

> +    }
> +
> +    /* Configure handlers for target CPUs */
> +    for ( int i = 0; i < nr_parent_irqs; i++ )
> +    {
> +        rc = imsic_get_parent_hartid(node, i, &hartid);
> +        if ( rc )
> +        {
> +            printk(XENLOG_WARNING "%s: hart ID for parent irq%d not found\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( hartid > NR_CPUS )
> +        {
> +            printk(XENLOG_WARNING "%s: unsupported hart ID=%lu for parent 
> irq%d\n",
> +                   node->name, hartid, i);
> +            continue;
> +        }
> +
> +        /* Find MMIO location of MSI page */
> +        index = imsic_cfg.nr_mmios;
> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * 
> IMSIC_MMIO_PAGE_SZ;
> +        for ( int j = 0; imsic_cfg.nr_mmios; j++ )
> +        {
> +            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 >= imsic_cfg.nr_mmios )
> +        {
> +            printk(XENLOG_WARNING "%s: MMIO not found for parent irq%d\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( !IS_ALIGNED(imsic_cfg.msi[hartid].base_addr + reloff, 
> PAGE_SIZE) )
> +        {
> +            printk(XENLOG_WARNING "%s: MMIO address 0x%lx is not aligned on 
> a page\n",
> +                   node->name, imsic_cfg.msi[hartid].base_addr + reloff);
> +            imsic_cfg.msi[hartid].offset = 0;
> +            imsic_cfg.msi[hartid].base_addr = 0;
> +            continue;
> +        }
> +
> +        imsic_cfg.mmios[index].harts[hartid] = true;
> +        imsic_cfg.msi[hartid].base_addr = imsic_cfg.mmios[index].base_addr;
> +        imsic_cfg.msi[hartid].offset = reloff;
> +        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:

Labels indented by at least one blank please.

> +    xfree(imsic_cfg.mmios);

Better use XFREE() in cases like this one?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/imsic.h
> @@ -0,0 +1,66 @@
> +/* 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
> +#define IMSIC_MAX_ID            2048
> +
> +struct imsic_msi {
> +    paddr_t base_addr;
> +    unsigned long offset;
> +};
> +
> +struct imsic_mmios {
> +    paddr_t base_addr;
> +    unsigned long size;
> +    bool harts[NR_CPUS];

An array of bool - won't a bitmap do here? Even then I wouldn't be overly
happy to see it dimensioned by NR_CPUS.

> +};
> +
> +struct imsic_config {
> +    /* base address */
> +    paddr_t base_addr;
> +
> +    /* Bits representing Guest index, HART index, and Group index */
> +    unsigned int guest_index_bits;
> +    unsigned int hart_index_bits;
> +    unsigned int group_index_bits;
> +    unsigned int group_index_shift;
> +
> +    /* imsic phandle */
> +    unsigned int phandle;
> +
> +    /* number of parent irq */
> +    unsigned int nr_parent_irqs;
> +
> +    /* number off interrupt identities */
> +    unsigned int nr_ids;
> +
> +    /* mmios */
> +    unsigned int nr_mmios;
> +    struct imsic_mmios *mmios;
> +
> +    /* MSI */
> +    struct imsic_msi msi[NR_CPUS];

You surely can avoid wasting perhaps a lot of memory by allocating this
based on the number of CPUs in use?

> +};
> +
> +struct dt_device_node;
> +int imsic_init(struct dt_device_node *n);

Misra demands that parameter names match between declaration and definition.

> +struct imsic_config;

I don't think you need this, as it's ...

> +const struct imsic_config *imsic_get_config(void);

... not used as parameter type (where its scope would otherwise be wrongly
limited).

Jan