Re: [PATCH v4 3/9] xen/riscv: imsic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2025 17:58, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/imsic.c
> @@ -0,0 +1,358 @@
> +/* 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/xvmalloc.h>
> +
> +#include <asm/imsic.h>
> +
> +#define IMSIC_HART_SIZE(guest_bits_) (BIT(guest_bits_, U) * 
> IMSIC_MMIO_PAGE_SZ)

Minor: Any particular reason for the trailing underscore here?

> +/*
> + * 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;
> +    struct imsic_mmios *mmios;
> +    struct imsic_msi *msi = NULL;
> +
> +    /* Parse IMSIC node */
> +    rc = imsic_parse_node(node, &nr_parent_irqs, &nr_mmios);
> +    /*
> +     * If machine mode imsic node => ignore it.
> +     * If rc < 0 => parsing of IMSIC DT node failed.
> +     */
> +    if ( (rc == IRQ_M_EXT) || (rc < 0) )
> +        return rc;
> +
> +    /* Allocate MMIO resource array */
> +    mmios = xvzalloc_array(struct imsic_mmios, nr_mmios);
> +    if ( !mmios )
> +    {
> +        rc = -ENOMEM;
> +        goto imsic_init_err;
> +    }
> +
> +    msi = xvzalloc_array(struct imsic_msi, nr_parent_irqs);
> +    if ( !msi )
> +    {
> +        rc = -ENOMEM;
> +        goto imsic_init_err;
> +    }
> +
> +    /* Check MMIO register sets */
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +    {
> +        unsigned int guest_bits = imsic_cfg.guest_index_bits;
> +        unsigned long expected_mmio_size =
> +            IMSIC_HART_SIZE(guest_bits) * nr_parent_irqs;
> +
> +        rc = dt_device_get_address(node, i, &mmios[i].base_addr,
> +                                   &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 = mmios[i].base_addr;
> +        base_addr &= ~(BIT(guest_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;
> +        }
> +
> +        if ( mmios[i].size != expected_mmio_size )
> +        {
> +            rc = -EINVAL;
> +            printk(XENLOG_ERR "%s: IMSIC MMIO size is incorrect %ld, "
> +                   "expeced MMIO size: %ld\n", node->name, mmios[i].size,

To aid grep-ability, please avoid wrapping format strings across lines.
(Same at least once more elsewhere.)

> +                   expected_mmio_size);
> +            goto imsic_init_err;
> +        }
> +    }
> +
> +    /* Configure handlers for target CPUs */
> +    for ( unsigned int i = 0; i < nr_parent_irqs; i++ )
> +    {
> +        unsigned long cpu;

Along the lines of questions on earlier versions: Any reason this isn't
unsigned int?

> +        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;
> +        }
> +
> +        cpu = hartid_to_cpuid(hartid);
> +
> +        if ( cpu >= num_possible_cpus() )
> +        {
> +            printk(XENLOG_WARNING "%s: unsupported hart ID=%#lx for parent "
> +                   "irq%u\n", node->name, hartid, i);
> +            continue;
> +        }
> +
> +        /* Find MMIO location of MSI page */
> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * 
> IMSIC_MMIO_PAGE_SZ;

Any reason to open-code IMSIC_HART_SIZE() here and ...

> +        for ( index = 0; index < nr_mmios; index++ )
> +        {
> +            if ( reloff < 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(mmios[index].size,
> +                      BIT(imsic_cfg.guest_index_bits, UL) * 
> IMSIC_MMIO_PAGE_SZ);

... here?

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

DYM mmios[index].base_addr here, considering that ...

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

msi[cpu] is set only here?

Also is the setting to zero of both fields on the "continue" path really
needed, seeing that the array starts out zero-filled? Can the same CPU
be found twice, making it necessary(?) to invalidate the array slot later?

> +        nr_handlers++;
> +    }
> +
> +    if ( !nr_handlers )
> +    {
> +        printk(XENLOG_ERR "%s: No CPU handlers found\n", node->name);
> +        rc = -ENODEV;
> +        goto imsic_init_err;
> +    }
> +
> +    imsic_cfg.msi = msi;
> +
> +    XFREE(mmios);

No need to use the macro, and you really mean xvfree() now.

> +    return 0;
> +
> + imsic_init_err:
> +    XFREE(mmios);
> +    XFREE(msi);

Same here.

Jan