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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2025 18:03, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/imsic.c
> @@ -0,0 +1,354 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/imsic.c
> + *
> + * RISC-V Incoming MSI Controller support
> + *
> + * (c) Microchip Technology Inc.
> + * (c) Vates
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/const.h>
> +#include <xen/cpumask.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/macros.h>
> +#include <xen/smp.h>
> +#include <xen/spinlock.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;
> +}

Minor remark regarding the comment: Consider replacing "expected" by "supposed"
or "intended"?

> +/*
> + * Parses IMSIC DT node.
> + *
> + * Returns 0 if initialization is successful, a negative value on failure,
> + * or IRQ_M_EXT if the IMSIC node corresponds to a machine-mode IMSIC,
> + * which should be ignored by the hypervisor.
> + */
> +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;
> +    uint32_t *irq_range;
> +
> +    *nr_parent_irqs = dt_number_of_irq(node);
> +    if ( !*nr_parent_irqs )
> +        panic("%s: irq_num can be 0. Check %s node\n", __func__,
> +              dt_node_full_name(node));

DYM "can't be"?

> +    irq_range = xzalloc_array(uint32_t, *nr_parent_irqs * 2);
> +    if ( !irq_range )
> +        panic("%s: irq_range[] allocation failed\n", __func__);
> +
> +    if ( (rc = dt_property_read_u32_array(node, "interrupts-extended",
> +                                    irq_range, *nr_parent_irqs * 2)) )
> +        panic("%s: unable to find interrupt-extended in %s node: %d\n",
> +              __func__, dt_node_full_name(node), rc);
> +
> +    if ( irq_range[1] == IRQ_M_EXT )
> +    {
> +        /* Machine mode imsic node, ignore it. */
> +        rc = IRQ_M_EXT;
> +        goto cleanup;
> +    }

Wouldn't this better be done ...

> +    /* Check that interrupts-extended property is well-formed. */
> +    for ( unsigned int i = 2; i < (*nr_parent_irqs * 2); i += 2 )
> +    {
> +        if ( irq_range[i + 1] != irq_range[1] )
> +            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
> +    }

... after this consistency check?

Also %u please when you log unsigned values.

> +    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",
> +               dt_node_name(node));
> +        rc = -ENOENT;
> +        goto cleanup;
> +    }
> +
> +    /* 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++;

Since fls() returns a 1-based bit number, isn't it rather that in the
exact-power-of-2 case you'd need to subtract 1?

> +    }
> +    tmp -= imsic_cfg.guest_index_bits;
> +    if ( tmp < imsic_cfg.hart_index_bits )
> +    {
> +        printk(XENLOG_ERR "%s: HART index bits too big\n",
> +               dt_node_name(node));
> +        rc = -ENOENT;
> +        goto cleanup;
> +    }
> +
> +    /* 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",
> +               dt_node_name(node));
> +        rc = -ENOENT;
> +        goto cleanup;
> +    }
> +
> +    /* 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",
> +               dt_node_name(node));
> +        rc = -ENOENT;
> +        goto cleanup;
> +    }
> +    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",
> +               dt_node_name(node));
> +        rc = -EINVAL;
> +        goto cleanup;
> +    }
> +
> +    /* 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);
> +        rc = -ENOENT;
> +        goto cleanup;
> +    }
> +
> +    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) )

Now that you've explained to me what the deal is with these constants: Isn't
the 1st of the three checks redundant with the last one?

> +    {
> +        printk(XENLOG_ERR "%s: invalid number of interrupt identities\n",
> +               node->name);
> +        rc = -EINVAL;
> +        goto cleanup;
> +    }
> +
> +    /* 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: %d\n",
> +               dt_node_name(node), rc);
> +        goto cleanup;
> +    }
> +
> +    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);
> +
> +    /* Find number of MMIO register sets */
> +    do {
> +        imsic_cfg.nr_mmios++;
> +    } while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, 
> NULL) );
> +
> + cleanup:
> +    xfree(irq_range);

Afacit you could free this array way earlier. That would then simplify quite
a few of the error paths, I think.

> +/*
> + * Initialize the imsic_cfg structure based on the IMSIC DT node.
> + *
> + * Returns 0 if initialization is successful, a negative value on failure,
> + * or IRQ_M_EXT if the IMSIC node corresponds to a machine-mode IMSIC,
> + * which should be ignored by the hypervisor.
> + */
> +int __init imsic_init(const struct dt_device_node *node)
> +{
> +    int rc;
> +    unsigned long reloff, hartid;
> +    unsigned int nr_parent_irqs, index, nr_handlers = 0;
> +    paddr_t base_addr;
> +    unsigned int nr_mmios;
> +
> +    /* Parse IMSIC node */
> +    rc = imsic_parse_node(node, &nr_parent_irqs);
> +    /*
> +     * If machine mode imsic node => ignore it.
> +     * If rc < 0 => parsing of IMSIC DT node failed.
> +     */
> +    if ( (rc == IRQ_M_EXT) || rc )
> +        return rc;

The former of the checks is redundant with the latter. Did you perhaps mean
"rc < 0" for that one?

> +    nr_mmios = imsic_cfg.nr_mmios;
> +
> +    /* Allocate MMIO resource array */
> +    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios);

How large can this and ...

> +    if ( !imsic_cfg.mmios )
> +    {
> +        rc = -ENOMEM;
> +        goto imsic_init_err;
> +    }
> +
> +    imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs);

