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

Oleksii Kurochko posted 27 patches 4 weeks ago
[PATCH v1 10/27] xen/riscv: generate IMSIC DT node for guest domains
Posted by Oleksii Kurochko 4 weeks ago
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. The generated node
contains guest-specific information such as the MMIO region sized for
the number of vCPUs and the interrupts-extended property referencing
per-vCPU interrupt controllers.

This allows guests to discover and use the IMSIC interrupt controller.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/imsic.c             | 113 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/imsic.h |   7 ++
 2 files changed, 120 insertions(+)

diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index a4460576f620..0956b187705f 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -13,8 +13,11 @@
 #include <xen/const.h>
 #include <xen/cpumask.h>
 #include <xen/device_tree.h>
+#include <xen/domain.h>
 #include <xen/errno.h>
+#include <xen/fdt-kernel.h>
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
 #include <xen/macros.h>
 #include <xen/smp.h>
 #include <xen/spinlock.h>
@@ -329,6 +332,8 @@ int __init imsic_init(const struct dt_device_node *node)
     struct imsic_mmios *mmios;
     struct imsic_msi *msi = NULL;
 
+    imsic_cfg.host_node = node;
+
     /* Parse IMSIC node */
     rc = imsic_parse_node(node, &nr_parent_irqs, &nr_mmios);
     /*
@@ -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);
+
+    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;
+    uint32_t *irq_ext;
+    char buf[64];
+    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;
+
+        irq_ext[pos++] = cpu_to_be32(phandle);
+        len += sizeof(uint32_t);
+        irq_ext[pos++] = cpu_to_be32(IRQ_S_EXT);
+        len += sizeof(uint32_t);
+    }
+
+    res = fdt_property(fdt, "interrupts-extended", irq_ext, len);
+
+    XVFREE(irq_ext);
+
+    return res;
+}
+
+int __init imsic_make_dt_node(const struct kernel_info *kinfo)
+{
+    uint32_t len;
+    const void *data = NULL;
+    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;
+    }
+
+    res = fdt_property(fdt, "compatible", data, len);
+    if ( res )
+        return res;
+
+    res = imsic_make_reg_property(kinfo->bd.d, fdt);
+    if ( res )
+        return res;
+
+    res = imsic_set_interrupt_extended_prop(kinfo->bd.d, fdt);
+    if ( res )
+        return res;
+
+    res = fdt_property_u32(fdt, "riscv,num-ids", imsic_cfg.nr_ids);
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "msi-controller", NULL, 0);
+    if ( res )
+        return res;
+
+    res = fdt_property_u32(fdt, "#msi-cells", 0);
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+    if ( res )
+        return res;
+
+    res = fdt_property_u32(fdt, "#interrupt-cells", 0);
+    if ( res )
+        return res;
+
+    imsic_cfg.phandle = (*next_phandle)++;
+
+    res = fdt_property_cell(fdt, "phandle", imsic_cfg.phandle);
+    if ( res )
+        return res;
+
+    return fdt_end_node(fdt);
+}
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index c6c59215df20..a63d56fbd5d9 100644
--- 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;
+
     /* Lock to protect access to IMSIC's stuff */
     spinlock_t lock;
 };
 
 struct dt_device_node;
+struct kernel_info;
+
 int imsic_init(const struct dt_device_node *node);
 
 const struct imsic_config *imsic_get_config(void);
@@ -71,4 +76,6 @@ void imsic_irq_disable(unsigned int hwirq);
 
 void imsic_ids_local_delivery(bool enable);
 
+int imsic_make_dt_node(const struct kernel_info *kinfo);
+
 #endif /* ASM_RISCV_IMSIC_H */
-- 
2.53.0
Re: [PATCH v1 10/27] xen/riscv: generate IMSIC DT node for guest domains
Posted by Jan Beulich 6 days, 11 hours ago
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