imsic_init() is introduced to parse device tree node, which has the following
bindings [2], and based on the parsed information update IMSIC configuration
which is stored in imsic_cfg.
The following helpers are introduces for imsic_init() usage:
- imsic_parse_node() parses IMSIC node from DTS
- imsic_get_parent_hartid() returns the hart ( CPU ) ID of the given device
tree node.
This patch is based on the code from [1].
Since Microchip originally developed imsic.{c,h}, an internal discussion with
them led to the decision to use the MIT license.
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/0b1a94f2bc3bb1a81cd26bb75f0bf578f84cb4d4
[2] https://elixir.bootlin.com/linux/v6.12/source/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/Makefile | 1 +
xen/arch/riscv/imsic.c | 286 +++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/imsic.h | 66 +++++++
3 files changed, 353 insertions(+)
create mode 100644 xen/arch/riscv/imsic.c
create mode 100644 xen/arch/riscv/include/asm/imsic.h
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 457e8e88a4..baa499a72d 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -2,6 +2,7 @@ obj-y += aplic.o
obj-y += cpufeature.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += entry.o
+obj-y += imsic.o
obj-y += intc.o
obj-y += irq.o
obj-y += mm.o
diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
new file mode 100644
index 0000000000..99def9af2d
--- /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
+ */
+
+#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)
+{
+ 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 */
+ 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;
+}
+
+
+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 )
+ 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;
+ 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) ||
+ ((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)
+ {
+ printk(XENLOG_ERR "%s: first MMIO resource not found\n", node->name);
+ return -EINVAL;
+ }
+
+ 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 */
+ imsic_cfg.nr_mmios++;
+ while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL) )
+ imsic_cfg.nr_mmios++;
+
+ 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++ )
+ {
+ 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);
+ 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;
+ }
+ }
+
+ /* 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:
+ xfree(imsic_cfg.mmios);
+
+ return rc;
+}
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
new file mode 100644
index 0000000000..126e651863
--- /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];
+};
+
+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];
+};
+
+struct dt_device_node;
+int imsic_init(struct dt_device_node *n);
+
+struct imsic_config;
+const struct imsic_config *imsic_get_config(void);
+
+#endif /* ASM__RISCV__IMSIC_H */
--
2.49.0On 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
On 4/14/25 11:32 AM, Jan Beulich wrote:
> 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)?
I think it doesn't really matter and it could be just dropped.
>
>> + */
>> +
>> +#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?
No, I think it could be const struct imsic_config * const as a return type.
It isn't expected that the caller will change a pointer.
>
>> +{
>> + 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.
A stale comment, we decided to support only interrupt-extended to be in line with
Linux device tree bindings.
I'll drop this comment.
>
> 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?
No particular reason, just overlooked that we could really return just `res`.
>> +static int imsic_parse_node(struct dt_device_node *node,
>> + unsigned int *nr_parent_irqs)
(for me: fix an indentation)
>> +{
>> + 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?
'int` is used because of ...:
rc = dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL);
... at the end of imsic_parse_node().
Agree, we can just drop `rc` variable and return err code explicitly.
>
>> + 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.)
Yes, it would be simpler.
>> + 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.
But it is the maximum of identities for IMISIC according to the AIA specs:
```
( MRIF stands for Memory-Resident Interrupt File )
All MRIFs are the size to accommodate 2047 valid interrupt identities,
the maximum allowed for an IMSIC interrupt file. If a system’s actual IMSICs
have interrupt files that implement only N interrupt identities, N < 2047,
then the contents of MRIFs for identities greater than N may be ignored by
softwareand
```
and prefix IMSIC (IMO)emphasizes this. Would it be better to rename to IMSIC_MAX_ALLOWED_ID?
And then IMSIC_MIN_ALLOWED_ID?
>
>> + ((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?
Agree, it could be just `rc`, but considering of that this functions always returns
-EINVAL in case of the error and that `rc` could be dropped at all, we just explicitly
return here -EINVAL.
>
>> + }
>> +
>> + 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?
The group index is used to differentiate between clusters/groups. For
example, consider two clusters: - Cluster 1 with cpu0 and cpu1 - Cluster
2 with cpu2 and cpu3 Then, the reg property in the IMSIC node will
include two entries: reg = <0x0 0xd100000 0x0 0x20000>, <0x0 0x2d100000
0x0 0x20000>; riscv,guest-index-bits = <3>; riscv,hart-index-bits = <2>;
riscv,group-index-bits = <1>; riscv,group-index-shift = <29>; In this
example: The group index is 1 bit wide (group-index-bits = <1>), It is
located at bit 29 (group-index-shift = <29>) of the address.
so imsic_cfg.group_index_bits will be used to distinguish clusters, but
they must have
the same base address and this is the reason why the mask is applied.
>
>> + /* 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.
It is fetched again because we are using imsic_cfg.nr_mmios to calculate the size of
imsic_cfg.mmios array:
imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, imsic_cfg.nr_mmios);
>
> Also - use do-while here?
It could be, I'll update that.
>> + 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.
>
>> + xfree(imsic_cfg.mmios);
> Better use XFREE() in cases like this one?
I think, yes.
>
>> --- /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.
Bitmap will fit here well. But for DECLARE_BITMAP() is necessary the size
of bitmap so NR_CPUS should be used again.
Could you please remind me why it isn't good to use it?
Because NR_CPUS not always equal to an amount of physical cpus?
Should I use non-static version of bitmap declaration? (if we have such...)
>
>> +};
>> +
>> +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?
It make sense. I'll allocate then this dynamically.
Thanks!
~ Oleksii
© 2016 - 2025 Red Hat, Inc.