On 6/10/25 5:17 PM, Jan Beulich wrote:
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?
Not really, I will drop it.
+/*
+ * 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?
Failed to do proper cleanup. It should be 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?
Thanks, missed that. I'll update such places.
+ }
+
+ 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?
Yes, mmios[] should be used instead of msi[]...
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?
Normally no, it can't be that the same CPU is found twice.
On other side, I tried to compile by hand DTS which has two the same CPUs
mentioned in IMSIC node and it allows me to do that:
(XEN) Latest ChangeSet:
(XEN) build-id: 2cd98ed5a91bf443a02a9c4e83f05df4c1f7ec61
(XEN) !!! hartid:0
(XEN) imsics: unsupported hart ID=0 for parent irq0
(XEN) !!! hartid:2
(XEN) imsics: unsupported hart ID=0x2 for parent irq1
(XEN) !!! hartid:2
(XEN) imsics: unsupported hart ID=0x2 for parent irq2
(XEN) !!! hartid:3
(XEN) Allocated console ring of 16 KiB.
(unsupported hart ID=... is printed here because I told QEMU that you have 1
cpu, but in DTS I mentioned more then 1)
It seems like it is needed also to check if IMSIC node provides unique cpu in
its interrupts-extended property. And to verify that I think it would be enough
to check if msi[cpu].base_addr isn't 0:
@@ -408,15 +407,19 @@ int __init imsic_init(const struct dt_device_node *node)
cpu = hartid_to_cpuid(hartid);
+ if ( msi[cpu].base_addr )
+ panic("%s: cpu%d is found twice in interrupts-extended prop\n",
+ node->name, cpu);
+
Or probably it would be better just ignore by doing 'continue' instead of
panic() as it is done in other places in imsic_init().
Thanks.
~ Oleksii