... this array grow (in principle)? I think you're aware that in principle
new code is expected to use xvmalloc() and friends unless there are specific
reasons speaking against that.

> +    if ( !imsic_cfg.msi )
> +    {
> +        rc = -ENOMEM;
> +        goto imsic_init_err;
> +    }
> +
> +    /* Check MMIO register sets */
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +    {
> +        if ( !alloc_cpumask_var(&imsic_cfg.mmios[i].cpus) )
> +        {
> +            rc = -ENOMEM;
> +            goto imsic_init_err;
> +        }
> +
> +        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 %u\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);
> +        if ( base_addr != imsic_cfg.base_addr )
> +        {
> +            rc = -EINVAL;
> +            printk(XENLOG_ERR "%s: address mismatch for regset %u\n",
> +                   node->name, i);
> +            goto imsic_init_err;
> +        }

Maybe just for my own understanding: There's no similar check for the
sizes to match / be consistent wanted / needed?

> +    }
> +
> +    /* Configure handlers for target CPUs */
> +    for ( unsigned int i = 0; i < nr_parent_irqs; i++ )
> +    {
> +        unsigned long xen_cpuid;
> +
> +        rc = imsic_get_parent_hartid(node, i, &hartid);
> +        if ( rc )
> +        {
> +            printk(XENLOG_WARNING "%s: cpu ID for parent irq%u not found\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        xen_cpuid = hartid_to_cpuid(hartid);

I'm probably biased by "cpuid" having different meaning on x86, but: To
be consistent with variable names elsewhere, couldn't this variable simply
be named "cpu"? With the other item named "hartid" there's no ambiguity
there anymore.

> +        if ( xen_cpuid >= num_possible_cpus() )
> +        {
> +            printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent 
> irq%u\n",
> +                   node->name, hartid, i);

The message continues to be ambiguous (to me as a non-RISC-V person at
least): You log a hart ID, while you say "cpu ID". Also, as I think I
said elsewhere already, the hart ID may better be logged using %#lx.

> +            continue;
> +        }
> +
> +        /* Find MMIO location of MSI page */
> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * 
> IMSIC_MMIO_PAGE_SZ;
> +        for ( index = 0; index < nr_mmios; index++ )
> +        {
> +            if ( reloff < imsic_cfg.mmios[index].size )
> +                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[index].size,
> +                BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ);

Nit: Indentation once again.

> +        }
> +
> +        if ( index == nr_mmios )
> +        {
> +            printk(XENLOG_WARNING "%s: MMIO not found for parent irq%u\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( !IS_ALIGNED(imsic_cfg.msi[xen_cpuid].base_addr + reloff, 
> PAGE_SIZE) )

If this is the crucial thing to check, ...

> +        {
> +            printk(XENLOG_WARNING "%s: MMIO address %#lx is not aligned on a 
> page\n",
> +                   node->name, imsic_cfg.msi[xen_cpuid].base_addr + reloff);
> +            imsic_cfg.msi[xen_cpuid].offset = 0;
> +            imsic_cfg.msi[xen_cpuid].base_addr = 0;
> +            continue;
> +        }
> +
> +        cpumask_set_cpu(xen_cpuid, imsic_cfg.mmios[index].cpus);
> +
> +        imsic_cfg.msi[xen_cpuid].base_addr = 
> imsic_cfg.mmios[index].base_addr;
> +        imsic_cfg.msi[xen_cpuid].offset = reloff;

... why is it that the two parts are stored separately? If their sum needs to
be page-aligned, I'd kind of expect it's only ever the sum which is used?

Also is it really PAGE_SHIFT or rather IMSIC_MMIO_PAGE_SHIFT that needs
chacking against?

Further please pay attention to line length limits - there are at least two
violations around my earlier comment here.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/imsic.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/include/asm/imsic.h
> + *
> + * RISC-V Incoming MSI Controller support
> + *
> + * (c) Microchip Technology Inc.
> + */
> +
> +#ifndef ASM__RISCV__IMSIC_H
> +#define ASM__RISCV__IMSIC_H

Please update according to the most recent naming rules change (all it takes
may be to shrink the double underscores).

> +#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            2047
> +
> +struct imsic_msi {
> +    paddr_t base_addr;
> +    unsigned long offset;
> +};
> +
> +struct imsic_mmios {
> +    paddr_t base_addr;
> +    unsigned long size;
> +    cpumask_var_t 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;

Are the contents of this and ...

> +    /* MSI */
> +    struct imsic_msi *msi;

... this array ever changing post-init? If not, the pointers here may want
to be pointer-to-const (requiring local variables in the function populating
the field).

> @@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long 
> cpuid)
>      return pcpu_info[cpuid].hart_id;
>  }
>  
> +static inline unsigned long hartid_to_cpuid(unsigned long hartid)
> +{
> +    for ( unsigned int cpuid = 0; cpuid < ARRAY_SIZE(pcpu_info); cpuid++ )
> +    {
> +        if ( hartid == cpuid_to_hartid(cpuid) )
> +            return cpuid;
> +    }
> +
> +    /* hartid isn't valid for some reason */
> +    return NR_CPUS;
> +}

Considering the values being returned, why's the function's return type
"unsigned long"?

Why the use of ARRAY_SIZE() in the loop header? You don't use pcpu_info[]
in the loop body.

Finally - on big systems this is going to be pretty inefficient a lookup.
This may want to at least have a TODO comment attached.

Jan