Re: [PATCH v1 10/27] xen/riscv: generate IMSIC DT node for guest domains

[Jan Beulich <jbeulich@suse.com>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> Guests using the IMSIC interrupt controller require a corresponding
> Device Tree description. Add support for generating an IMSIC node when
> building the guest DT.
> 
> Keep a reference to the host IMSIC DT node and reuse its compatible
> property while constructing the guest-visible node.

Again raises a migration concern. Presumably a guest would then be able
to migrate only to other hosts with the same compatible property.

> @@ -487,3 +492,111 @@ int __init imsic_init(const struct dt_device_node *node)
>  
>      return rc;
>  }
> +
> +static int __init imsic_make_reg_property(struct domain *d, void *fdt)
> +{
> +    __be32 regs[4];
> +
> +    regs[0] = cpu_to_be32(imsic_cfg.base_addr >> 32);
> +    regs[1] = cpu_to_be32(imsic_cfg.base_addr);
> +    regs[2] = cpu_to_be32((IMSIC_MMIO_PAGE_SZ * d->max_vcpus) >> 32);
> +    regs[3] = cpu_to_be32(IMSIC_MMIO_PAGE_SZ * d->max_vcpus);

Might be nice to encode as an initializer of the array variable. There don't
look to be any side effects in the expressions used, so Misra should take no
issue with this.

> +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> +}
> +
> +static int __init imsic_set_interrupt_extended_prop(struct domain *d,
> +                                                    void *fdt)
> +{
> +    uint32_t len = 0, pos = 0, cpu, phandle;

At least pos and cpu should be of fixed width types.

> +    uint32_t *irq_ext;
> +    char buf[64];

Move this one into the loop's scope?

> +    int res;
> +
> +    irq_ext = xvzalloc_array(uint32_t, d->max_vcpus * 2);
> +    if ( !irq_ext )
> +        return -ENOMEM;
> +
> +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> +    {
> +        snprintf(buf, sizeof(buf), "/cpus/cpu@%u/interrupt-controller", cpu);
> +        phandle = fdt_get_phandle(fdt, fdt_path_offset(fdt, buf));
> +
> +        if ( phandle <= 0 )
> +            return phandle;

<= is odd to use on an unsigned type. Arm, in a similar situation, uses != .

Yet then - if 0 is an error indicator, aren't you converting this into a
success indicator for the caller?

> +        irq_ext[pos++] = cpu_to_be32(phandle);
> +        len += sizeof(uint32_t);

As before, preferably sizeof(<expression>) to clarify the connection. It's
pretty obvious here, but still. Yet then - do you really need to maintain
"len"? Why not use ...

> +        irq_ext[pos++] = cpu_to_be32(IRQ_S_EXT);
> +        len += sizeof(uint32_t);
> +    }
> +
> +    res = fdt_property(fdt, "interrupts-extended", irq_ext, len);

... "d->max_vcpus * 2 * sizeof(*irq_ext)" here?

> +    XVFREE(irq_ext);

This can be just xvfree(), as the variable goes out of scpoe just afterwards
anyway.

> +    return res;
> +}
> +
> +int __init imsic_make_dt_node(const struct kernel_info *kinfo)

So this function is unused until the next patch. Acceptable as long as Eclair
(or other) scans aren't done for RISC-V, but generally you may want to try to
get used to order patches such that this is avoided.

> +{
> +    uint32_t len;
> +    const void *data = NULL;

Why the initializer?

> +    int res = 0;
> +    void *fdt = kinfo->fdt;
> +    const struct dt_device_node *host_imsic_node = imsic_cfg.host_node;
> +    uint32_t *next_phandle = &kinfo->bd.d->arch.next_phandle;
> +
> +    res = fdt_begin_node(fdt, host_imsic_node->full_name);
> +    if ( res )
> +        return res;
> +
> +    data = dt_get_property(host_imsic_node, "compatible", &len);
> +    if ( !data )
> +    {
> +        printk(XENLOG_ERR "%s: Can't find 'compatible' property\n",
> +               host_imsic_node->full_name);
> +
> +        return -ENOENT;
> +    }

Move fdt_begin_node() down below here?

> --- a/xen/arch/riscv/include/asm/imsic.h
> +++ b/xen/arch/riscv/include/asm/imsic.h
> @@ -57,11 +57,16 @@ struct imsic_config {
>      /* MSI */
>      const struct imsic_msi *msi;
>  
> +    /* DT node of IMSIC */
> +    const struct dt_device_node *host_node;

Does "host" in the name carry much of a meaning? Maybe better dt_node, to
(potentially) distinguish it from a NUMA node which also may need tracking
for an imsic at some point?

Jan