* [PATCH v6 1/7] xen/riscv: imsic_init() implementation
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-08 13:52 ` Jan Beulich
2025-07-07 9:01 ` [PATCH v6 2/7] xen/riscv: aplic_init() implementation Oleksii Kurochko
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
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>
---
Changes in V6:
- Code style fixes.
- s/xfree/xvfree.
- (*nr_mmios)++ -> ++*nr_mmios.
- Use %u for unsigned int arguments.
- Change name of local variable cpuid to cpu.
---
Changes in V5:
- Drop trailing underscore for an argument of IMSIC_HART_SIZE macros.
- Avoid wrapping format strings across lines.
- Use 'unsigned int' for cpu variable inside imsic_init().
- Use IMSIC_HART_SIZE() instead of open-code it.
- s/msi[cpu].base_addr /mmios[].base_addr for the check which checks
that MMIO addres is properly aligned.
- s/XFREE/xvfree.
- Drop zero-ing of msi[cpu].{offset,base_addr} as msi[] is zero-ed
when is allocated and cpu id can't be found twice.
- Add check to vefiry that CPU won't be found twice in interrupt-extended
property of IMSIC node.
---
Changes in V4:
- s/expected/intended in the comment above imsic_get_config().
- [imsic_parse_node()] s/irq_num can be 0/irq_num can't be 0 in panic()
message.
- [imsic_parse_node()] Move "if ( irq_range[1] == IRQ_M_EXT )" after 'for loop'
which checks interrupts-extended property.
- [imsic_parse_node()] s/%d/%u for logging unsigned values.
- [imsic_parse_node()] drop redundant check "(imsic_cfg.nr_ids & IMSIC_MIN_ID) != IMSIC_MIN_ID)".
- [imsic_parse_node()] free irq_range much earlier and simlify error paths.
- [imsic_parse_node()] s/-EINVAL/-ENOENT in the case of incorrect value for
riscv,group-index-shift and id number.
- [[imsic_parse_node()] s/xzalloc_array/xvzalloc_array.
- s/xen_cpuid/cpu.
- Identation fix.
- use IMSIC_MMIO_PAGE_SZ instead of PAGE_SZ to check if interrupt file base
addr is properly aligned.
- s/ASM__RISCV__IMSIC_H/ASM_RISCV_IMSIC_H.
- Drop *mmios from struct imsic_cfg as it is used only by imsic_init().
- Drop cpus[] form struct imsic_mmios as it isn't really used.
- Update declaration of hartid_to_cpuid() to return unsigned int instead of
NR_CPUs as processor_id is in range [0, NR_CPUS) and NR_CPUs is less then
unsigned int.
- Calculate hart_index_bits as fls(*nr_parent_irqs - 1) to cover the case if
nr_parent_irqs is a power of two.
- Check an MMIO's size for IMSIC node.
---
Changes in V3:
- Drop year in imsic.h in copyrights.
- Correct identation in imsic_parse_node() and imsic_init()
where for imsic_cfg.base_addr a mask is applied.
- Use unsigned int istead of uint32_t for local variable nr_parent_irqs,
index, nr_handlers in imsic_init().
- Fix a leakage of ealiers successfull allocations in case if imsic_init()
returns with an error.
- Excess blank in printk() message: "%s: unable to parse MMIO regset %d\n".
- Introduce hartid_to_cpuid() and use it in the check:
if ( hardid_to_cpuid(cpuid) >= num_possible_cpus() )
in imsic_init().
- Use "%u" for unsigned int in printk(...).
- Fix for loop condition: nr_mmios -> "j < nr_mmios".
- [imsic_init()] Drop usage of j in nested loop. It is enough to have only
index.
- Change 0x%lx to %#lx for formatting of an address in printk().
- [imsic_init()] Rename local variable cpuid to hartid.
- s/imsic_get_parent_cpuid/imsic_get_parent_hartid, s/cpuid/hartid for an
imsic_get_parent_hartid()'s argument.
- Declare cpus member of struct imsic_mmios as cpumask_t.
- [imsic_init()] Allocate imsic_mmios.cpus by using of alloc_cpumask_var().
- [imsic_init()] Use cpumask_set_cpu() instead of bitmap_set().
- [imsic_parse_node()] add check for correctness of "interrupt-extended" property.
- [imsic_parse_node()] Use dt_node_name() or dt_full_node_name() instead of
derefence of struct dt_node.
- [imsic_parse_node()] Add cleanup label and update 'rc = XXX; goto cleanup'
instead of 'return rc' as now we have to cleanup dynamically allocated irq_range
array.
- Add comments above imsic_init() and imsic_parse_node().
- s/xen/arch/riscv/imsic.h/xen/arch/riscv/include/asm/imsic.h in the comment of
imsic.h.
---
Changes in V2:
- Drop years in copyrights.
- s/riscv_of_processor_hartid/dt_processor_cpuid.
- s/imsic_get_parent_hartid/imsic_get_parent_hartid.
Rename argument hartid to cpuid.
Make node argument const.
Return res instead of -EINVAL for the failure case of dt_processor_cpuid().
Drop local variable hart and use cpuid argument instead.
Drop useless return res;
- imsic_parse_node() changes:
- Make node argument const.
- Check the return value of dt_property_read_u32() directly instead of
saving it to rc variable.
- Update tmp usage, use short form "-=".
- Update a check (imsic_cfg.nr_ids >= IMSIC_MAX_ID) to (imsic_cfg.nr_ids > IMSIC_MAX_ID)
as IMSIC_MAX_ID is changed to maximum valid value, not just the firsr out-of-range.
- Use `rc` to return value instead of explicitly use -EINVAL.
- Use do {} while() to find number of MMIO register sets.
- Set IMSIC_MAX_ID to 2047 (maximum possible IRQ number).
- imsic_init() changes:
- Use unsigned int in for's expression1.
- s/xfree/XFEE.
- Allocate msi and cpus array dynamically.
- Drop forward declaration before declaration of imsic_get_config() in asm/imsic.h
as it is not used as parameter type.
- Align declaration of imisic_init with defintion.
- s/harts/cpus in imisic_mmios.
Also, change type from bool harts[NR_CPUS] to unsigned long *cpus.
- Allocate msi member of imsic_config dynamically to save some memory.
- Code style fixes.
- Update the commit message.
---
xen/arch/riscv/Makefile | 1 +
xen/arch/riscv/imsic.c | 369 +++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/imsic.h | 55 +++++
xen/arch/riscv/include/asm/smp.h | 13 +
4 files changed, 438 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 a1c145c506..e2b8aa42c8 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..63f4233035
--- /dev/null
+++ b/xen/arch/riscv/imsic.c
@@ -0,0 +1,369 @@
+/* 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)
+
+struct imsic_mmios {
+ paddr_t base_addr;
+ unsigned long size;
+};
+
+static struct imsic_config imsic_cfg;
+
+/* Callers aren't intended to changed imsic_cfg so return const. */
+const struct imsic_config *imsic_get_config(void)
+{
+ return &imsic_cfg;
+}
+
+static int __init imsic_get_parent_hartid(const struct dt_device_node *node,
+ unsigned int index,
+ unsigned long *hartid)
+{
+ int res;
+ struct dt_phandle_args args;
+
+ res = dt_parse_phandle_with_args(node, "interrupts-extended",
+ "#interrupt-cells", index, &args);
+ if ( !res )
+ res = dt_processor_hartid(args.np->parent, hartid);
+
+ return res;
+}
+
+/*
+ * Parses 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.
+ */
+static int imsic_parse_node(const struct dt_device_node *node,
+ unsigned int *nr_parent_irqs,
+ unsigned int *nr_mmios)
+{
+ int rc;
+ unsigned int tmp;
+ paddr_t base_addr;
+ uint32_t *irq_range;
+
+ *nr_parent_irqs = dt_number_of_irq(node);
+ if ( !*nr_parent_irqs )
+ panic("%s: irq_num can't be 0. Check %s node\n", __func__,
+ dt_node_full_name(node));
+
+ irq_range = xvzalloc_array(uint32_t, *nr_parent_irqs * 2);
+ if ( !irq_range )
+ panic("%s: irq_range[] allocation failed\n", __func__);
+
+ if ( (rc = dt_property_read_u32_array(node, "interrupts-extended",
+ irq_range, *nr_parent_irqs * 2)) )
+ panic("%s: unable to find interrupt-extended in %s node: %d\n",
+ __func__, dt_node_full_name(node), rc);
+
+ /* Check that interrupts-extended property is well-formed. */
+ for ( unsigned int i = 2; i < (*nr_parent_irqs * 2); i += 2 )
+ {
+ if ( irq_range[i + 1] != irq_range[1] )
+ panic("%s: mode[%u] != %u\n", __func__, i + 1, irq_range[1]);
+ }
+
+ if ( irq_range[1] == IRQ_M_EXT )
+ {
+ /* Machine mode imsic node, ignore it. */
+ xvfree(irq_range);
+
+ return IRQ_M_EXT;
+ }
+
+ xvfree(irq_range);
+
+ if ( !dt_property_read_u32(node, "riscv,guest-index-bits",
+ &imsic_cfg.guest_index_bits) )
+ 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",
+ dt_node_name(node));
+ return -ENOENT;
+ }
+
+ /* Find number of HART index bits */
+ if ( !dt_property_read_u32(node, "riscv,hart-index-bits",
+ &imsic_cfg.hart_index_bits) )
+ /* Assume default value */
+ imsic_cfg.hart_index_bits = fls(*nr_parent_irqs - 1);
+ tmp -= imsic_cfg.guest_index_bits;
+ if ( tmp < imsic_cfg.hart_index_bits )
+ {
+ printk(XENLOG_ERR "%s: HART index bits too big\n",
+ dt_node_name(node));
+ return -ENOENT;
+ }
+
+ /* Find number of group index bits */
+ if ( !dt_property_read_u32(node, "riscv,group-index-bits",
+ &imsic_cfg.group_index_bits) )
+ imsic_cfg.group_index_bits = 0;
+ tmp -= imsic_cfg.hart_index_bits;
+ if ( tmp < imsic_cfg.group_index_bits )
+ {
+ printk(XENLOG_ERR "%s: group index bits too big\n",
+ dt_node_name(node));
+ return -ENOENT;
+ }
+
+ /* Find first bit position of group index */
+ tmp = IMSIC_MMIO_PAGE_SHIFT * 2;
+ if ( !dt_property_read_u32(node, "riscv,group-index-shift",
+ &imsic_cfg.group_index_shift) )
+ imsic_cfg.group_index_shift = tmp;
+ if ( imsic_cfg.group_index_shift < tmp )
+ {
+ printk(XENLOG_ERR "%s: group index shift too small\n",
+ dt_node_name(node));
+ 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",
+ dt_node_name(node));
+ return -ENOENT;
+ }
+
+ /* Find number of interrupt identities */
+ if ( !dt_property_read_u32(node, "riscv,num-ids", &imsic_cfg.nr_ids) )
+ {
+ 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) )
+ {
+ printk(XENLOG_ERR "%s: invalid number of interrupt identities\n",
+ node->name);
+ return -ENOENT;
+ }
+
+ /* Compute base address */
+ *nr_mmios = 0;
+ rc = dt_device_get_address(node, *nr_mmios, &base_addr, NULL);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "%s: first MMIO resource not found: %d\n",
+ dt_node_name(node), rc);
+ return rc;
+ }
+
+ 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 */
+ do {
+ ++*nr_mmios;
+ } while ( !dt_device_get_address(node, *nr_mmios, &base_addr, NULL) );
+
+ return 0;
+}
+
+/*
+ * 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, expected MMIO size: %ld\n",
+ node->name, mmios[i].size, expected_mmio_size);
+ goto imsic_init_err;
+ }
+ }
+
+ /* Configure handlers for target CPUs */
+ for ( unsigned int i = 0; i < nr_parent_irqs; i++ )
+ {
+ unsigned int cpu;
+
+ 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 .base_addr is not 0, it indicates that the CPU has already been
+ * found.
+ * In this case, skip re-initialization to avoid duplicate setup.
+ * Also, print a warning message to signal that the DTS should be
+ * reviewed for possible duplication.
+ */
+ if ( msi[cpu].base_addr )
+ {
+ printk("%s: cpu%u is found twice in interrupts-extended prop\n",
+ node->name, cpu);
+ continue;
+ }
+
+ 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 * IMSIC_HART_SIZE(imsic_cfg.guest_index_bits);
+ for ( index = 0; index < nr_mmios; index++ )
+ {
+ if ( reloff < mmios[index].size )
+ break;
+
+ /*
+ * MMIO region size may not be aligned to
+ * IMSIC_HART_SIZE(guest_index_bits) if
+ * holes are present.
+ */
+ reloff -= ROUNDUP(mmios[index].size,
+ IMSIC_HART_SIZE(imsic_cfg.guest_index_bits));
+ }
+
+ if ( index == nr_mmios )
+ {
+ printk(XENLOG_WARNING "%s: MMIO not found for parent irq%u\n",
+ node->name, i);
+ continue;
+ }
+
+ if ( !IS_ALIGNED(mmios[cpu].base_addr + reloff,
+ 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);
+ continue;
+ }
+
+ msi[cpu].base_addr = mmios[index].base_addr;
+ msi[cpu].offset = reloff;
+
+ 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;
+
+ xvfree(mmios);
+
+ return 0;
+
+ imsic_init_err:
+ xvfree(mmios);
+ xvfree(msi);
+
+ 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..9cd12365b1
--- /dev/null
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/include/asm/imsic.h
+ *
+ * RISC-V Incoming MSI Controller support
+ *
+ * (c) 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 2047
+
+struct imsic_msi {
+ paddr_t base_addr;
+ unsigned long offset;
+};
+
+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;
+
+ /* MSI */
+ const struct imsic_msi *msi;
+};
+
+struct dt_device_node;
+int imsic_init(const struct dt_device_node *node);
+
+const struct imsic_config *imsic_get_config(void);
+
+#endif /* ASM_RISCV_IMSIC_H */
diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
index eb58b6576b..5dbacf1f9d 100644
--- a/xen/arch/riscv/include/asm/smp.h
+++ b/xen/arch/riscv/include/asm/smp.h
@@ -3,6 +3,7 @@
#define ASM__RISCV__SMP_H
#include <xen/cpumask.h>
+#include <xen/macros.h>
#include <xen/percpu.h>
#include <asm/current.h>
@@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long cpuid)
return pcpu_info[cpuid].hart_id;
}
+static inline unsigned int hartid_to_cpuid(unsigned long hartid)
+{
+ for ( unsigned int cpu = 0; cpu < ARRAY_SIZE(pcpu_info); cpu++ )
+ {
+ if ( hartid == cpuid_to_hartid(cpu) )
+ return cpu;
+ }
+
+ /* hartid isn't valid for some reason */
+ return NR_CPUS;
+}
+
static inline void set_cpuid_to_hartid(unsigned long cpuid,
unsigned long hartid)
{
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v6 1/7] xen/riscv: imsic_init() implementation
2025-07-07 9:01 ` [PATCH v6 1/7] xen/riscv: imsic_init() implementation Oleksii Kurochko
@ 2025-07-08 13:52 ` Jan Beulich
2025-07-10 10:47 ` Oleksii Kurochko
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2025-07-08 13:52 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
On 07.07.2025 11:01, Oleksii Kurochko wrote:
> 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
I'm curious though:
> --- a/xen/arch/riscv/include/asm/smp.h
> +++ b/xen/arch/riscv/include/asm/smp.h
> @@ -3,6 +3,7 @@
> #define ASM__RISCV__SMP_H
>
> #include <xen/cpumask.h>
> +#include <xen/macros.h>
> #include <xen/percpu.h>
>
> #include <asm/current.h>
> @@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long cpuid)
> return pcpu_info[cpuid].hart_id;
> }
>
> +static inline unsigned int hartid_to_cpuid(unsigned long hartid)
> +{
> + for ( unsigned int cpu = 0; cpu < ARRAY_SIZE(pcpu_info); cpu++ )
> + {
> + if ( hartid == cpuid_to_hartid(cpu) )
> + return cpu;
> + }
> +
> + /* hartid isn't valid for some reason */
> + return NR_CPUS;
> +}
Since there's no FIXME or alike here, is this really intended to remain this
way? With many CPUs this form of lookup can be pretty inefficient.
JAn
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v6 1/7] xen/riscv: imsic_init() implementation
2025-07-08 13:52 ` Jan Beulich
@ 2025-07-10 10:47 ` Oleksii Kurochko
0 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-10 10:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]
On 7/8/25 3:52 PM, Jan Beulich wrote:
> On 07.07.2025 11:01, Oleksii Kurochko wrote:
>> 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>
> Acked-by: Jan Beulich <jbeulich@suse.com
>
> I'm curious though:
>
>> --- a/xen/arch/riscv/include/asm/smp.h
>> +++ b/xen/arch/riscv/include/asm/smp.h
>> @@ -3,6 +3,7 @@
>> #define ASM__RISCV__SMP_H
>>
>> #include <xen/cpumask.h>
>> +#include <xen/macros.h>
>> #include <xen/percpu.h>
>>
>> #include <asm/current.h>
>> @@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long cpuid)
>> return pcpu_info[cpuid].hart_id;
>> }
>>
>> +static inline unsigned int hartid_to_cpuid(unsigned long hartid)
>> +{
>> + for ( unsigned int cpu = 0; cpu < ARRAY_SIZE(pcpu_info); cpu++ )
>> + {
>> + if ( hartid == cpuid_to_hartid(cpu) )
>> + return cpu;
>> + }
>> +
>> + /* hartid isn't valid for some reason */
>> + return NR_CPUS;
>> +}
> Since there's no FIXME or alike here, is this really intended to remain this
> way? With many CPUs this form of lookup can be pretty inefficient.
In the case with a big amount of CPUs it will require to update it. I will create
TODO task in my repo to not forget to update it in a future.
Thanks.
~ Oleksii
[-- Attachment #2: Type: text/html, Size: 3386 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 2/7] xen/riscv: aplic_init() implementation
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 1/7] xen/riscv: imsic_init() implementation Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-08 13:58 ` Jan Beulich
2025-07-10 11:56 ` Jan Beulich
2025-07-07 9:01 ` [PATCH v6 3/7] xen/riscv: introduce intc_init() and helpers Oleksii Kurochko
` (4 subsequent siblings)
6 siblings, 2 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
aplic_init() function does the following few things:
- checks that IMSIC in device tree node ( by checking msi-parent property
in APLIC node ) is present as current one implmenetaion of AIA is
supported only MSI method.
- initialize IMSIC based on IMSIC device tree node
- Read value of APLIC's paddr start/end and size.
- Map aplic.regs
- Setup APLIC initial state interrupts (disable all interrupts, set
interrupt type and default priority, confgifure APLIC domaincfg) by
calling aplic_init_hw_interrutps().
aplic_init() is based on the code from [1] and [2].
Since Microchip originally developed aplic.c, an internal discussion with
them led to the decision to use the MIT license.
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
[2] https://gitlab.com/xen-project/people/olkur/xen/-/commit/392a531bfad39bf4656ce8128e004b241b8b3f3e
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
- Code style fixes.
- Shorten message string in aplic_init().
- Add hex offset for APLIC regs in struct aplic_regs.
---
Changes in V5:
- Fix a typo in panic massege in aplic_init(): s/Failded/Failed.
- Correct size check: (!IS_ALIGNED(size, KB(4)) && (size < KB(16))) ->
(!IS_ALIGNED(size, KB(4)) || (size < KB(16))).
- Avoid wrapping format strings across lines.
---
Changes in V4:
- s/ASM__RISCV_PRIV_APLIC_H/ASM_RISCV_PRIV_APLIC_H
- Drop APLIC_MAX_NUM_WIRED_IRQ_SOURCES and use ARRAY_SIZE(aplic.regs->sourcecfg).
- Don't use 'else if' for the case when the earlier if() ends in an
unconditional control flow change.
- Use const for aplic_ops and drop __ro_after_init.
- Add checks of APLIC's memory-mapped control region physical address and
size.
- s/ASM__RISCV__APLIC_H/ASM_RISCV_APLIC_H.
- Use unsigned int in defintion of APLIC_DOMAINCFG_IE and APLIC_DOMAINCFG_DM.
- set aplic_info.num_irqs to ARRAY_SIZE(aplic.regs->sourcecfg) if DT node
provides too much.
- Add some constraints for aplic.paddr_start and aplic.size.
---
Changes in V3:
- Correct the comment on top of aplic-priv.h:
xen/arch/riscv/aplic.h -> .../aplic-priv.h
- Add __iomem for regs member of aplic_priv structure.
- [aplic_init_hw_interrupts] Use ~0U instead of -1U in aplic_init_hw_interrupts()
to disable all interrupts.
- [aplic_init_hw_interrupts] Start 'i' (for-cycle variable) from 0, not from 1.
- [aplic_init()] Declare imsic_node as pointer-to-const.
- Use decimal for arrays in struct aplic_regs.
- [aplic_init()] Check that aplic_info.num_irqs are less then 1023.
- [aplic_init()] Drop out check of IMSIC's node interrupt-extended property
from aplic_init().
---
Changes in V2:
- use __ro_after_init for aplic_ops.
- s/nr_irqs/num_irqs.
- s/dt_processor_hartid/dt_processor_cpuid.
- Drop confusing comment in aplic_init_hw_interrupts().
- Code style fixes.
- Drop years for Copyright.
- Revert changes which drop nr_irq define from asm/irq.h,
it shouldn't be, at least, part of this patch.
- Drop paddr_enf from struct aplic_regs. It is enough to have pair
(paddr_start, size).
- Make struct aplic_priv of asm/aplic.h private by moving it to
riscv/aplic-priv.h.
- Add the comment above the initialization of APLIC's target register.
- use writel() to access APLIC's registers.
- use 'unsinged int' for local variable i in aplic_init_hw_interrupts.
- Add the check that all modes in interrupts-extended property of
imsic node are equal. And drop rc != EOVERFLOW when interrupts-extended
property is read.
- Add cf_check to aplic_init().
- Fix a cycle of clrie register initialization in aplic_init_hw_interrupts().
Previous implementation leads to out-of-boundary.
- Declare member num_irqs in struct intc_info as it is used by APLIC code.
---
xen/arch/riscv/aplic-priv.h | 34 ++++++++++
xen/arch/riscv/aplic.c | 103 +++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/aplic.h | 64 ++++++++++++++++++
xen/arch/riscv/include/asm/intc.h | 3 +
4 files changed, 204 insertions(+)
create mode 100644 xen/arch/riscv/aplic-priv.h
create mode 100644 xen/arch/riscv/include/asm/aplic.h
diff --git a/xen/arch/riscv/aplic-priv.h b/xen/arch/riscv/aplic-priv.h
new file mode 100644
index 0000000000..71792bbf76
--- /dev/null
+++ b/xen/arch/riscv/aplic-priv.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/aplic-priv.h
+ *
+ * Private part of aplic.h header.
+ *
+ * RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) Microchip.
+ * Copyright (c) Vates.
+ */
+
+#ifndef ASM_RISCV_PRIV_APLIC_H
+#define ASM_RISCV_PRIV_APLIC_H
+
+#include <xen/types.h>
+
+#include <asm/aplic.h>
+#include <asm/imsic.h>
+
+struct aplic_priv {
+ /* Base physical address and size */
+ paddr_t paddr_start;
+ size_t size;
+
+ /* Registers */
+ volatile struct aplic_regs __iomem *regs;
+
+ /* IMSIC configuration */
+ const struct imsic_config *imsic_cfg;
+};
+
+#endif /* ASM_RISCV_PRIV_APLIC_H */
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 10ae81f7ac..0b0101ebc1 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -9,19 +9,118 @@
* Copyright (c) 2024-2025 Vates
*/
+#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/irq.h>
+#include <xen/mm.h>
#include <xen/sections.h>
#include <xen/types.h>
+#include <xen/vmap.h>
+
+#include "aplic-priv.h"
#include <asm/device.h>
+#include <asm/imsic.h>
#include <asm/intc.h>
+#include <asm/riscv_encoding.h>
+
+#define APLIC_DEFAULT_PRIORITY 1
+
+static struct aplic_priv aplic;
static struct intc_info __ro_after_init aplic_info = {
.hw_version = INTC_APLIC,
};
+static void __init aplic_init_hw_interrupts(void)
+{
+ unsigned int i;
+
+ /* Disable all interrupts */
+ for ( i = 0; i < ARRAY_SIZE(aplic.regs->clrie); i++)
+ writel(~0U, &aplic.regs->clrie[i]);
+
+ /* Set interrupt type and default priority for all interrupts */
+ for ( i = 0; i < aplic_info.num_irqs; i++ )
+ {
+ writel(0, &aplic.regs->sourcecfg[i]);
+ /*
+ * Low bits of target register contains Interrupt Priority bits which
+ * can't be zero according to AIA spec.
+ * Thereby they are initialized to APLIC_DEFAULT_PRIORITY.
+ */
+ writel(APLIC_DEFAULT_PRIORITY, &aplic.regs->target[i]);
+ }
+
+ writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
+}
+
+static int __init cf_check aplic_init(void)
+{
+ dt_phandle imsic_phandle;
+ const __be32 *prop;
+ uint64_t size, paddr;
+ const struct dt_device_node *imsic_node;
+ const struct dt_device_node *node = aplic_info.node;
+ int rc;
+
+ /* Check for associated imsic node */
+ if ( !dt_property_read_u32(node, "msi-parent", &imsic_phandle) )
+ panic("%s: IDC mode not supported\n", node->full_name);
+
+ imsic_node = dt_find_node_by_phandle(imsic_phandle);
+ if ( !imsic_node )
+ panic("%s: unable to find IMSIC node\n", node->full_name);
+
+ rc = imsic_init(imsic_node);
+ if ( rc == IRQ_M_EXT )
+ /* Machine mode imsic node, ignore this aplic node */
+ return 0;
+
+ if ( rc )
+ panic("%s: Failed to initialize IMSIC\n", node->full_name);
+
+ /* Find out number of interrupt sources */
+ if ( !dt_property_read_u32(node, "riscv,num-sources",
+ &aplic_info.num_irqs) )
+ panic("%s: failed to get number of interrupt sources\n",
+ node->full_name);
+
+ if ( aplic_info.num_irqs > ARRAY_SIZE(aplic.regs->sourcecfg) )
+ aplic_info.num_irqs = ARRAY_SIZE(aplic.regs->sourcecfg);
+
+ prop = dt_get_property(node, "reg", NULL);
+ dt_get_range(&prop, node, &paddr, &size);
+ if ( !paddr )
+ panic("%s: first MMIO resource not found\n", node->full_name);
+
+ if ( !IS_ALIGNED(paddr, KB(4)) )
+ panic("%s: paddr of memory-mapped control region should be 4Kb aligned:%#lx\n",
+ __func__, paddr);
+
+ if ( !IS_ALIGNED(size, KB(4)) || (size < KB(16)) )
+ panic("%s: control region size must be >= 16KB and 4KB-aligned:%#lx\n",
+ __func__, size);
+
+ aplic.paddr_start = paddr;
+ aplic.size = size;
+
+ aplic.regs = ioremap(paddr, size);
+ if ( !aplic.regs )
+ panic("%s: unable to map\n", node->full_name);
+
+ /* Setup initial state APLIC interrupts */
+ aplic_init_hw_interrupts();
+
+ return 0;
+}
+
+static const struct intc_hw_operations aplic_ops = {
+ .info = &aplic_info,
+ .init = aplic_init,
+};
+
static int cf_check aplic_irq_xlate(const uint32_t *intspec,
unsigned int intsize,
unsigned int *out_hwirq,
@@ -53,8 +152,12 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
aplic_info.node = node;
+ aplic.imsic_cfg = imsic_get_config();
+
dt_irq_xlate = aplic_irq_xlate;
+ register_intc_ops(&aplic_ops);
+
return 0;
}
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
new file mode 100644
index 0000000000..1388a977e6
--- /dev/null
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/asm/include/aplic.h
+ *
+ * RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) Microchip.
+ */
+
+#ifndef ASM_RISCV_APLIC_H
+#define ASM_RISCV_APLIC_H
+
+#include <xen/types.h>
+
+#include <asm/imsic.h>
+
+#define APLIC_DOMAINCFG_IE BIT(8, U)
+#define APLIC_DOMAINCFG_DM BIT(2, U)
+
+struct aplic_regs {
+ uint32_t domaincfg; /* 0x0000 */
+ uint32_t sourcecfg[1023]; /* 0x0004 */
+ uint8_t _reserved1[3008]; /* 0x1000 */
+
+ uint32_t mmsiaddrcfg; /* 0x1BC0 */
+ uint32_t mmsiaddrcfgh; /* 0x1BC4 */
+ uint32_t smsiaddrcfg; /* 0x1BC8 */
+ uint32_t smsiaddrcfgh; /* 0x1BCC */
+ uint8_t _reserved2[48]; /* 0x1BD0 */
+
+ uint32_t setip[32]; /* 0x1C00 */
+ uint8_t _reserved3[92]; /* 0x1C80 */
+
+ uint32_t setipnum; /* 0x1CDC */
+ uint8_t _reserved4[32]; /* 0x1CE0 */
+
+ uint32_t in_clrip[32]; /* 0x1D00 */
+ uint8_t _reserved5[92]; /* 0x1D80 */
+
+ uint32_t clripnum; /* 0x1DDC */
+ uint8_t _reserved6[32]; /* 0x1DE0 */
+
+ uint32_t setie[32]; /* 0x1E00 */
+ uint8_t _reserved7[92]; /* 0x1E80 */
+
+ uint32_t setienum; /* 0x1EDC */
+ uint8_t _reserved8[32]; /* 0x1EE0 */
+
+ uint32_t clrie[32]; /* 0x1F00 */
+ uint8_t _reserved9[92]; /* 0x1F80 */
+
+ uint32_t clrienum; /* 0x1FDC */
+ uint8_t _reserved10[32]; /* 0x1FF0 */
+
+ uint32_t setipnum_le; /* 0x2000 */
+ uint32_t setipnum_be; /* 0x2004 */
+ uint8_t _reserved11[4088]; /* 0x2008 */
+
+ uint32_t genmsi; /* 0x3000 */
+ uint32_t target[1023]; /* 0x3008 */
+};
+
+#endif /* ASM_RISCV_APLIC_H */
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 434c9d0781..3c4b211f58 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -19,6 +19,9 @@ struct irq_desc;
struct intc_info {
enum intc_version hw_version;
const struct dt_device_node *node;
+
+ /* number of irqs */
+ unsigned int num_irqs;
};
struct intc_hw_operations {
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v6 2/7] xen/riscv: aplic_init() implementation
2025-07-07 9:01 ` [PATCH v6 2/7] xen/riscv: aplic_init() implementation Oleksii Kurochko
@ 2025-07-08 13:58 ` Jan Beulich
2025-07-10 11:19 ` Oleksii Kurochko
2025-07-10 11:56 ` Jan Beulich
1 sibling, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2025-07-08 13:58 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
On 07.07.2025 11:01, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic-priv.h
> + *
> + * Private part of aplic.h header.
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates.
> + */
> +
> +#ifndef ASM_RISCV_PRIV_APLIC_H
> +#define ASM_RISCV_PRIV_APLIC_H
> +
> +#include <xen/types.h>
> +
> +#include <asm/aplic.h>
> +#include <asm/imsic.h>
Why is the latter one needed here? Certainly not for ...
> +struct aplic_priv {
> + /* Base physical address and size */
> + paddr_t paddr_start;
> + size_t size;
> +
> + /* Registers */
> + volatile struct aplic_regs __iomem *regs;
> +
> + /* IMSIC configuration */
> + const struct imsic_config *imsic_cfg;
... this field decl (which itself serves as a forward decl of the struct
tag).
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -9,19 +9,118 @@
> * Copyright (c) 2024-2025 Vates
> */
>
> +#include <xen/device_tree.h>
> #include <xen/errno.h>
> #include <xen/init.h>
> #include <xen/irq.h>
> +#include <xen/mm.h>
> #include <xen/sections.h>
> #include <xen/types.h>
> +#include <xen/vmap.h>
> +
> +#include "aplic-priv.h"
>
> #include <asm/device.h>
> +#include <asm/imsic.h>
> #include <asm/intc.h>
> +#include <asm/riscv_encoding.h>
> +
> +#define APLIC_DEFAULT_PRIORITY 1
> +
> +static struct aplic_priv aplic;
Is this altered post-init? IOW can it be __ro_after_init? Or otherwise at
least __read_mostly?
With these two taken care of (one way or another):
Acked-by: Jan Beulich <jbeulich@suse.com>
If you clarify what is wanted / needed, I'm also happy to make adjustments
while committing.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v6 2/7] xen/riscv: aplic_init() implementation
2025-07-08 13:58 ` Jan Beulich
@ 2025-07-10 11:19 ` Oleksii Kurochko
2025-07-10 11:29 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-10 11:19 UTC (permalink / raw)
To: Jan Beulich
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]
On 7/8/25 3:58 PM, Jan Beulich wrote:
> On 07.07.2025 11:01, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/arch/riscv/aplic-priv.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +/*
>> + * xen/arch/riscv/aplic-priv.h
>> + *
>> + * Private part of aplic.h header.
>> + *
>> + * RISC-V Advanced Platform-Level Interrupt Controller support
>> + *
>> + * Copyright (c) Microchip.
>> + * Copyright (c) Vates.
>> + */
>> +
>> +#ifndef ASM_RISCV_PRIV_APLIC_H
>> +#define ASM_RISCV_PRIV_APLIC_H
>> +
>> +#include <xen/types.h>
>> +
>> +#include <asm/aplic.h>
>> +#include <asm/imsic.h>
> Why is the latter one needed here? Certainly not for ...
>
>> +struct aplic_priv {
>> + /* Base physical address and size */
>> + paddr_t paddr_start;
>> + size_t size;
>> +
>> + /* Registers */
>> + volatile struct aplic_regs __iomem *regs;
>> +
>> + /* IMSIC configuration */
>> + const struct imsic_config *imsic_cfg;
> ... this field decl (which itself serves as a forward decl of the struct
> tag).
The purpose was to deal with a forward decl but I agree that in this case
inclusion of <asm/imsic.h> could be dropped.
>
>> --- a/xen/arch/riscv/aplic.c
>> +++ b/xen/arch/riscv/aplic.c
>> @@ -9,19 +9,118 @@
>> * Copyright (c) 2024-2025 Vates
>> */
>>
>> +#include <xen/device_tree.h>
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> #include <xen/irq.h>
>> +#include <xen/mm.h>
>> #include <xen/sections.h>
>> #include <xen/types.h>
>> +#include <xen/vmap.h>
>> +
>> +#include "aplic-priv.h"
>>
>> #include <asm/device.h>
>> +#include <asm/imsic.h>
>> #include <asm/intc.h>
>> +#include <asm/riscv_encoding.h>
>> +
>> +#define APLIC_DEFAULT_PRIORITY 1
>> +
>> +static struct aplic_priv aplic;
> Is this altered post-init? IOW can it be __ro_after_init? Or otherwise at
> least __read_mostly?
Looking at the current downstream code there is, at least one case, where aplic->regs
are changing (during vaplic_emulate_store()).
So __read_mostly would be better in this case.
> With these two taken care of (one way or another):
> Acked-by: Jan Beulich<jbeulich@suse.com>
> If you clarify what is wanted / needed, I'm also happy to make adjustments
> while committing.
It would be nice to do the following:
- Drop the inclusion of <asm/imsic.h> in aplic-priv.h.
- Add __read_mostly to the definition of the aplic variable.
Thanks for adjustments.
~ Oleksii
[-- Attachment #2: Type: text/html, Size: 3587 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v6 2/7] xen/riscv: aplic_init() implementation
2025-07-10 11:19 ` Oleksii Kurochko
@ 2025-07-10 11:29 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2025-07-10 11:29 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
On 10.07.2025 13:19, Oleksii Kurochko wrote:
> On 7/8/25 3:58 PM, Jan Beulich wrote:
>> On 07.07.2025 11:01, Oleksii Kurochko wrote:
>>> +static struct aplic_priv aplic;
>> Is this altered post-init? IOW can it be __ro_after_init? Or otherwise at
>> least __read_mostly?
>
> Looking at the current downstream code there is, at least one case, where aplic->regs
> are changing (during vaplic_emulate_store()).
> So __read_mostly would be better in this case.
>
>> With these two taken care of (one way or another):
>> Acked-by: Jan Beulich<jbeulich@suse.com>
>> If you clarify what is wanted / needed, I'm also happy to make adjustments
>> while committing.
>
> It would be nice to do the following:
> - Drop the inclusion of <asm/imsic.h> in aplic-priv.h.
> - Add __read_mostly to the definition of the aplic variable.
Actually - no. Patch 4 adds a spin lock in the structure. That way it's
definitely not mostly read.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/7] xen/riscv: aplic_init() implementation
2025-07-07 9:01 ` [PATCH v6 2/7] xen/riscv: aplic_init() implementation Oleksii Kurochko
2025-07-08 13:58 ` Jan Beulich
@ 2025-07-10 11:56 ` Jan Beulich
1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2025-07-10 11:56 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Alistair Francis, Bob Eshleman, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Romain Caritey, xen-devel
On 07.07.2025 11:01, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic-priv.h
> + *
> + * Private part of aplic.h header.
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates.
> + */
> +
> +#ifndef ASM_RISCV_PRIV_APLIC_H
> +#define ASM_RISCV_PRIV_APLIC_H
While, as indicated in an earlier reply, I didn't make the section placement
change originally suggested, I did take the liberty to correct the order of
name components of this header guard.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 3/7] xen/riscv: introduce intc_init() and helpers
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 1/7] xen/riscv: imsic_init() implementation Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 2/7] xen/riscv: aplic_init() implementation Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 4/7] xen/riscv: implementation of aplic and imsic operations Oleksii Kurochko
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
Introduce intc_init() to initialize the interrupt controller using the
registered hardware ops.
Also add intc_route_irq_to_xen() to route IRQs to Xen, with support for
setting IRQ type and priority via new internal helpers intc_set_irq_type()
and intc_set_irq_priority().
Call intc_init() to do basic initialization steps for APLIC and IMSIC.
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V5-V6:
- Nothing changed. Only rebase.
---
Changes in V4:
- Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in V3:
- Drop ASSERIT(intc_hw_ops) in intc_init().
- Drop ASSERT(intc_hw_ops) in intc_set_irq_type() and
intc_set_irq_priority() as intc_init() will be called first and so if it
won't crash, then intc_hw_ops is registered.
---
Changes in V2:
- This patch was part of "xen/riscv: Introduce intc_hw_operations abstraction"
and splitted to have ability to merge patch "xen/riscv: initialize interrupt controller"
to the current patch (where intc_init() call is actually introduced).
- Add checks of that callbacks aren't set to NULL in intc_set_irq_priority()
and intc_set_irq_type().
- add num_irqs member to struct intc_info as it is used now in
intc_route_irq_to_xen().
- Add ASSERT(spin_is_locked(&desc->lock)) to intc_set_irq_priority() in
the case this function will be called outside intc_route_irq_to_xen().
---
xen/arch/riscv/include/asm/intc.h | 4 +++
xen/arch/riscv/intc.c | 41 +++++++++++++++++++++++++++++++
xen/arch/riscv/setup.c | 2 ++
3 files changed, 47 insertions(+)
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 3c4b211f58..a11b7aa55e 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -43,4 +43,8 @@ void intc_preinit(void);
void register_intc_ops(const struct intc_hw_operations *ops);
+void intc_init(void);
+
+void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
+
#endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 1ecd651bf3..f2823267a9 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -1,9 +1,12 @@
/* SPDX-License-Identifier: GPL-2.0-only */
#include <xen/acpi.h>
+#include <xen/bug.h>
#include <xen/device_tree.h>
#include <xen/init.h>
+#include <xen/irq.h>
#include <xen/lib.h>
+#include <xen/spinlock.h>
#include <asm/intc.h>
@@ -21,3 +24,41 @@ void __init intc_preinit(void)
else
panic("ACPI interrupt controller preinit() isn't implemented\n");
}
+
+void __init intc_init(void)
+{
+ if ( intc_hw_ops->init() )
+ panic("Failed to initialize the interrupt controller drivers\n");
+}
+
+/* desc->irq needs to be disabled before calling this function */
+static void intc_set_irq_type(struct irq_desc *desc, unsigned int type)
+{
+ ASSERT(desc->status & IRQ_DISABLED);
+ ASSERT(spin_is_locked(&desc->lock));
+ ASSERT(type != IRQ_TYPE_INVALID);
+
+ if ( intc_hw_ops->set_irq_type )
+ intc_hw_ops->set_irq_type(desc, type);
+}
+
+static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
+{
+ ASSERT(spin_is_locked(&desc->lock));
+
+ if ( intc_hw_ops->set_irq_priority )
+ intc_hw_ops->set_irq_priority(desc, priority);
+}
+
+void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+ ASSERT(desc->status & IRQ_DISABLED);
+ ASSERT(spin_is_locked(&desc->lock));
+ /* Can't route interrupts that don't exist */
+ ASSERT(intc_hw_ops && desc->irq < intc_hw_ops->info->num_irqs);
+
+ desc->handler = intc_hw_ops->host_irq_type;
+
+ intc_set_irq_type(desc, desc->arch.type);
+ intc_set_irq_priority(desc, priority);
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 8bcd19218d..0e7398159c 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -134,6 +134,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
intc_preinit();
+ intc_init();
+
printk("All set up\n");
machine_halt();
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v6 4/7] xen/riscv: implementation of aplic and imsic operations
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
` (2 preceding siblings ...)
2025-07-07 9:01 ` [PATCH v6 3/7] xen/riscv: introduce intc_init() and helpers Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 5/7] xen/riscv: add external interrupt handling for hypervisor mode Oleksii Kurochko
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=all, Size: 15805 bytes --]
Introduce interrupt controller descriptor for host APLIC to describe
the low-lovel hardare. It includes implementation of the following functions:
- aplic_irq_startup()
- aplic_irq_enable()
- aplic_irq_disable()
- aplic_set_irq_affinity()
As APLIC is used in MSI mode it requires to enable/disable interrupts not
only for APLIC but also for IMSIC. Thereby for the purpose of
aplic_irq_{enable,disable}() it is introduced imsic_irq_{enable,disable)().
For the purpose of aplic_set_irq_affinity() aplic_get_cpu_from_mask() is
introduced to get hart id.
Also, introduce additional interrupt controller h/w operations and
host_irq_type for APLIC:
- aplic_host_irq_type
Patch is based on the code from [1].
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7390e2365828b83e27ead56b03114a56e3699dd5
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
- Fix code style issue: start multi-word comments with a capital letter.
- Align properly comment in imsic_irq_enable().
---
Changes in V5:
- Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in V4:
- Code style fixes.
- Add cf_check for hook functions.
- Use min() macros instead of open-coding it in imsic_local_eix_update().
- Update the comment above ASSERT() in imsic_irq_disable():
s/aplic_irq_enable/aplic_irq_disable.
- Add an explanatory comment above initializing of hhxs in
aplic_set_irq_affinity().
---
Changes in V3:
- Update the lock above lock member of struct aplic_priv and imsic_config struct.
- Use spin_{un}lock() in aplic_irq_{enable,disable}() as it is IRQ-safe.
Also drop local variable 'flags'.
- Add ASSERT(spin_is_locked(&desc->lock)) to aplic_set_irq_affinity() and
aplic_set_irq_type().
- Use an initializer instead of spin_lock_init() for aplic.lock.
- Drop "(l)" in the comment in imsic_irq_enable() as it doesn't point to
anything.
- Use ASSERT(!local_irq_is_enabled()) + spin_lock() in imsic_irq_{enable,disable}().
- Use an initializer instead of spin_lock_init() for imsic_config.lock.
---
Changes in V2:
- Move imsic_ids_local_delivery() and connected to it parts to the current
patch to fix compilation issue. Also, add __init for
imsic_ids_local_delivery().
- Move introduction of aplic_set_irq_type() and aplic_set_irq_priority()
to patch [PATCH v1 12/14] xen/riscv: implement setup_irq() where they
really started to be used.
- Update the commit message.
- Drop is_used variable for imsic_cfg and use (aplic.regs->domaincfg & APLIC_DOMAINCFG_DM) instead.
- Use writel() to write to APLIC regs.
- Drop aplic_irq_shutdown() and use aplic_irq_disable explicitly.
- Drop local variable cpu in aplic_get_cpu_from_mask():
Use cpu_online_map instead of cpu_possible_map.
Remame possible_mask to mask.
- Code style fixes.
- Move spin_lock(&aplic.lock) down before write to the register in aplic_set_irq_affinity.
- Make aplic_host_irq_type const.
- imsic_local_eix_update() updates:
- move unsigned long isel, ireg; to inner loop.
- Drop unnecessary parentheses.
- Optimize inner loop of ireg's setting.
- Drop aplic_irq_ack() and aplic_host_irq_end() as they do nothing.
- Rename s/hwirq/irq.
- Add explanatory comment to imsic_irq_enable() about why there is not -1 for IRQ in
comparison with APLIC's sourcecfg.
- Use IMSIC_MMIO_PAGE_SHIFT instead of constant 12 in aplic_set_irq_affinity().
- s/aplic_host_irq_type/aplic_xen_irq_type
- Drop set/clear of IRQ_DISABLED bit in aplic_{enable,disable}() as guest will always
first request an interrupt and then only an interrupt will be enabled.
(for example, in Arm, the physical interrupts would be enabled when the
interrupt is initially routed. This could lead to problem because the guest
will usually boot with interrupt disabled.)
---
xen/arch/riscv/aplic-priv.h | 4 +
xen/arch/riscv/aplic.c | 123 ++++++++++++++++++++++++++++-
xen/arch/riscv/imsic.c | 122 +++++++++++++++++++++++++++-
xen/arch/riscv/include/asm/aplic.h | 2 +
xen/arch/riscv/include/asm/imsic.h | 18 +++++
5 files changed, 267 insertions(+), 2 deletions(-)
diff --git a/xen/arch/riscv/aplic-priv.h b/xen/arch/riscv/aplic-priv.h
index 71792bbf76..b70e353ae4 100644
--- a/xen/arch/riscv/aplic-priv.h
+++ b/xen/arch/riscv/aplic-priv.h
@@ -14,6 +14,7 @@
#ifndef ASM_RISCV_PRIV_APLIC_H
#define ASM_RISCV_PRIV_APLIC_H
+#include <xen/spinlock.h>
#include <xen/types.h>
#include <asm/aplic.h>
@@ -27,6 +28,9 @@ struct aplic_priv {
/* Registers */
volatile struct aplic_regs __iomem *regs;
+ /* Lock to protect access to APLIC's registers */
+ spinlock_t lock;
+
/* IMSIC configuration */
const struct imsic_config *imsic_cfg;
};
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 0b0101ebc1..edf4db3113 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -15,6 +15,7 @@
#include <xen/irq.h>
#include <xen/mm.h>
#include <xen/sections.h>
+#include <xen/spinlock.h>
#include <xen/types.h>
#include <xen/vmap.h>
@@ -23,11 +24,14 @@
#include <asm/device.h>
#include <asm/imsic.h>
#include <asm/intc.h>
+#include <asm/io.h>
#include <asm/riscv_encoding.h>
#define APLIC_DEFAULT_PRIORITY 1
-static struct aplic_priv aplic;
+static struct aplic_priv aplic = {
+ .lock = SPIN_LOCK_UNLOCKED,
+};
static struct intc_info __ro_after_init aplic_info = {
.hw_version = INTC_APLIC,
@@ -116,9 +120,126 @@ static int __init cf_check aplic_init(void)
return 0;
}
+static void cf_check aplic_irq_enable(struct irq_desc *desc)
+{
+ /*
+ * TODO: Currently, APLIC is supported only with MSI interrupts.
+ * If APLIC without MSI interrupts is required in the future,
+ * this function will need to be updated accordingly.
+ */
+ ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
+
+ ASSERT(spin_is_locked(&desc->lock));
+
+ spin_lock(&aplic.lock);
+
+ /* Enable interrupt in IMSIC */
+ imsic_irq_enable(desc->irq);
+
+ /* Enable interrupt in APLIC */
+ writel(desc->irq, &aplic.regs->setienum);
+
+ spin_unlock(&aplic.lock);
+}
+
+static void cf_check aplic_irq_disable(struct irq_desc *desc)
+{
+ /*
+ * TODO: Currently, APLIC is supported only with MSI interrupts.
+ * If APLIC without MSI interrupts is required in the future,
+ * this function will need to be updated accordingly.
+ */
+ ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
+
+ ASSERT(spin_is_locked(&desc->lock));
+
+ spin_lock(&aplic.lock);
+
+ /* Disable interrupt in APLIC */
+ writel(desc->irq, &aplic.regs->clrienum);
+
+ /* Disable interrupt in IMSIC */
+ imsic_irq_disable(desc->irq);
+
+ spin_unlock(&aplic.lock);
+}
+
+static unsigned int cf_check aplic_irq_startup(struct irq_desc *desc)
+{
+ aplic_irq_enable(desc);
+
+ return 0;
+}
+
+static unsigned int aplic_get_cpu_from_mask(const cpumask_t *cpumask)
+{
+ cpumask_t mask;
+
+ cpumask_and(&mask, cpumask, &cpu_online_map);
+
+ return cpumask_any(&mask);
+}
+
+static void cf_check aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t *mask)
+{
+ unsigned int cpu;
+ uint64_t group_index, base_ppn;
+ uint32_t hhxw, lhxw, hhxs, value;
+ const struct imsic_config *imsic = aplic.imsic_cfg;
+
+ /*
+ * TODO: Currently, APLIC is supported only with MSI interrupts.
+ * If APLIC without MSI interrupts is required in the future,
+ * this function will need to be updated accordingly.
+ */
+ ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
+
+ ASSERT(!cpumask_empty(mask));
+
+ ASSERT(spin_is_locked(&desc->lock));
+
+ cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
+ hhxw = imsic->group_index_bits;
+ lhxw = imsic->hart_index_bits;
+ /*
+ * Although this variable is used only once in the calculation of
+ * group_index, and it might seem that hhxs could be defined as:
+ * hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT;
+ * and then the addition of IMSIC_MMIO_PAGE_SHIFT could be omitted
+ * when calculating the group index.
+ * It was done intentionally this way to follow the formula from
+ * the AIA specification for calculating the MSI address.
+ */
+ hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2;
+ base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT;
+
+ /* Update hart and EEID in the target register */
+ group_index = (base_ppn >> (hhxs + IMSIC_MMIO_PAGE_SHIFT)) &
+ (BIT(hhxw, UL) - 1);
+ value = desc->irq;
+ value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
+ value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT);
+
+ spin_lock(&aplic.lock);
+
+ writel(value, &aplic.regs->target[desc->irq - 1]);
+
+ spin_unlock(&aplic.lock);
+}
+
+static const hw_irq_controller aplic_xen_irq_type = {
+ .typename = "aplic",
+ .startup = aplic_irq_startup,
+ .shutdown = aplic_irq_disable,
+ .enable = aplic_irq_enable,
+ .disable = aplic_irq_disable,
+ .set_affinity = aplic_set_irq_affinity,
+};
+
static const struct intc_hw_operations aplic_ops = {
.info = &aplic_info,
.init = aplic_init,
+ .host_irq_type = &aplic_xen_irq_type,
};
static int cf_check aplic_irq_xlate(const uint32_t *intspec,
diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index 63f4233035..a4460576f6 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -29,7 +29,124 @@ struct imsic_mmios {
unsigned long size;
};
-static struct imsic_config imsic_cfg;
+static struct imsic_config imsic_cfg = {
+ .lock = SPIN_LOCK_UNLOCKED,
+};
+
+#define IMSIC_DISABLE_EIDELIVERY 0
+#define IMSIC_ENABLE_EIDELIVERY 1
+#define IMSIC_DISABLE_EITHRESHOLD 1
+#define IMSIC_ENABLE_EITHRESHOLD 0
+
+#define imsic_csr_write(c, v) \
+do { \
+ csr_write(CSR_SISELECT, c); \
+ csr_write(CSR_SIREG, v); \
+} while (0)
+
+#define imsic_csr_set(c, v) \
+do { \
+ csr_write(CSR_SISELECT, c); \
+ csr_set(CSR_SIREG, v); \
+} while (0)
+
+#define imsic_csr_clear(c, v) \
+do { \
+ csr_write(CSR_SISELECT, c); \
+ csr_clear(CSR_SIREG, v); \
+} while (0)
+
+void __init imsic_ids_local_delivery(bool enable)
+{
+ if ( enable )
+ {
+ imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD);
+ imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_ENABLE_EIDELIVERY);
+ }
+ else
+ {
+ imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_DISABLE_EITHRESHOLD);
+ imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_DISABLE_EIDELIVERY);
+ }
+}
+
+static void imsic_local_eix_update(unsigned long base_id, unsigned long num_id,
+ bool pend, bool val)
+{
+ unsigned long id = base_id, last_id = base_id + num_id;
+
+ while ( id < last_id )
+ {
+ unsigned long isel, ireg;
+ unsigned long start_id = id & (__riscv_xlen - 1);
+ unsigned long chunk = __riscv_xlen - start_id;
+ unsigned long count = min(last_id - id, chunk);
+
+ isel = id / __riscv_xlen;
+ isel *= __riscv_xlen / IMSIC_EIPx_BITS;
+ isel += pend ? IMSIC_EIP0 : IMSIC_EIE0;
+
+ ireg = GENMASK(start_id + count - 1, start_id);
+
+ id += count;
+
+ if ( val )
+ imsic_csr_set(isel, ireg);
+ else
+ imsic_csr_clear(isel, ireg);
+ }
+}
+
+void imsic_irq_enable(unsigned int irq)
+{
+ /*
+ * The only caller of imsic_irq_enable() is aplic_irq_enable(), which
+ * already runs with IRQs disabled. Therefore, there's no need to use
+ * spin_lock_irqsave() in this function.
+ *
+ * This ASSERT is added as a safeguard: if imsic_irq_enable() is ever
+ * called from a context where IRQs are not disabled,
+ * spin_lock_irqsave() should be used instead of spin_lock().
+ */
+ ASSERT(!local_irq_is_enabled());
+
+ spin_lock(&imsic_cfg.lock);
+ /*
+ * There is no irq - 1 here (look at aplic_set_irq_type()) because:
+ * From the spec:
+ * When an interrupt file supports distinct interrupt identities,
+ * valid identity numbers are between 1 and inclusive. The identity
+ * numbers within this range are said to be implemented by the interrupt
+ * file; numbers outside this range are not implemented. The number zero
+ * is never a valid interrupt identity.
+ * ...
+ * Bit positions in a valid eiek register that don’t correspond to a
+ * supported interrupt identity (such as bit 0 of eie0) are read-only zeros.
+ *
+ * So in EIx registers interrupt i corresponds to bit i in comparison wiht
+ * APLIC's sourcecfg which starts from 0.
+ */
+ imsic_local_eix_update(irq, 1, false, true);
+ spin_unlock(&imsic_cfg.lock);
+}
+
+void imsic_irq_disable(unsigned int irq)
+{
+ /*
+ * The only caller of imsic_irq_disable() is aplic_irq_disable(), which
+ * already runs with IRQs disabled. Therefore, there's no need to use
+ * spin_lock_irqsave() in this function.
+ *
+ * This ASSERT is added as a safeguard: if imsic_irq_disable() is ever
+ * called from a context where IRQs are not disabled,
+ * spin_lock_irqsave() should be used instead of spin_lock().
+ */
+ ASSERT(!local_irq_is_enabled());
+
+ spin_lock(&imsic_cfg.lock);
+ imsic_local_eix_update(irq, 1, false, false);
+ spin_unlock(&imsic_cfg.lock);
+}
/* Callers aren't intended to changed imsic_cfg so return const. */
const struct imsic_config *imsic_get_config(void)
@@ -355,6 +472,9 @@ int __init imsic_init(const struct dt_device_node *node)
goto imsic_init_err;
}
+ /* Enable local interrupt delivery */
+ imsic_ids_local_delivery(true);
+
imsic_cfg.msi = msi;
xvfree(mmios);
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index 1388a977e6..7d811d3522 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -18,6 +18,8 @@
#define APLIC_DOMAINCFG_IE BIT(8, U)
#define APLIC_DOMAINCFG_DM BIT(2, U)
+#define APLIC_TARGET_HART_IDX_SHIFT 18
+
struct aplic_regs {
uint32_t domaincfg; /* 0x0000 */
uint32_t sourcecfg[1023]; /* 0x0004 */
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index 9cd12365b1..378e49d933 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -11,6 +11,7 @@
#ifndef ASM_RISCV_IMSIC_H
#define ASM_RISCV_IMSIC_H
+#include <xen/spinlock.h>
#include <xen/types.h>
#define IMSIC_MMIO_PAGE_SHIFT 12
@@ -19,6 +20,15 @@
#define IMSIC_MIN_ID 63
#define IMSIC_MAX_ID 2047
+#define IMSIC_EIDELIVERY 0x70
+
+#define IMSIC_EITHRESHOLD 0x72
+
+#define IMSIC_EIP0 0x80
+#define IMSIC_EIPx_BITS 32
+
+#define IMSIC_EIE0 0xC0
+
struct imsic_msi {
paddr_t base_addr;
unsigned long offset;
@@ -45,6 +55,9 @@ struct imsic_config {
/* MSI */
const struct imsic_msi *msi;
+
+ /* Lock to protect access to IMSIC's stuff */
+ spinlock_t lock;
};
struct dt_device_node;
@@ -52,4 +65,9 @@ int imsic_init(const struct dt_device_node *node);
const struct imsic_config *imsic_get_config(void);
+void imsic_irq_enable(unsigned int hwirq);
+void imsic_irq_disable(unsigned int hwirq);
+
+void imsic_ids_local_delivery(bool enable);
+
#endif /* ASM_RISCV_IMSIC_H */
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v6 5/7] xen/riscv: add external interrupt handling for hypervisor mode
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
` (3 preceding siblings ...)
2025-07-07 9:01 ` [PATCH v6 4/7] xen/riscv: implementation of aplic and imsic operations Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 6/7] xen/riscv: implement setup_irq() Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 7/7] xen/riscv: add basic UART support Oleksii Kurochko
6 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=all, Size: 11655 bytes --]
Implement functions necessarry to have working external interrupts in
hypervisor mode. The following changes are done:
- Add a common function intc_handle_external_irq() to call APLIC specific
function to handle an interrupt.
- Update do_trap() function to handle IRQ_S_EXT case; add the check to catch
case when cause of trap is an interrupt.
- Add handle_interrrupt() member to intc_hw_operations structure.
- Enable local interrupt delivery for IMSIC by calling of
imsic_ids_local_delivery() in imsic_init(); additionally introduce helper
imsic_csr_write() to update IMSIC_EITHRESHOLD and IMSIC_EITHRESHOLD.
- Enable hypervisor external interrupts.
- Implement aplic_handler_interrupt() and use it to init ->handle_interrupt
member of intc_hw_operations for APLIC.
- Add implementation of do_IRQ() to dispatch the interrupt.
The current patch is based on the code from [1].
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7390e2365828b83e27ead56b03114a56e3699dd5
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
- Fix code style issue: start multi-word comments with a capital letter.
- Align properly comment in aplic_set_irq_type().
---
Changes in V4 - V5:
- Nothing changed. Only rebase.
---
Changes in V3:
- Add ASSERT(spin_is_locked(&desc->lock)) to aplic_set_irq_type().
- Fix code style for switch () in aplic_set_irq_type().
- Drop fallthrough between "case IRQ_TYPE_NONE: case IRQ_TYPE_INVALID:" as there
is no other statements in "case IRQ_TYPE_NONE".
- Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in V2:
- use BIT() macros instead of 1UL << bit_num in aplic.c.
- Drop passing of a cause to aplic_handle_interrupt() function. And update
prototype of handle_interrupt() callback.
- Drop ASSERT() in intc_handle_external_irqs() as it is useless.
- Code style fixes.
- Drop cause argument for intc_handle_external_irqs().
- Update the commit message: drop words that imsic_ids_local_delivery() is
implemented in this patch, it is only called.
- Add cf_check for aplic_handle_interrupt(), aplic_set_irq_type().
- Move forward declarations in asm/intc.h up.
- Use plain C operator instead if {clear,set}_bit() for desc->status as it
is always used under spinlock().
- use writel() for writing to APLIC's sourcecfg in aplic_set_irq_type().
---
xen/arch/riscv/aplic.c | 73 ++++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/aplic.h | 7 +++
xen/arch/riscv/include/asm/imsic.h | 1 +
xen/arch/riscv/include/asm/intc.h | 6 +++
xen/arch/riscv/include/asm/irq.h | 6 ++-
xen/arch/riscv/intc.c | 5 ++
xen/arch/riscv/irq.c | 47 +++++++++++++++++++
xen/arch/riscv/traps.c | 19 ++++++++
8 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index edf4db3113..739e8dab34 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -9,6 +9,7 @@
* Copyright (c) 2024-2025 Vates
*/
+#include <xen/const.h>
#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
@@ -227,6 +228,73 @@ static void cf_check aplic_set_irq_affinity(struct irq_desc *desc, const cpumask
spin_unlock(&aplic.lock);
}
+static void cf_check aplic_handle_interrupt(struct cpu_user_regs *regs)
+{
+ /* Disable to avoid more external interrupts */
+ csr_clear(CSR_SIE, BIT(IRQ_S_EXT, UL));
+
+ /* Clear the pending bit */
+ csr_clear(CSR_SIP, BIT(IRQ_S_EXT, UL));
+
+ /* Dispatch the interrupt */
+ do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT);
+
+ /* Enable external interrupts */
+ csr_set(CSR_SIE, BIT(IRQ_S_EXT, UL));
+}
+
+static void cf_check aplic_set_irq_type(struct irq_desc *desc,
+ unsigned int type)
+{
+ /*
+ * Interrupt 0 isn't possible based on the spec:
+ * Each of an APLIC’s interrupt sources has a fixed unique identity
+ * number in the range 1 to N, where N is the total number of sources at
+ * the APLIC. The number zero is not a valid interrupt identity number at
+ * an APLIC. The maximum number of interrupt sources an APLIC may support
+ * is 1023.
+ *
+ * Thereby interrupt 1 will correspond to bit 0 in sourcecfg[] register,
+ * interrupt 2 ->sourcecfg[1] and so on.
+ *
+ * And that is the reason why we need -1.
+ */
+ unsigned int irq_bit = desc->irq - 1;
+
+ ASSERT(spin_is_locked(&desc->lock));
+
+ spin_lock(&aplic.lock);
+
+ switch ( type )
+ {
+ case IRQ_TYPE_EDGE_RISING:
+ writel(APLIC_SOURCECFG_SM_EDGE_RISE, &aplic.regs->sourcecfg[irq_bit]);
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ writel(APLIC_SOURCECFG_SM_EDGE_FALL, &aplic.regs->sourcecfg[irq_bit]);
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ writel(APLIC_SOURCECFG_SM_LEVEL_HIGH, &aplic.regs->sourcecfg[irq_bit]);
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ writel(APLIC_SOURCECFG_SM_LEVEL_LOW, &aplic.regs->sourcecfg[irq_bit]);
+ break;
+
+ case IRQ_TYPE_NONE:
+ case IRQ_TYPE_INVALID:
+ writel(APLIC_SOURCECFG_SM_INACTIVE, &aplic.regs->sourcecfg[irq_bit]);
+ break;
+
+ default:
+ panic("%s: APLIC doesnt support IRQ type: 0x%x?\n", __func__, type);
+ }
+
+ spin_unlock(&aplic.lock);
+}
+
static const hw_irq_controller aplic_xen_irq_type = {
.typename = "aplic",
.startup = aplic_irq_startup,
@@ -240,6 +308,8 @@ static const struct intc_hw_operations aplic_ops = {
.info = &aplic_info,
.init = aplic_init,
.host_irq_type = &aplic_xen_irq_type,
+ .handle_interrupt = aplic_handle_interrupt,
+ .set_irq_type = aplic_set_irq_type,
};
static int cf_check aplic_irq_xlate(const uint32_t *intspec,
@@ -279,6 +349,9 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
register_intc_ops(&aplic_ops);
+ /* Enable supervisor external interrupt */
+ csr_set(CSR_SIE, BIT(IRQ_S_EXT, UL));
+
return 0;
}
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index 7d811d3522..b0724fe6f3 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -18,6 +18,13 @@
#define APLIC_DOMAINCFG_IE BIT(8, U)
#define APLIC_DOMAINCFG_DM BIT(2, U)
+#define APLIC_SOURCECFG_SM_INACTIVE 0x0
+#define APLIC_SOURCECFG_SM_DETACH 0x1
+#define APLIC_SOURCECFG_SM_EDGE_RISE 0x4
+#define APLIC_SOURCECFG_SM_EDGE_FALL 0x5
+#define APLIC_SOURCECFG_SM_LEVEL_HIGH 0x6
+#define APLIC_SOURCECFG_SM_LEVEL_LOW 0x7
+
#define APLIC_TARGET_HART_IDX_SHIFT 18
struct aplic_regs {
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index 378e49d933..c6c59215df 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -12,6 +12,7 @@
#define ASM_RISCV_IMSIC_H
#include <xen/spinlock.h>
+#include <xen/stdbool.h>
#include <xen/types.h>
#define IMSIC_MMIO_PAGE_SHIFT 12
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index a11b7aa55e..ecdc8a5e65 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -14,6 +14,7 @@ enum intc_version {
INTC_APLIC,
};
+struct cpu_user_regs;
struct irq_desc;
struct intc_info {
@@ -37,6 +38,9 @@ struct intc_hw_operations {
void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
/* Set IRQ priority */
void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
+
+ /* handle external interrupt */
+ void (*handle_interrupt)(struct cpu_user_regs *regs);
};
void intc_preinit(void);
@@ -47,4 +51,6 @@ void intc_init(void);
void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
+void intc_handle_external_irqs(struct cpu_user_regs *regs);
+
#endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index 84c3c2904d..94151eb083 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -33,16 +33,20 @@ struct arch_irq_desc {
unsigned int type;
};
+struct cpu_user_regs;
+struct dt_device_node;
+
static inline void arch_move_irqs(struct vcpu *v)
{
BUG_ON("unimplemented");
}
-struct dt_device_node;
int platform_get_irq(const struct dt_device_node *device, int index);
void init_IRQ(void);
+void do_IRQ(struct cpu_user_regs *regs, unsigned int irq);
+
#endif /* ASM__RISCV__IRQ_H */
/*
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index f2823267a9..ea317aea5a 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -50,6 +50,11 @@ static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
intc_hw_ops->set_irq_priority(desc, priority);
}
+void intc_handle_external_irqs(struct cpu_user_regs *regs)
+{
+ intc_hw_ops->handle_interrupt(regs);
+}
+
void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
{
ASSERT(desc->status & IRQ_DISABLED);
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 669ef3ae9e..466f1b4ba9 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -11,6 +11,10 @@
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/irq.h>
+#include <xen/spinlock.h>
+
+#include <asm/hardirq.h>
+#include <asm/intc.h>
static irq_desc_t irq_desc[NR_IRQS];
@@ -90,3 +94,46 @@ void __init init_IRQ(void)
if ( init_irq_data() < 0 )
panic("initialization of IRQ data failed\n");
}
+
+/* Dispatch an interrupt */
+void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+ struct irqaction *action;
+
+ irq_enter();
+
+ spin_lock(&desc->lock);
+
+ if ( desc->handler->ack )
+ desc->handler->ack(desc);
+
+ if ( desc->status & IRQ_DISABLED )
+ goto out;
+
+ desc->status |= IRQ_INPROGRESS;
+
+ action = desc->action;
+
+ spin_unlock_irq(&desc->lock);
+
+#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+ action->handler(irq, action->dev_id);
+#else
+ do {
+ action->handler(irq, action->dev_id);
+ action = action->next;
+ } while ( action );
+#endif /* CONFIG_IRQ_HAS_MULTIPLE_ACTION */
+
+ spin_lock_irq(&desc->lock);
+
+ desc->status &= ~IRQ_INPROGRESS;
+
+ out:
+ if ( desc->handler->end )
+ desc->handler->end(desc);
+
+ spin_unlock(&desc->lock);
+ irq_exit();
+}
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ea3638a54f..f061004d83 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -11,6 +11,7 @@
#include <xen/nospec.h>
#include <xen/sched.h>
+#include <asm/intc.h>
#include <asm/processor.h>
#include <asm/riscv_encoding.h>
#include <asm/traps.h>
@@ -128,6 +129,24 @@ void do_trap(struct cpu_user_regs *cpu_regs)
}
fallthrough;
default:
+ if ( cause & CAUSE_IRQ_FLAG )
+ {
+ /* Handle interrupt */
+ unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
+
+ switch ( icause )
+ {
+ case IRQ_S_EXT:
+ intc_handle_external_irqs(cpu_regs);
+ break;
+
+ default:
+ break;
+ }
+
+ break;
+ }
+
do_unexpected_trap(cpu_regs);
break;
}
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v6 6/7] xen/riscv: implement setup_irq()
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
` (4 preceding siblings ...)
2025-07-07 9:01 ` [PATCH v6 5/7] xen/riscv: add external interrupt handling for hypervisor mode Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
2025-07-07 9:01 ` [PATCH v6 7/7] xen/riscv: add basic UART support Oleksii Kurochko
6 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Romain Caritey
Introduce support for IRQ setup on RISC-V by implementing setup_irq() and
__setup_irq(), adapted and extended from an initial implementation by [1].
__setup_irq() does the following:
- Sets up an IRQ action.
- Validates that shared IRQs have non-NULL `dev_id` and are only used when
existing handlers allow sharing.
- Uses smp_wmb() to enforce memory ordering after assigning desc->action
to ensure visibility before enabling the IRQ.
- Supports multi-action setups via CONFIG_IRQ_HAS_MULTIPLE_ACTION.
setup_irq() does the following:
- Converts IRQ number to descriptor and acquires its lock.
- Rejects registration if the IRQ is already assigned to a guest domain,
printing an error.
- Delegates the core setup to __setup_irq().
- On first-time setup, disables the IRQ, routes it to Xen using
intc_route_irq_to_xen(), sets default CPU affinity (current CPU),
calls the handler’s startup routine, and finally enables the IRQ.
irq_set_affinity() invokes set_affinity() callback from the IRQ handler
if present.
Defined IRQ_NO_PRIORITY as default priority used when routing IRQs to Xen.
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7390e2365828b83e27ead56b03114a56e3699dd5
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
- Nothing changed. Only rebase.
---
Changes in V5:
- Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in V3-4:
- Nothing changed. Only rebase.
---
Changes in V2:
- Added implenmtation of aplic_set_irq_type() as it is going to be used in
this commit. And also, update the implementation of it. Make default case
of switch to do panic().
- Move all forward declaration up in asm/irq.h.
- s/__setup_irq/_setup_irq.
- Code style fixes.
- Update commit message.
- use smp_wmb() instead of smp_mb() in _setup_irq().
- Drop irq_set_affinity().
- Use plain C operator instead if {clear,set}_bit() for desc->status as it
is always used under spinlock().
- Drop set_bit(_IRQ_DISABLED, &desc->status) in setup_irq() as in the case
when IRQ is setuped for a first time, desc->status should be already set
to IRQ_DISABLED in init_one_irq_desc().
----
xen/arch/riscv/include/asm/irq.h | 2 +
xen/arch/riscv/irq.c | 84 ++++++++++++++++++++++++++++++++
2 files changed, 86 insertions(+)
diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index 94151eb083..f633636dc3 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -17,6 +17,8 @@
*/
#define NR_IRQS 1024
+#define IRQ_NO_PRIORITY 0
+
/* TODO */
#define nr_irqs 0U
#define nr_static_irqs 0
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 466f1b4ba9..25d3295002 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -7,6 +7,7 @@
*/
#include <xen/bug.h>
+#include <xen/cpumask.h>
#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
@@ -63,6 +64,89 @@ int platform_get_irq(const struct dt_device_node *device, int index)
return dt_irq.irq;
}
+static int _setup_irq(struct irq_desc *desc, unsigned int irqflags,
+ struct irqaction *new)
+{
+ bool shared = irqflags & IRQF_SHARED;
+
+ ASSERT(new != NULL);
+
+ /*
+ * Sanity checks:
+ * - if the IRQ is marked as shared
+ * - dev_id is not NULL when IRQF_SHARED is set
+ */
+ if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
+ return -EINVAL;
+ if ( shared && new->dev_id == NULL )
+ return -EINVAL;
+
+ if ( shared )
+ desc->status |= IRQF_SHARED;
+
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+ new->next = desc->action;
+#endif
+
+ desc->action = new;
+ smp_wmb();
+
+ return 0;
+}
+
+int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
+{
+ int rc;
+ unsigned long flags;
+ struct irq_desc *desc = irq_to_desc(irq);
+ bool disabled;
+
+ spin_lock_irqsave(&desc->lock, flags);
+
+ disabled = (desc->action == NULL);
+
+ if ( desc->status & IRQ_GUEST )
+ {
+ spin_unlock_irqrestore(&desc->lock, flags);
+ /*
+ * TODO: would be nice to have functionality to print which domain owns
+ * an IRQ.
+ */
+ printk(XENLOG_ERR "ERROR: IRQ %u is already in use by a domain\n", irq);
+ return -EBUSY;
+ }
+
+ rc = _setup_irq(desc, irqflags, new);
+ if ( rc )
+ goto err;
+
+ /* First time the IRQ is setup */
+ if ( disabled )
+ {
+ /* Route interrupt to xen */
+ intc_route_irq_to_xen(desc, IRQ_NO_PRIORITY);
+
+ /*
+ * We don't care for now which CPU will receive the
+ * interrupt.
+ *
+ * TODO: Handle case where IRQ is setup on different CPU than
+ * the targeted CPU and the priority.
+ */
+ desc->handler->set_affinity(desc, cpumask_of(smp_processor_id()));
+
+ desc->handler->startup(desc);
+
+ /* Enable irq */
+ desc->status &= ~IRQ_DISABLED;
+ }
+
+ err:
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ return rc;
+}
+
int arch_init_one_irq_desc(struct irq_desc *desc)
{
desc->arch.type = IRQ_TYPE_INVALID;
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v6 7/7] xen/riscv: add basic UART support
2025-07-07 9:01 [PATCH v6 0/7] riscv: introduce basic UART support and interrupts for hypervisor mode Oleksii Kurochko
` (5 preceding siblings ...)
2025-07-07 9:01 ` [PATCH v6 6/7] xen/riscv: implement setup_irq() Oleksii Kurochko
@ 2025-07-07 9:01 ` Oleksii Kurochko
6 siblings, 0 replies; 14+ messages in thread
From: Oleksii Kurochko @ 2025-07-07 9:01 UTC (permalink / raw)
To: xen-devel
Cc: Oleksii Kurochko, Alistair Francis, Bob Eshleman, Connor Davis,
Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini
Update Kconfig to select GENERIC_UART_INIT for basic UART init ( find a dt node
and call device specific device_init() ).
Drop `default n if RISCV` statement for config HAS_NS16550 as now ns16550 is
ready to be compiled and used by RISC-V. Also, make the config user selectable
for everyone except X86.
Initialize a minimal amount of stuff to have UART and Xen console:
- Initialize uart by calling uart_init().
- Initialize Xen console by calling console_init_{pre,post}irq().
- Initialize timer and its internal lists which are used by
init_timer() which is called by ns16550_init_postirq(); otherwise
"Unhandled exception: Store/AMO Page Fault" occurs.
- Enable local interrupt to recieve an input from UART
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V4-V6:
- Nothing changed. Only rebase.
---
Changes in v3:
- Drop inclusion of <xen/percpu.h> as nothing in setup.c requires it.
- Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v2:
- Drop #include <xen/keyhandler.h> in setup.c, isn't needed anymore.
- Drop call of percpu_init_areas() as it was needed when I used polling
mode for UART, for this case percpu is used to receive serial port info:
struct serial_port *port = this_cpu(poll_port);
So percpu isn't really needed at the current development state.
- Make HAS_NS16550 user selectable for everyone, except X86.
- Update the commit message.
---
xen/arch/riscv/Kconfig | 1 +
xen/arch/riscv/setup.c | 12 ++++++++++++
xen/drivers/char/Kconfig | 3 +--
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 62c5b7ba34..96bef90751 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -2,6 +2,7 @@ config RISCV
def_bool y
select FUNCTION_ALIGNMENT_16B
select GENERIC_BUG_FRAME
+ select GENERIC_UART_INIT
select HAS_DEVICE_TREE
select HAS_PMAP
select HAS_UBSAN
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 0e7398159c..a17096bf02 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -4,12 +4,15 @@
#include <xen/bug.h>
#include <xen/bootfdt.h>
#include <xen/compile.h>
+#include <xen/console.h>
#include <xen/device_tree.h>
#include <xen/init.h>
#include <xen/irq.h>
#include <xen/mm.h>
+#include <xen/serial.h>
#include <xen/shutdown.h>
#include <xen/smp.h>
+#include <xen/timer.h>
#include <xen/vmap.h>
#include <xen/xvmalloc.h>
@@ -134,8 +137,17 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
intc_preinit();
+ uart_init();
+ console_init_preirq();
+
intc_init();
+ timer_init();
+
+ local_irq_enable();
+
+ console_init_postirq();
+
printk("All set up\n");
machine_halt();
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e6e12bb413..8e49a52c73 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -2,8 +2,7 @@ config GENERIC_UART_INIT
bool
config HAS_NS16550
- bool "NS16550 UART driver" if ARM
- default n if RISCV
+ bool "NS16550 UART driver" if !X86
default y
help
This selects the 16550-series UART support. For most systems, say Y.
--
2.50.0
^ permalink raw reply related [flat|nested] 14+ messages in thread