[PATCH v2 00/16] riscv: introduce basic UART support and interrupts for hypervisor mode

[PATCH v2 00/16] riscv: introduce basic UART support and interrupts for hypervisor mode

[Oleksii Kurochko]

The patch series introduces basic UART support (in interrupt mode) and support of
interrupts for hypervisor mode.

To implement this the following has been added:
- APLIC and IMISC initialization.
- Introduce of intc_hw_operations abstraction.
- Introduce some APLIC and IMSIC operations.
- Introduce init_IRQ(), platform_get_irq() and setup_irq() functions.
- Update do_trap() handler to handle IRQ_S_EXT.
- Introduce some other functions such as: get_s_time(), smp_clear_cpu_maps(),
  ioremap().
- Enable UART. 

CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/1802596163

---
Changes in v2:
- Split patch [PATCH v1 07/14] xen/riscv: Introduce intc_hw_operations abstraction
  into two:
  - xen/riscv: introduce register_intc_ops() and intc_hw_ops
  - xen/riscv: introduce intc_init() and helpers
  It was needed to be able to merge [PATCH v1 13/14] xen/riscv: initialize interrupt controller
  into the patch where intc_init() is introduced.
- Merge  [PATCH v1 13/14] xen/riscv: initialize interrupt controller to
  xen/riscv: introduce intc_init() and helpers.
- xen/riscv: implement get_s_time() has been merged to staging.
- All other changes please look in specific patch.
---

Oleksii Kurochko (16):
  xen/riscv: initialize bitmap to zero in
    riscv_fill_hwcap_from_isa_string()
  xen/riscv: introduce smp_prepare_boot_cpu()
  xen/riscv: introduce support of Svpbmt extension
  xen/riscv: add ioremap_*() variants using ioremap_attr()
  xen/asm-generic: introduce asm-generic/irq-dt.h
  xen/riscv: introduce init_IRQ()
  xen/riscv: introduce platform_get_irq()
  xen/riscv: dt_processor_cpuid() implementation
  xen/riscv: introduce register_intc_ops() and intc_hw_ops.
  xen/riscv: imsic_init() implementation
  xen/riscv: aplic_init() implementation
  xen/riscv: introduce intc_init() and helpers
  xen/riscv: implementation of aplic and imsic operations
  xen/riscv: add external interrupt handling for hypervisor mode
  xen/riscv: implement setup_irq()
  xen/riscv: add basic UART support

 automation/scripts/qemu-smoke-riscv64.sh |   1 +
 docs/misc/riscv/booting.txt              |   4 +
 xen/arch/arm/include/asm/Makefile        |   1 +
 xen/arch/arm/include/asm/irq.h           |  15 +-
 xen/arch/riscv/Kconfig                   |  15 +
 xen/arch/riscv/Makefile                  |   3 +
 xen/arch/riscv/aplic-priv.h              |  38 +++
 xen/arch/riscv/aplic.c                   | 309 ++++++++++++++++++
 xen/arch/riscv/cpufeature.c              |   4 +
 xen/arch/riscv/imsic.c                   | 390 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/Makefile      |   1 +
 xen/arch/riscv/include/asm/aplic.h       |  73 +++++
 xen/arch/riscv/include/asm/cpufeature.h  |   1 +
 xen/arch/riscv/include/asm/fixmap.h      |   2 +-
 xen/arch/riscv/include/asm/imsic.h       |  84 +++++
 xen/arch/riscv/include/asm/intc.h        |  34 ++
 xen/arch/riscv/include/asm/io.h          |  11 +-
 xen/arch/riscv/include/asm/irq.h         |  16 +
 xen/arch/riscv/include/asm/mm-types.h    |   8 +
 xen/arch/riscv/include/asm/page.h        |  17 +-
 xen/arch/riscv/include/asm/smp.h         |   3 +
 xen/arch/riscv/intc.c                    |  59 ++++
 xen/arch/riscv/irq.c                     | 218 +++++++++++++
 xen/arch/riscv/mm.c                      |  34 ++
 xen/arch/riscv/pt.c                      |  20 +-
 xen/arch/riscv/setup.c                   |  22 +-
 xen/arch/riscv/smpboot.c                 |  78 +++++
 xen/arch/riscv/stubs.c                   |  11 -
 xen/arch/riscv/traps.c                   |  19 ++
 xen/arch/riscv/xen.lds.S                 |   2 +
 xen/drivers/char/Kconfig                 |   3 +-
 xen/include/asm-generic/irq-dt.h         |  21 ++
 32 files changed, 1470 insertions(+), 47 deletions(-)
 create mode 100644 xen/arch/riscv/aplic-priv.h
 create mode 100644 xen/arch/riscv/imsic.c
 create mode 100644 xen/arch/riscv/include/asm/aplic.h
 create mode 100644 xen/arch/riscv/include/asm/imsic.h
 create mode 100644 xen/arch/riscv/include/asm/mm-types.h
 create mode 100644 xen/arch/riscv/irq.c
 create mode 100644 xen/arch/riscv/smpboot.c
 create mode 100644 xen/include/asm-generic/irq-dt.h

-- 
2.49.0

[PATCH v2 01/16] xen/riscv: initialize bitmap to zero in riscv_fill_hwcap_from_isa_string()

[Oleksii Kurochko]

The this_isa bitmap should be explicitly initialized to zero to avoid
false positives when detecting supported ISA extensions. Without proper
zero-initialization, the bitmap may retain non-zero values from
uninitialized memory, causing Xen to incorrectly assume that certain
extensions are supported.

This change ensures reliable detection of ISA capabilities.

Fixes: 0c2f717eae ("xen/riscv: identify specific ISA supported by cpu")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - new patch.
---
 xen/arch/riscv/cpufeature.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 5aafab0f49..3246a03624 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -405,6 +405,8 @@ static void __init riscv_fill_hwcap_from_isa_string(void)
         const char *isa;
         unsigned long cpuid;
 
+        bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
+
         if ( !dt_device_type_is_equal(cpu, "cpu") )
             continue;
 
-- 
2.49.0

[PATCH v2 02/16] xen/riscv: introduce smp_prepare_boot_cpu()

[Oleksii Kurochko]

Initialize cpu_{possible, online}_map by using smp_prepare_boot_cpu().

Drop DEFINE_PER_CPU(unsigned int, cpu_id) from stubs.c as this variable isn't
expected to be used in RISC-V at all.

Move declaration of cpu_{possible,online}_map from stubs.c to smpboot.c
as now smpboot.c is now introduced.
Other defintions keep in stubs.c as they are not initialized and not needed, at
the moment.

Drop cpu_present_map as it is enough to have cpu_possible_map. Also, ask
linker to provide symbol for cpu_present_map as common code references it.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Add __read_mostly for cpu_online_map.
 - Add __ro_after_init for cpu_possible_map.
 - Drop cpu_present_map and cpumask_copy(&cpu_present_map, &cpu_possible_map);
 - Drop cpumask_clear() for cpu_{possible,online}_map.
 - Ask the linker provide the symbol for cpu_present_map as common code uses
   it.
 - s/smp_clear_cpu_maps/smp_prepare_boot_cpu.
 - Include <xen/smp.h> in setup.c as smp_prepare_boot_cpu() is declare in that
   header now.
   Also, drop inclusion of asm/smp.h in setup.c asm xen/smp.h has inclusion of
   asm/smp.h.
 - Update the commit message.
---
 xen/arch/riscv/Makefile  |  1 +
 xen/arch/riscv/setup.c   |  4 +++-
 xen/arch/riscv/smpboot.c | 12 ++++++++++++
 xen/arch/riscv/stubs.c   |  6 ------
 xen/arch/riscv/xen.lds.S |  2 ++
 5 files changed, 18 insertions(+), 7 deletions(-)
 create mode 100644 xen/arch/riscv/smpboot.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index d882c57528..f42cf3dfa6 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -10,6 +10,7 @@ obj-y += sbi.o
 obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smp.o
+obj-y += smpboot.o
 obj-y += stubs.o
 obj-y += time.o
 obj-y += traps.o
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 4e416f6e44..2a564512d7 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -8,6 +8,7 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/shutdown.h>
+#include <xen/smp.h>
 #include <xen/vmap.h>
 #include <xen/xvmalloc.h>
 
@@ -19,7 +20,6 @@
 #include <asm/intc.h>
 #include <asm/sbi.h>
 #include <asm/setup.h>
-#include <asm/smp.h>
 #include <asm/traps.h>
 
 /* Xen stack for bringing up the first CPU. */
@@ -72,6 +72,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     remove_identity_mapping();
 
+    smp_prepare_boot_cpu();
+
     set_processor_id(0);
 
     set_cpuid_to_hartid(0, bootcpu_id);
diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
new file mode 100644
index 0000000000..0371dfa53e
--- /dev/null
+++ b/xen/arch/riscv/smpboot.c
@@ -0,0 +1,12 @@
+#include <xen/cpumask.h>
+#include <xen/init.h>
+#include <xen/sections.h>
+
+cpumask_t __read_mostly cpu_online_map;
+cpumask_t __ro_after_init cpu_possible_map;
+
+void __init smp_prepare_boot_cpu(void)
+{
+    cpumask_set_cpu(0, &cpu_possible_map);
+    cpumask_set_cpu(0, &cpu_online_map);
+}
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 83416d3350..fdcf91054e 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -11,12 +11,6 @@
 
 /* smpboot.c */
 
-cpumask_t cpu_online_map;
-cpumask_t cpu_present_map;
-cpumask_t cpu_possible_map;
-
-/* ID of the PCPU we're running on */
-DEFINE_PER_CPU(unsigned int, cpu_id);
 /* XXX these seem awfully x86ish... */
 /* representing HT siblings of each logical CPU */
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 818aa43669..8c3c06de01 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -165,6 +165,8 @@ SECTIONS
     ELF_DETAILS_SECTIONS
 }
 
+PROVIDE(cpu_present_map = cpu_possible_map);
+
 ASSERT(IS_ALIGNED(__bss_start,      POINTER_ALIGN), "__bss_start is misaligned")
 ASSERT(IS_ALIGNED(__bss_end,        POINTER_ALIGN), "__bss_end is misaligned")
 
-- 
2.49.0

[PATCH v2 03/16] xen/riscv: introduce support of Svpbmt extension

[Oleksii Kurochko]

Svpbmt extension is necessary for chaning the memory type for a page contains
a combination of attributes that indicate the cacheability, idempotency,
and ordering properties for access to that page.

As a part of the patch the following is introduced:
- Svpbmt memory type defintions: PTE_PBMT_{NOCACHE,IO}.
- PAGE_HYPERVISOR_{NOCACHE,WC}.
- RISCV_ISA_EXT_svpbmt and add a check in runtime that Svpbmt is
  supported by platform.
- Update riscv/booting.txt with information about Svpbmt.
- Update logic of pt_update_entry() to take into account PBMT bits.

Use 'unsigned long' for pte_attr_t as PMBT bits are 61 and 62 and it doesn't
fit into 'unsigned int'. Also, update function prototypes which uses
'unsigned int' for flags/attibutes.

Enable Svpbmt for testing in QEMU as Svpmbt is now mandatory for
Xen work.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - new patch.
---
 automation/scripts/qemu-smoke-riscv64.sh |  1 +
 docs/misc/riscv/booting.txt              |  4 ++++
 xen/arch/riscv/Kconfig                   | 14 ++++++++++++++
 xen/arch/riscv/cpufeature.c              |  2 ++
 xen/arch/riscv/include/asm/cpufeature.h  |  1 +
 xen/arch/riscv/include/asm/fixmap.h      |  2 +-
 xen/arch/riscv/include/asm/mm-types.h    |  8 ++++++++
 xen/arch/riscv/include/asm/page.h        | 17 ++++++++++++++++-
 xen/arch/riscv/pt.c                      | 20 +++++++++++---------
 9 files changed, 58 insertions(+), 11 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/mm-types.h

diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index b2e112c942..25f9e4190e 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -7,6 +7,7 @@ rm -f smoke.serial
 
 export TEST_CMD="qemu-system-riscv64 \
     -M virt,aia=aplic-imsic \
+    -cpu rv64,svpbmt=on \
     -smp 1 \
     -nographic \
     -m 2g \
diff --git a/docs/misc/riscv/booting.txt b/docs/misc/riscv/booting.txt
index 3a8474a27d..e100bde575 100644
--- a/docs/misc/riscv/booting.txt
+++ b/docs/misc/riscv/booting.txt
@@ -18,3 +18,7 @@ Xen is run:
 - Zihintpause:
   On a system that doesn't have this extension, cpu_relax() should be
   implemented properly.
+- SVPBMT is mandatory to enable changing the memory attributes of a page.
+  For platforms that do not support SVPBMT, it is necessary to introduce a
+  similar mechanism as described in:
+  https://lore.kernel.org/all/20241102000843.1301099-1-samuel.holland@sifive.com/
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index d882e0a059..60520dab57 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -10,11 +10,25 @@ config RISCV
 config RISCV_64
 	def_bool y
 	select 64BIT
+	select HAS_SVPBMT
 
 config ARCH_DEFCONFIG
 	string
 	default "arch/riscv/configs/tiny64_defconfig"
 
+config HAS_SVPBMT
+	bool
+	depends on RISCV_64
+	help
+	  This config enables usage of Svpbmt ISA-extension ( Supervisor-mode:
+	  page-based memory types).
+
+	  The memory type for a page contains a combination of attributes
+	  that indicate the cacheability, idempotency, and ordering
+	  properties for access to that page.
+
+	  The Svpbmt extension is only available on 64-bit cpus.
+
 menu "Architecture Features"
 
 source "arch/Kconfig"
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 3246a03624..b7d5ec6580 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -137,6 +137,7 @@ const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {
     RISCV_ISA_EXT_DATA(zbs),
     RISCV_ISA_EXT_DATA(smaia),
     RISCV_ISA_EXT_DATA(ssaia),
+    RISCV_ISA_EXT_DATA(svpbmt),
 };
 
 static const struct riscv_isa_ext_data __initconst required_extensions[] = {
@@ -151,6 +152,7 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
     RISCV_ISA_EXT_DATA(zifencei),
     RISCV_ISA_EXT_DATA(zihintpause),
     RISCV_ISA_EXT_DATA(zbb),
+    RISCV_ISA_EXT_DATA(svpbmt),
 };
 
 static bool __init is_lowercase_extension_name(const char *str)
diff --git a/xen/arch/riscv/include/asm/cpufeature.h b/xen/arch/riscv/include/asm/cpufeature.h
index 1015b6ee44..768b84b769 100644
--- a/xen/arch/riscv/include/asm/cpufeature.h
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -37,6 +37,7 @@ enum riscv_isa_ext_id {
     RISCV_ISA_EXT_zbs,
     RISCV_ISA_EXT_smaia,
     RISCV_ISA_EXT_ssaia,
+    RISCV_ISA_EXT_svpbmt,
     RISCV_ISA_EXT_MAX
 };
 
diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
index e399a15f53..5990c964aa 100644
--- a/xen/arch/riscv/include/asm/fixmap.h
+++ b/xen/arch/riscv/include/asm/fixmap.h
@@ -33,7 +33,7 @@
 extern pte_t xen_fixmap[];
 
 /* Map a page in a fixmap entry */
-void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags);
+void set_fixmap(unsigned int map, mfn_t mfn, pte_attr_t flags);
 /* Remove a mapping from a fixmap entry */
 void clear_fixmap(unsigned int map);
 
diff --git a/xen/arch/riscv/include/asm/mm-types.h b/xen/arch/riscv/include/asm/mm-types.h
new file mode 100644
index 0000000000..fa512064b8
--- /dev/null
+++ b/xen/arch/riscv/include/asm/mm-types.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef ASM_RISCV_MM_TYPES_H
+#define ASM_RISCV_MM_TYPES_H
+
+typedef unsigned long pte_attr_t;
+
+#endif /* ASM_RISCV_MM_TYPES_H */
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index bf8988f657..2af4823170 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -46,6 +46,8 @@
 #define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
 
 #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
+#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PMBT_IO)
+#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PMBT_NOCACHE)
 
 /*
  * The PTE format does not contain the following bits within itself;
@@ -56,8 +58,21 @@
 #define PTE_SMALL       BIT(10, UL)
 #define PTE_POPULATE    BIT(11, UL)
 
+/*
+ * [62:61] Svpbmt Memory Type definitions:
+ *
+ *  00 - PMA    Normal Cacheable, No change to implied PMA memory type
+ *  01 - NC     Non-cacheable, idempotent, weakly-ordered Main Memory
+ *  10 - IO     Non-cacheable, non-idempotent, strongly-ordered I/O memory
+ *  11 - Rsvd   Reserved for future standard use
+ */
+#define PTE_PMBT_NOCACHE    BIT(61, UL)
+#define PTE_PMBT_IO         BIT(62, UL)
+
 #define PTE_ACCESS_MASK (PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
 
+#define PTE_PBMT_MASK   (PTE_PMBT_NOCACHE | PTE_PMBT_IO)
+
 /* Calculate the offsets into the pagetables for a given VA */
 #define pt_linear_offset(lvl, va)   ((va) >> XEN_PT_LEVEL_SHIFT(lvl))
 
@@ -202,7 +217,7 @@ static inline pte_t read_pte(const pte_t *p)
     return read_atomic(p);
 }
 
-static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags)
+static inline pte_t pte_from_mfn(mfn_t mfn, pte_attr_t flags)
 {
     unsigned long pte = (mfn_x(mfn) << PTE_PPN_SHIFT) | flags;
     return (pte_t){ .pte = pte };
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index 918b1b91ab..82c8c73c3e 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -25,7 +25,7 @@ static inline mfn_t get_root_page(void)
  * See the comment about the possible combination of (mfn, flags) in
  * the comment above pt_update().
  */
-static bool pt_check_entry(pte_t entry, mfn_t mfn, unsigned int flags)
+static bool pt_check_entry(pte_t entry, mfn_t mfn, pte_attr_t flags)
 {
     /* Sanity check when modifying an entry. */
     if ( (flags & PTE_VALID) && mfn_eq(mfn, INVALID_MFN) )
@@ -260,7 +260,7 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
  */
 static int pt_update_entry(mfn_t root, vaddr_t virt,
                            mfn_t mfn, unsigned int *target,
-                           unsigned int flags)
+                           pte_attr_t flags)
 {
     int rc;
     /*
@@ -328,17 +328,19 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
         pte.pte = 0;
     else
     {
+        const pte_attr_t attrs = PTE_ACCESS_MASK | PTE_PBMT_MASK;
+
         /* We are inserting a mapping => Create new pte. */
         if ( !mfn_eq(mfn, INVALID_MFN) )
             pte = pte_from_mfn(mfn, PTE_VALID);
-        else /* We are updating the permission => Copy the current pte. */
+        else /* We are updating the attributes => Copy the current pte. */
         {
             pte = *ptep;
-            pte.pte &= ~PTE_ACCESS_MASK;
+            pte.pte &= ~attrs;
         }
 
-        /* update permission according to the flags */
-        pte.pte |= (flags & PTE_ACCESS_MASK) | PTE_ACCESSED | PTE_DIRTY;
+        /* Update attributes of PTE according to the flags. */
+        pte.pte |= (flags & attrs) | PTE_ACCESSED | PTE_DIRTY;
     }
 
     write_pte(ptep, pte);
@@ -353,7 +355,7 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
 
 /* Return the level where mapping should be done */
 static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr,
-                            unsigned int flags)
+                            pte_attr_t flags)
 {
     unsigned int level = 0;
     unsigned long mask;
@@ -407,7 +409,7 @@ static DEFINE_SPINLOCK(pt_lock);
  * inserting will be done.
  */
 static int pt_update(vaddr_t virt, mfn_t mfn,
-                     unsigned long nr_mfns, unsigned int flags)
+                     unsigned long nr_mfns, pte_attr_t flags)
 {
     int rc = 0;
     unsigned long vfn = PFN_DOWN(virt);
@@ -535,7 +537,7 @@ int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 }
 
 /* Map a 4k page in a fixmap entry */
-void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags)
+void set_fixmap(unsigned int map, mfn_t mfn, pte_attr_t flags)
 {
     if ( map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL) != 0 )
         BUG();
-- 
2.49.0

[PATCH v2 04/16] xen/riscv: add ioremap_*() variants using ioremap_attr()

[Oleksii Kurochko]

Introduce ioremap_attr() as a shared helper to implement architecture-specific
ioremap variants:
- ioremap_nocache()
- ioremap_cache()
- ioremap_wc()

These functions use __vmap() internally and apply appropriate memory attributes
for RISC-V.

These functions are implemned not as static inline function or macros as it will
require to include asm/page.h into asm/io.h what will lead to compilation
issue.

Also, remove the unused ioremap_wt() macro from asm/io.h, as write-through
mappings are not expected to be used on RISC-V.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Update the commit subject + message.  
 - move out Svpbmt changes to separate patch.
 - Drop #ifdef SVPBMT for ioremap().
 - Redefine ioremap_* in io.h.
 - Introduce ioremap_attr().
---
 xen/arch/riscv/include/asm/io.h | 11 +++--------
 xen/arch/riscv/mm.c             | 34 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/xen/arch/riscv/include/asm/io.h b/xen/arch/riscv/include/asm/io.h
index 8bab4ffa03..921e334ce1 100644
--- a/xen/arch/riscv/include/asm/io.h
+++ b/xen/arch/riscv/include/asm/io.h
@@ -41,14 +41,9 @@
 #include <xen/macros.h>
 #include <xen/types.h>
 
-/*
- * The RISC-V ISA doesn't yet specify how to query or modify PMAs, so we can't
- * change the properties of memory regions.  This should be fixed by the
- * upcoming platform spec.
- */
-#define ioremap_nocache(addr, size) ioremap(addr, size)
-#define ioremap_wc(addr, size) ioremap(addr, size)
-#define ioremap_wt(addr, size) ioremap(addr, size)
+void __iomem *ioremap_nocache(paddr_t start, size_t len);
+void __iomem *ioremap_cache(paddr_t start, size_t len);
+void __iomem *ioremap_wc(paddr_t start, size_t len);
 
 /* Generic IO read/write.  These perform native-endian accesses. */
 static inline void __raw_writeb(uint8_t val, volatile void __iomem *addr)
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index d3ece9f132..c8526dd2ef 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -11,6 +11,7 @@
 #include <xen/pfn.h>
 #include <xen/sections.h>
 #include <xen/sizes.h>
+#include <xen/vmap.h>
 
 #include <asm/early_printk.h>
 #include <asm/csr.h>
@@ -583,3 +584,36 @@ void *__init arch_vmap_virt_end(void)
 {
     return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
 }
+
+static void *ioremap_attr(paddr_t start, size_t len, pte_attr_t attributes)
+{
+    mfn_t mfn = _mfn(PFN_DOWN(start));
+    unsigned int offs = start & (PAGE_SIZE - 1);
+    unsigned int nr = PFN_UP(offs + len);
+    void *ptr = __vmap(&mfn, nr, 1, 1, attributes, VMAP_DEFAULT);
+
+    if ( ptr == NULL )
+        return NULL;
+
+    return ptr + offs;
+}
+
+void __iomem *ioremap_nocache(paddr_t start, size_t len)
+{
+    return ioremap_attr(start, len, PAGE_HYPERVISOR_NOCACHE);
+}
+
+void __iomem *ioremap_cache(paddr_t start, size_t len)
+{
+    return ioremap_attr(start, len, PAGE_HYPERVISOR);
+}
+
+void __iomem *ioremap_wc(paddr_t start, size_t len)
+{
+    return ioremap_attr(start, len, PAGE_HYPERVISOR_WC);
+}
+
+void *ioremap(paddr_t pa, size_t len)
+{
+    return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
+}
-- 
2.49.0

[PATCH v2 05/16] xen/asm-generic: introduce asm-generic/irq-dt.h

[Oleksii Kurochko]

Introduce defintions of IRQ_TYPE_* which correspond to the Xen internal
representation of the IRQ types and make them the same asthe existing
device tree defintions for convenience.

These defines are going to be re-used, at least, by RISC-V architecture.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - new patch.
---
 xen/arch/arm/include/asm/Makefile |  1 +
 xen/arch/arm/include/asm/irq.h    | 15 +--------------
 xen/include/asm-generic/irq-dt.h  | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 14 deletions(-)
 create mode 100644 xen/include/asm-generic/irq-dt.h

diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile
index 831c914cce..87c8821421 100644
--- a/xen/arch/arm/include/asm/Makefile
+++ b/xen/arch/arm/include/asm/Makefile
@@ -4,6 +4,7 @@ generic-y += device.h
 generic-y += dom0less-build.h
 generic-y += hardirq.h
 generic-y += iocap.h
+generic-y += irq-dt.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += random.h
diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index 88e060bf29..fce7e42a33 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -4,20 +4,7 @@
 #include <xen/device_tree.h>
 #include <public/device_tree_defs.h>
 
-/*
- * These defines correspond to the Xen internal representation of the
- * IRQ types. We choose to make them the same as the existing device
- * tree definitions for convenience.
- */
-#define IRQ_TYPE_NONE           DT_IRQ_TYPE_NONE
-#define IRQ_TYPE_EDGE_RISING    DT_IRQ_TYPE_EDGE_RISING
-#define IRQ_TYPE_EDGE_FALLING   DT_IRQ_TYPE_EDGE_FALLING
-#define IRQ_TYPE_EDGE_BOTH      DT_IRQ_TYPE_EDGE_BOTH 
-#define IRQ_TYPE_LEVEL_HIGH     DT_IRQ_TYPE_LEVEL_HIGH
-#define IRQ_TYPE_LEVEL_LOW      DT_IRQ_TYPE_LEVEL_LOW
-#define IRQ_TYPE_LEVEL_MASK     DT_IRQ_TYPE_LEVEL_MASK
-#define IRQ_TYPE_SENSE_MASK     DT_IRQ_TYPE_SENSE_MASK
-#define IRQ_TYPE_INVALID        DT_IRQ_TYPE_INVALID
+#include <asm/irq-dt.h>
 
 #define NR_VECTORS 256 /* XXX */
 
diff --git a/xen/include/asm-generic/irq-dt.h b/xen/include/asm-generic/irq-dt.h
new file mode 100644
index 0000000000..5ec46da533
--- /dev/null
+++ b/xen/include/asm-generic/irq-dt.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_GENERIC_IRQ_DT_H__
+#define __ASM_GENERIC_IRQ_DT_H__
+
+/*
+ * These defines correspond to the Xen internal representation of the
+ * IRQ types. We choose to make them the same as the existing device
+ * tree definitions for convenience.
+ */
+#define IRQ_TYPE_NONE           DT_IRQ_TYPE_NONE
+#define IRQ_TYPE_EDGE_RISING    DT_IRQ_TYPE_EDGE_RISING
+#define IRQ_TYPE_EDGE_FALLING   DT_IRQ_TYPE_EDGE_FALLING
+#define IRQ_TYPE_EDGE_BOTH      DT_IRQ_TYPE_EDGE_BOTH
+#define IRQ_TYPE_LEVEL_HIGH     DT_IRQ_TYPE_LEVEL_HIGH
+#define IRQ_TYPE_LEVEL_LOW      DT_IRQ_TYPE_LEVEL_LOW
+#define IRQ_TYPE_LEVEL_MASK     DT_IRQ_TYPE_LEVEL_MASK
+#define IRQ_TYPE_SENSE_MASK     DT_IRQ_TYPE_SENSE_MASK
+#define IRQ_TYPE_INVALID        DT_IRQ_TYPE_INVALID
+
+#endif /* __ASM_GENERIC_IRQ_DT_H__ */
-- 
2.49.0

[PATCH v2 06/16] xen/riscv: introduce init_IRQ()

[Oleksii Kurochko]

Implement init_IRQ() to initalize various IRQs.

Currently, this function initializes the irq_desc[] array,
which stores IRQ descriptors containing various information
about each IRQ, such as the type of hardware handling, whether
the IRQ is disabled, etc.
The initialization is basic at this point and includes setting
IRQ_TYPE_INVALID as the IRQ type, assigning the IRQ number ( which
is just a consequent index of irq_desc[] array ) to
desc->irq, and setting desc->action to NULL.

Additionally, the function init_irq_data() is introduced to
initialize the IRQ descriptors for all IRQs in the system.

Reuse defines of IRQ_TYPE_* from asm-generic/irq-dt.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Move an introduction of IRQ_TYPE_* defines to the separate patch.
 - Reuse asm-generic/irq-dt.h.
 - Use 'unsigned int' for local irq variable inside init_irq_data().
---
 xen/arch/riscv/Makefile             |  1 +
 xen/arch/riscv/include/asm/Makefile |  1 +
 xen/arch/riscv/include/asm/irq.h    |  7 +++++
 xen/arch/riscv/irq.c                | 45 +++++++++++++++++++++++++++++
 xen/arch/riscv/setup.c              |  3 ++
 xen/arch/riscv/stubs.c              |  5 ----
 6 files changed, 57 insertions(+), 5 deletions(-)
 create mode 100644 xen/arch/riscv/irq.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index f42cf3dfa6..a1c145c506 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -3,6 +3,7 @@ obj-y += cpufeature.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += entry.o
 obj-y += intc.o
+obj-y += irq.o
 obj-y += mm.o
 obj-y += pt.o
 obj-$(CONFIG_RISCV_64) += riscv64/
diff --git a/xen/arch/riscv/include/asm/Makefile b/xen/arch/riscv/include/asm/Makefile
index c989a7f89b..bfdf186c68 100644
--- a/xen/arch/riscv/include/asm/Makefile
+++ b/xen/arch/riscv/include/asm/Makefile
@@ -5,6 +5,7 @@ generic-y += div64.h
 generic-y += hardirq.h
 generic-y += hypercall.h
 generic-y += iocap.h
+generic-y += irq-dt.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += perfc_defn.h
diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index 2a48da2651..f609df466e 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -3,6 +3,11 @@
 #define ASM__RISCV__IRQ_H
 
 #include <xen/bug.h>
+#include <xen/device_tree.h>
+
+#include <asm/irq-dt.h>
+
+#define NR_IRQS 1024
 
 /* TODO */
 #define nr_irqs 0U
@@ -25,6 +30,8 @@ static inline void arch_move_irqs(struct vcpu *v)
     BUG_ON("unimplemented");
 }
 
+void init_IRQ(void);
+
 #endif /* ASM__RISCV__IRQ_H */
 
 /*
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
new file mode 100644
index 0000000000..26a8556b2c
--- /dev/null
+++ b/xen/arch/riscv/irq.c
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * RISC-V Trap handlers
+ *
+ * Copyright (c) 2024 Vates
+ */
+
+#include <xen/bug.h>
+#include <xen/init.h>
+#include <xen/irq.h>
+
+static irq_desc_t irq_desc[NR_IRQS];
+
+int arch_init_one_irq_desc(struct irq_desc *desc)
+{
+    desc->arch.type = IRQ_TYPE_INVALID;
+
+    return 0;
+}
+
+static int __init init_irq_data(void)
+{
+    unsigned int irq;
+
+    for ( irq = 0; irq < NR_IRQS; irq++ )
+    {
+        struct irq_desc *desc = irq_to_desc(irq);
+        int rc = init_one_irq_desc(desc);
+
+        if ( rc )
+            return rc;
+
+        desc->irq = irq;
+        desc->action  = NULL;
+    }
+
+    return 0;
+}
+
+void __init init_IRQ(void)
+{
+    if ( init_irq_data() < 0 )
+        panic("initialization of IRQ data failed\n");
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 2a564512d7..82f8839eff 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -6,6 +6,7 @@
 #include <xen/compile.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
+#include <xen/irq.h>
 #include <xen/mm.h>
 #include <xen/shutdown.h>
 #include <xen/smp.h>
@@ -127,6 +128,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
         panic("Booting using ACPI isn't supported\n");
     }
 
+    init_IRQ();
+
     riscv_fill_hwcap();
 
     preinit_xen_time();
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index fdcf91054e..e396b67cd3 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -107,11 +107,6 @@ void irq_ack_none(struct irq_desc *desc)
     BUG_ON("unimplemented");
 }
 
-int arch_init_one_irq_desc(struct irq_desc *desc)
-{
-    BUG_ON("unimplemented");
-}
-
 void smp_send_state_dump(unsigned int cpu)
 {
     BUG_ON("unimplemented");
-- 
2.49.0

[PATCH v2 07/16] xen/riscv: introduce platform_get_irq()

[Oleksii Kurochko]

platform_get_irq() recieves information about device's irq ( type
and irq number ) from device tree node and using this information
update irq descriptor in irq_desc[] array.

Introduce dt_irq_xlate and initialize with aplic_irq_xlate() as
it is used by dt_device_get_irq() which is called by
platform_get_irq().

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - Add cf_check for aplic_irq_xlate().
 - Ident label in irq_set_type().
 - Return proper -E... values for platform_get_irq().
---
 xen/arch/riscv/aplic.c           | 20 +++++++++++++++
 xen/arch/riscv/include/asm/irq.h |  3 +++
 xen/arch/riscv/irq.c             | 42 ++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)

diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index caba8f8993..10ae81f7ac 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -11,6 +11,7 @@
 
 #include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/irq.h>
 #include <xen/sections.h>
 #include <xen/types.h>
 
@@ -21,6 +22,23 @@ static struct intc_info __ro_after_init aplic_info = {
     .hw_version = INTC_APLIC,
 };
 
+static int cf_check aplic_irq_xlate(const uint32_t *intspec,
+                                    unsigned int intsize,
+                                    unsigned int *out_hwirq,
+                                    unsigned int *out_type)
+{
+    if ( intsize < 2 )
+        return -EINVAL;
+
+    /* Mapping 1:1 */
+    *out_hwirq = intspec[0];
+
+    if ( out_type )
+        *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+
+    return 0;
+}
+
 static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
 {
     if ( aplic_info.node )
@@ -35,6 +53,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
 
     aplic_info.node = node;
 
+    dt_irq_xlate = aplic_irq_xlate;
+
     return 0;
 }
 
diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index f609df466e..6223bbbed5 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -30,6 +30,9 @@ 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);
 
 #endif /* ASM__RISCV__IRQ_H */
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 26a8556b2c..4c518bbd97 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -7,11 +7,53 @@
  */
 
 #include <xen/bug.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/irq.h>
 
 static irq_desc_t irq_desc[NR_IRQS];
 
+static bool irq_validate_new_type(unsigned int curr, unsigned int new)
+{
+    return (curr == IRQ_TYPE_INVALID || curr == new );
+}
+
+static int irq_set_type(unsigned int irq, unsigned int type)
+{
+    unsigned long flags;
+    struct irq_desc *desc = irq_to_desc(irq);
+    int ret = -EBUSY;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    if ( !irq_validate_new_type(desc->arch.type, type) )
+        goto err;
+
+    desc->arch.type = type;
+
+    ret = 0;
+
+ err:
+    spin_unlock_irqrestore(&desc->lock, flags);
+
+    return ret;
+}
+
+int platform_get_irq(const struct dt_device_node *device, int index)
+{
+    struct dt_irq dt_irq;
+    int ret;
+
+    if ( (ret = dt_device_get_irq(device, index, &dt_irq)) != 0 )
+        return ret;
+
+    if ( (ret = irq_set_type(dt_irq.irq, dt_irq.type)) != 0 )
+        return ret;
+
+    return dt_irq.irq;
+}
+
 int arch_init_one_irq_desc(struct irq_desc *desc)
 {
     desc->arch.type = IRQ_TYPE_INVALID;
-- 
2.49.0

[PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation

[Oleksii Kurochko]

Implements dt_processor_hartid() to get the hart ID of the given
device tree node and do some checks if CPU is available and given device
tree node has proper riscv,isa property.

As a helper function dt_get_cpuid() is introduced to deal specifically
with reg propery of a CPU device node.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - s/of_get_cpu_hwid()/dt_get_cpu_id().
 - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
 - Add empty line before last return in dt_get_cpu_hwid().
 - s/riscv_of_processor_hartid/dt_processor_cpuid().
 - Use pointer-to_const for node argument of dt_processor_cpuid().
 - Use for hart_id unsigned long type as according to the spec for RV128
   mhartid register will be 128 bit long.
 - Update commit message and subject.
 - use 'CPU' instead of 'HART'.
 - Drop thread argument of dt_get_cpu_id() (of_get_cpu_hwid) as it is
   expected to be always 0 according to RISC-V's DTS binding.
---
 xen/arch/riscv/include/asm/smp.h |  3 ++
 xen/arch/riscv/smpboot.c         | 66 ++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
index 5e170b57b3..9d846a1338 100644
--- a/xen/arch/riscv/include/asm/smp.h
+++ b/xen/arch/riscv/include/asm/smp.h
@@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
 
 void setup_tp(unsigned int cpuid);
 
+struct dt_device_node;
+int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
+
 #endif
 
 /*
diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
index 0371dfa53e..0b00dd0eb2 100644
--- a/xen/arch/riscv/smpboot.c
+++ b/xen/arch/riscv/smpboot.c
@@ -1,5 +1,8 @@
 #include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/types.h>
 #include <xen/sections.h>
 
 cpumask_t __read_mostly cpu_online_map;
@@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(0, &cpu_possible_map);
     cpumask_set_cpu(0, &cpu_online_map);
 }
+
+/**
+ * dt_get_cpuid - Get the cpuid from a CPU device node
+ *
+ * @cpun: CPU number(logical index) for which device node is required
+ *
+ * Return: The cpuid for the CPU node or ~0ULL if not found.
+ */
+static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
+{
+    const __be32 *cell;
+    int ac;
+    uint32_t len;
+
+    ac = dt_n_addr_cells(cpun);
+    cell = dt_get_property(cpun, "reg", &len);
+    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
+        return ~0ULL;
+
+    return dt_read_number(cell, ac);
+}
+
+/*
+ * Returns the cpuid of the given device tree node, or -ENODEV if the node
+ * isn't an enabled and valid RISC-V hart node.
+ */
+int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
+{
+    const char *isa;
+
+    if ( !dt_device_is_compatible(node, "riscv") )
+    {
+        printk("Found incompatible CPU\n");
+        return -ENODEV;
+    }
+
+    *cpuid = dt_get_cpuid(node);
+    if ( *cpuid == ~0UL )
+    {
+        printk("Found CPU without CPU ID\n");
+        return -ENODEV;
+    }
+
+    if ( !dt_device_is_available(node))
+    {
+        printk("CPU with cpuid=%lu is not available\n", *cpuid);
+        return -ENODEV;
+    }
+
+    if ( dt_property_read_string(node, "riscv,isa", &isa) )
+    {
+        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
+        return -ENODEV;
+    }
+
+    if ( isa[0] != 'r' || isa[1] != 'v' )
+    {
+        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
+        return -ENODEV;
+    }
+
+    return 0;
+}
-- 
2.49.0

[PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Oleksii Kurochko]

Introduce the intc_hw_operations structure to encapsulate interrupt
controller-specific data and operations. This structure includes:
- A pointer to interrupt controller information (`intc_info`)
- Callbacks to initialize the controller and set IRQ type/priority
- A reference to an interupt controller descriptor (`host_irq_type`)
- number of interrupt controller irqs.

Add function register_intc_ops() to mentioned above structure.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - This patch was part of "xen/riscv: Introduce intc_hw_operations abstraction"
   and splitted to have ability to merge some patches from this patch series
   into one patch.
 - Declare host_irq_type member of intc_hw_operations as pointer-to-const.
 - Moved up forward declaration of irq_desc.
 - Use attribute __init for register_intc_ops().
 - Use attribute __ro_after_init for intc_hw_ops variable.
 - Update prototype of register_intc_ops() because of what mention in the
   previous point.
---
 xen/arch/riscv/include/asm/intc.h | 22 ++++++++++++++++++++++
 xen/arch/riscv/intc.c             |  9 +++++++++
 2 files changed, 31 insertions(+)

diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 52ba196d87..e72d5fd9d3 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -8,6 +8,8 @@
 #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
 #define ASM__RISCV__INTERRUPT_CONTOLLER_H
 
+#include <xen/irq.h>
+
 enum intc_version {
     INTC_APLIC,
 };
@@ -17,6 +19,26 @@ struct intc_info {
     const struct dt_device_node *node;
 };
 
+struct irq_desc;
+
+struct intc_hw_operations {
+    /* Hold intc hw information */
+    const struct intc_info *info;
+    /* Initialize the intc and the boot CPU */
+    int (*init)(void);
+
+    /* hw_irq_controller to enable/disable/eoi host irq */
+    const hw_irq_controller *host_irq_type;
+
+    /* Set IRQ type */
+    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);
+
+};
+
 void intc_preinit(void);
 
+void register_intc_ops(struct intc_hw_operations *ops);
+
 #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 4061a3c457..122e7b32b5 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -5,6 +5,15 @@
 #include <xen/init.h>
 #include <xen/lib.h>
 
+#include <asm/intc.h>
+
+static struct __ro_after_init intc_hw_operations *intc_hw_ops;
+
+void __init register_intc_ops(struct intc_hw_operations *ops)
+{
+    intc_hw_ops = ops;
+}
+
 void __init intc_preinit(void)
 {
     if ( acpi_disabled )
-- 
2.49.0

[PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Oleksii Kurochko]

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_cpuid() 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 V2:
 - Drop years in copyrights.
 - s/riscv_of_processor_hartid/dt_processor_cpuid.
 - s/imsic_get_parent_hartid/imsic_get_parent_cpuid.
   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             | 286 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/imsic.h |  65 +++++++
 3 files changed, 352 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..43d0c92cbd
--- /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) Microchip Technology Inc.
+ * (c) 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;
+
+/* Callers aren't expected to changed imsic_cfg so return const. */
+const struct imsic_config *imsic_get_config(void)
+{
+    return &imsic_cfg;
+}
+
+static int __init imsic_get_parent_cpuid(const struct dt_device_node *node,
+                                         unsigned int index,
+                                         unsigned long *cpuid)
+{
+    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_cpuid(args.np->parent, cpuid);
+
+    return res;
+}
+
+static int imsic_parse_node(const 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;
+    }
+
+    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", node->name);
+        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);
+        if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs )
+            imsic_cfg.hart_index_bits++;
+    }
+    tmp -= 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 */
+    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", node->name);
+        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", 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 */
+    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) ||
+         ((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 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 {
+        imsic_cfg.nr_mmios++;
+    } while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL) );
+
+    return 0;
+}
+
+int __init imsic_init(const struct dt_device_node *node)
+{
+    int rc;
+    unsigned long reloff, cpuid;
+    uint32_t nr_parent_irqs, index, nr_handlers = 0;
+    paddr_t base_addr;
+    unsigned int nr_mmios;
+
+    /* Parse IMSIC node */
+    rc = imsic_parse_node(node, &nr_parent_irqs);
+    if ( rc )
+        return rc;
+
+    nr_mmios = imsic_cfg.nr_mmios;
+
+    /* Allocate MMIO resource array */
+    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios);
+    if ( !imsic_cfg.mmios )
+        return -ENOMEM;
+
+    imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs);
+    if ( !imsic_cfg.msi )
+        return -ENOMEM;
+
+    /* Check MMIO register sets */
+    for ( unsigned int i = 0; i < nr_mmios; i++ )
+    {
+        imsic_cfg.mmios[i].cpus = xzalloc_array(unsigned long,
+                                                BITS_TO_LONGS(nr_parent_irqs));
+        if ( !imsic_cfg.mmios[i].cpus )
+            return -ENOMEM;
+
+        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 ( unsigned int i = 0; i < nr_parent_irqs; i++ )
+    {
+        rc = imsic_get_parent_cpuid(node, i, &cpuid);
+        if ( rc )
+        {
+            printk(XENLOG_WARNING "%s: cpu ID for parent irq%d not found\n",
+                   node->name, i);
+            continue;
+        }
+
+        if ( cpuid >= num_possible_cpus() )
+        {
+            printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent irq%d\n",
+                   node->name, cpuid, i);
+            continue;
+        }
+
+        /* Find MMIO location of MSI page */
+        index = nr_mmios;
+        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ;
+        for ( unsigned int j = 0; 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 >= nr_mmios )
+        {
+            printk(XENLOG_WARNING "%s: MMIO not found for parent irq%d\n",
+                   node->name, i);
+            continue;
+        }
+
+        if ( !IS_ALIGNED(imsic_cfg.msi[cpuid].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[cpuid].base_addr + reloff);
+            imsic_cfg.msi[cpuid].offset = 0;
+            imsic_cfg.msi[cpuid].base_addr = 0;
+            continue;
+        }
+
+        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);
+        imsic_cfg.msi[cpuid].base_addr = imsic_cfg.mmios[index].base_addr;
+        imsic_cfg.msi[cpuid].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:
+    for ( unsigned int i = 0; i < nr_mmios; i++ )
+        XFREE(imsic_cfg.mmios[i].cpus);
+    XFREE(imsic_cfg.mmios);
+    XFREE(imsic_cfg.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..ed51cac780
--- /dev/null
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -0,0 +1,65 @@
+/* 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            2047
+
+struct imsic_msi {
+    paddr_t base_addr;
+    unsigned long offset;
+};
+
+struct imsic_mmios {
+    paddr_t base_addr;
+    unsigned long size;
+    unsigned long *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;
+};
+
+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 */
-- 
2.49.0

[PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Oleksii Kurochko]

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 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             | 106 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/aplic.h |  64 +++++++++++++++++
 xen/arch/riscv/include/asm/intc.h  |   3 +
 4 files changed, 207 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..8a208dba8a
--- /dev/null
+++ b/xen/arch/riscv/aplic-priv.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/aplic.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 *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..797e5df020 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -9,19 +9,121 @@
  * 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(-1U, &aplic.regs->clrie[i]);
+
+    /* Set interrupt type and default priority for all interrupts */
+    for ( i = 1; i <= aplic_info.num_irqs; i++ )
+    {
+        writel(0, &aplic.regs->sourcecfg[i - 1]);
+        /*
+         * 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 - 1]);
+    }
+
+    writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
+}
+
+static int __init cf_check aplic_init(void)
+{
+    int rc;
+    dt_phandle imsic_phandle;
+    uint32_t irq_range[num_possible_cpus() * 2];
+    const __be32 *prop;
+    uint64_t size, paddr;
+    struct dt_device_node *imsic_node;
+    const struct dt_device_node *node = aplic_info.node;
+
+    /* Check for associated imsic node */
+    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
+    if ( !rc )
+        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 = dt_property_read_u32_array(imsic_node, "interrupts-extended",
+                                    irq_range, ARRAY_SIZE(irq_range));
+    if ( rc )
+        panic("%s: unable to find interrupt-extended in %s node\n",
+              __func__, imsic_node->full_name);
+
+    if ( irq_range[1] == IRQ_M_EXT )
+        /* Machine mode imsic node, ignore this aplic node */
+        return 0;
+
+    for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
+    {
+        if ( irq_range[i + 1] != irq_range[1] )
+            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
+    }
+
+    rc = imsic_init(imsic_node);
+    if ( rc )
+        panic("%s: Failded to initialize IMSIC\n", node->full_name);
+
+    /* Find out number of interrupt sources */
+    rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.num_irqs);
+    if ( !rc )
+        panic("%s: failed to get number of interrupt sources\n",
+              node->full_name);
+
+    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);
+
+    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 struct intc_hw_operations __ro_after_init 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 +155,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..6221030a68
--- /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, UL)
+#define APLIC_DOMAINCFG_DM      BIT(2, UL)
+
+struct aplic_regs {
+    uint32_t domaincfg;
+    uint32_t sourcecfg[1023];
+    uint8_t _reserved1[0xBC0];
+
+    uint32_t mmsiaddrcfg;
+    uint32_t mmsiaddrcfgh;
+    uint32_t smsiaddrcfg;
+    uint32_t smsiaddrcfgh;
+    uint8_t _reserved2[0x30];
+
+    uint32_t setip[32];
+    uint8_t _reserved3[92];
+
+    uint32_t setipnum;
+    uint8_t _reserved4[0x20];
+
+    uint32_t in_clrip[32];
+    uint8_t _reserved5[92];
+
+    uint32_t clripnum;
+    uint8_t _reserved6[32];
+
+    uint32_t setie[32];
+    uint8_t _reserved7[92];
+
+    uint32_t setienum;
+    uint8_t _reserved8[32];
+
+    uint32_t clrie[32];
+    uint8_t _reserved9[92];
+
+    uint32_t clrienum;
+    uint8_t _reserved10[32];
+
+    uint32_t setipnum_le;
+    uint32_t setipnum_be;
+    uint8_t _reserved11[4088];
+
+    uint32_t genmsi;
+    uint32_t target[1023];
+};
+
+#endif /* ASM__RISCV__APLIC_H */
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index e72d5fd9d3..2d55d74a2e 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -17,6 +17,9 @@ enum intc_version {
 struct intc_info {
     enum intc_version hw_version;
     const struct dt_device_node *node;
+
+    /* number of irqs */
+    unsigned int num_irqs;
 };
 
 struct irq_desc;
-- 
2.49.0

[PATCH v2 12/16] xen/riscv: introduce intc_init() and helpers

[Oleksii Kurochko]

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>
---
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             | 45 +++++++++++++++++++++++++++++++
 xen/arch/riscv/setup.c            |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 2d55d74a2e..45a41147a6 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -44,4 +44,8 @@ void intc_preinit(void);
 
 void register_intc_ops(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 122e7b32b5..15f358601d 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,45 @@ void __init intc_preinit(void)
     else
         panic("ACPI interrupt controller preinit() isn't implemented\n");
 }
+
+void __init intc_init(void)
+{
+    ASSERT(intc_hw_ops);
+
+    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);
+    ASSERT(intc_hw_ops);
+
+    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));
+    ASSERT(intc_hw_ops);
+
+    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 82f8839eff..4f92266224 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     intc_preinit();
 
+    intc_init();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.49.0

[PATCH v2 13/16] xen/riscv: implementation of aplic and imsic operations

[Oleksii Kurochko]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=all, Size: 13637 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>
---
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             | 113 +++++++++++++++++++++++++++++
 xen/arch/riscv/imsic.c             | 104 ++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/aplic.h |   4 +-
 xen/arch/riscv/include/asm/imsic.h |  18 +++++
 5 files changed, 242 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/aplic-priv.h b/xen/arch/riscv/aplic-priv.h
index 8a208dba8a..a6cfed4ee0 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 *regs;
 
+    /* lock */
+    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 797e5df020..e2bee7ad23 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,6 +24,7 @@
 #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
@@ -119,9 +121,118 @@ static int __init cf_check aplic_init(void)
     return 0;
 }
 
+static void aplic_irq_enable(struct irq_desc *desc)
+{
+    unsigned long flags;
+
+    /*
+     * 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_irqsave(&aplic.lock, flags);
+
+    /* Enable interrupt in IMSIC */
+    imsic_irq_enable(desc->irq);
+
+    /* Enable interrupt in APLIC */
+    writel(desc->irq, &aplic.regs->setienum);
+
+    spin_unlock_irqrestore(&aplic.lock, flags);
+}
+
+static void aplic_irq_disable(struct irq_desc *desc)
+{
+    unsigned long flags;
+
+    /*
+     * 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_irqsave(&aplic.lock, flags);
+
+    /* disable interrupt in APLIC */
+    writel(desc->irq, &aplic.regs->clrienum);
+
+    /* disable interrupt in IMSIC */
+    imsic_irq_disable(desc->irq);
+
+    spin_unlock_irqrestore(&aplic.lock, flags);
+}
+
+static unsigned int 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 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));
+
+    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
+    hhxw = imsic->group_index_bits;
+    lhxw = imsic->hart_index_bits;
+    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 struct intc_hw_operations __ro_after_init 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,
@@ -159,6 +270,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
 
     dt_irq_xlate = aplic_irq_xlate;
 
+    spin_lock_init(&aplic.lock);
+
     register_intc_ops(&aplic_ops);
 
     return 0;
diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index 43d0c92cbd..70316d2e97 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -9,17 +9,116 @@
  * (c) Vates
  */
 
+#include <xen/bitops.h>
 #include <xen/const.h>
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/macros.h>
+#include <xen/spinlock.h>
 #include <xen/xmalloc.h>
 
 #include <asm/imsic.h>
 
+#define IMSIC_DISABLE_EIDELIVERY    0
+#define IMSIC_ENABLE_EIDELIVERY     1
+#define IMSIC_DISABLE_EITHRESHOLD   1
+#define IMSIC_ENABLE_EITHRESHOLD    0
+
 static struct imsic_config imsic_cfg;
 
+#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 = (last_id - id < chunk) ? 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)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&imsic_cfg.lock, flags);
+    /*
+     * 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. (l)
+     */
+    imsic_local_eix_update(irq, 1, false, true);
+    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
+}
+
+void imsic_irq_disable(unsigned int irq)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&imsic_cfg.lock, flags);
+    imsic_local_eix_update(irq, 1, false, false);
+    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
+}
+
 /* Callers aren't expected to changed imsic_cfg so return const. */
 const struct imsic_config *imsic_get_config(void)
 {
@@ -274,6 +373,11 @@ int __init imsic_init(const struct dt_device_node *node)
         goto imsic_init_err;
     }
 
+    spin_lock_init(&imsic_cfg.lock);
+
+    /* Enable local interrupt delivery */
+    imsic_ids_local_delivery(true);
+
     return 0;
 
  imsic_init_err:
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index 6221030a68..dc4ccbb9aa 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: MIT */
 
 /*
- * xen/arch/riscv/asm/include/aplic.h
+ * xen/arch/riscv/aplic.h
  *
  * RISC-V Advanced Platform-Level Interrupt Controller support
  *
@@ -18,6 +18,8 @@
 #define APLIC_DOMAINCFG_IE      BIT(8, UL)
 #define APLIC_DOMAINCFG_DM      BIT(2, UL)
 
+#define APLIC_TARGET_HART_IDX_SHIFT 18
+
 struct aplic_regs {
     uint32_t domaincfg;
     uint32_t sourcecfg[1023];
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index ed51cac780..1d6ab4d685 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;
@@ -55,6 +65,9 @@ struct imsic_config {
 
     /* MSI */
     struct imsic_msi *msi;
+
+    /* lock */
+    spinlock_t lock;
 };
 
 struct dt_device_node;
@@ -62,4 +75,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.49.0

[PATCH v2 14/16] xen/riscv: add external interrupt handling for hypervisor mode

[Oleksii Kurochko]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=all, Size: 10964 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>
---
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             | 70 ++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/aplic.h |  7 +++
 xen/arch/riscv/include/asm/imsic.h |  1 +
 xen/arch/riscv/include/asm/intc.h  |  5 +++
 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, 159 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index e2bee7ad23..ef7fc2562d 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>
@@ -220,6 +221,70 @@ static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t *mask)
     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;
+
+    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:
+        fallthrough;
+    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,
@@ -233,6 +298,8 @@ static struct intc_hw_operations __ro_after_init 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,
@@ -274,6 +341,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 dc4ccbb9aa..661d9f294f 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, UL)
 #define APLIC_DOMAINCFG_DM      BIT(2, UL)
 
+#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 1d6ab4d685..c765d3d8e4 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 45a41147a6..1efa80fff6 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -22,6 +22,7 @@ struct intc_info {
     unsigned int num_irqs;
 };
 
+struct cpu_user_regs;
 struct irq_desc;
 
 struct intc_hw_operations {
@@ -38,6 +39,8 @@ struct intc_hw_operations {
     /* 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);
@@ -48,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 6223bbbed5..1a05c5ff88 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -25,16 +25,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 15f358601d..478401cc74 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -54,6 +54,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 4c518bbd97..056bdf3ca8 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];
 
@@ -85,3 +89,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.49.0

[PATCH v2 15/16] xen/riscv: implement setup_irq()

[Oleksii Kurochko]

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>
---
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 1a05c5ff88..d35fac0a86 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -9,6 +9,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 056bdf3ca8..969e22395d 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>
@@ -58,6 +59,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.49.0

[PATCH v2 16/16] xen/riscv: add basic UART support

[Oleksii Kurochko]

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>
---
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   | 13 +++++++++++++
 xen/drivers/char/Kconfig |  3 +--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 60520dab57..eb44318dda 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 4f92266224..5c7cd568f0 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -4,12 +4,16 @@
 #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/percpu.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>
 
@@ -136,8 +140,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.49.0

Re: [PATCH v2 01/16] xen/riscv: initialize bitmap to zero in riscv_fill_hwcap_from_isa_string()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> The this_isa bitmap should be explicitly initialized to zero to avoid
> false positives when detecting supported ISA extensions. Without proper
> zero-initialization, the bitmap may retain non-zero values from
> uninitialized memory, causing Xen to incorrectly assume that certain
> extensions are supported.

Why "uninitialized" together with "retain values". Yes, the variable is
indeed uninitialized, like any automatic ones are when they don't have
an initializer. I'm not going to request re-wording, but to me the
above read as if you were concerned of a static variable ...

> This change ensures reliable detection of ISA capabilities.
> 
> Fixes: 0c2f717eae ("xen/riscv: identify specific ISA supported by cpu")
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 02/16] xen/riscv: introduce smp_prepare_boot_cpu()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> @@ -72,6 +72,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      remove_identity_mapping();
>  
> +    smp_prepare_boot_cpu();
> +
>      set_processor_id(0);
>  
>      set_cpuid_to_hartid(0, bootcpu_id);

Is this a good placement? I'd think that smp_prepare_boot_cpu() ought to be
permitted to rely on set_processor_id() already having run, for example (even
if right now there's no such dependency). Alternatively the set_processor_id()
call may want to move into the new function?

Jan

Re: [PATCH v2 03/16] xen/riscv: introduce support of Svpbmt extension

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> Svpbmt extension is necessary for chaning the memory type for a page contains
> a combination of attributes that indicate the cacheability, idempotency,
> and ordering properties for access to that page.

The title suggest use of the extension is optional.

> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -10,11 +10,25 @@ config RISCV
>  config RISCV_64
>  	def_bool y
>  	select 64BIT
> +	select HAS_SVPBMT

Such redundant ...

>  config ARCH_DEFCONFIG
>  	string
>  	default "arch/riscv/configs/tiny64_defconfig"
>  
> +config HAS_SVPBMT
> +	bool
> +	depends on RISCV_64

... dependencies are frowned upon, afaik. And it's pretty certainly not
needed here.

> +	help
> +	  This config enables usage of Svpbmt ISA-extension ( Supervisor-mode:
> +	  page-based memory types).
> +
> +	  The memory type for a page contains a combination of attributes
> +	  that indicate the cacheability, idempotency, and ordering
> +	  properties for access to that page.
> +
> +	  The Svpbmt extension is only available on 64-bit cpus.

I don't mind the help text, but for a prompt-less option it's of little
use (beyond what a comment could also achieve).

> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -46,6 +46,8 @@
>  #define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
>  
>  #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
> +#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PMBT_IO)
> +#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PMBT_NOCACHE)

Hmm, odd - NOCACHE doesn't really mean "no cache" then? I think this
would require a comment then.

> @@ -56,8 +58,21 @@
>  #define PTE_SMALL       BIT(10, UL)
>  #define PTE_POPULATE    BIT(11, UL)
>  
> +/*
> + * [62:61] Svpbmt Memory Type definitions:
> + *
> + *  00 - PMA    Normal Cacheable, No change to implied PMA memory type
> + *  01 - NC     Non-cacheable, idempotent, weakly-ordered Main Memory
> + *  10 - IO     Non-cacheable, non-idempotent, strongly-ordered I/O memory
> + *  11 - Rsvd   Reserved for future standard use
> + */
> +#define PTE_PMBT_NOCACHE    BIT(61, UL)
> +#define PTE_PMBT_IO         BIT(62, UL)

Unlike PTE_SMALL and PTE_POPULATE these are arch-defined; I think they
want to move up to where the other arch-defined bits are, thus also
maping them appear before their first use.

Jan

Re: [PATCH v2 04/16] xen/riscv: add ioremap_*() variants using ioremap_attr()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> @@ -583,3 +584,36 @@ void *__init arch_vmap_virt_end(void)
>  {
>      return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>  }
> +
> +static void *ioremap_attr(paddr_t start, size_t len, pte_attr_t attributes)
> +{
> +    mfn_t mfn = _mfn(PFN_DOWN(start));
> +    unsigned int offs = start & (PAGE_SIZE - 1);
> +    unsigned int nr = PFN_UP(offs + len);
> +    void *ptr = __vmap(&mfn, nr, 1, 1, attributes, VMAP_DEFAULT);
> +
> +    if ( ptr == NULL )
> +        return NULL;
> +
> +    return ptr + offs;
> +}
> +
> +void __iomem *ioremap_nocache(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_NOCACHE);
> +}

Why do you need both this and ...

> +void __iomem *ioremap_cache(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR);
> +}
> +
> +void __iomem *ioremap_wc(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_WC);
> +}
> +
> +void *ioremap(paddr_t pa, size_t len)
> +{
> +    return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
> +}

... this? And why's the 1st parameter named differently for this last
one? Can't they all be in sync in this regard?

Jan

Re: [PATCH v2 05/16] xen/asm-generic: introduce asm-generic/irq-dt.h

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> Introduce defintions of IRQ_TYPE_* which correspond to the Xen internal
> representation of the IRQ types and make them the same asthe existing
> device tree defintions for convenience.
> 
> These defines are going to be re-used, at least, by RISC-V architecture.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Assuming an Arm ack would arrive, this looks like it's independent of the
earlier patches, and hence could go in right away?

Jan

Re: [PATCH v2 06/16] xen/riscv: introduce init_IRQ()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -3,6 +3,11 @@
>  #define ASM__RISCV__IRQ_H
>  
>  #include <xen/bug.h>
> +#include <xen/device_tree.h>
> +
> +#include <asm/irq-dt.h>
> +
> +#define NR_IRQS 1024

Is this arbitrary or arch-induced? Imo it wants saying in a brief comment either
way. Then again maybe it's entirely obvious for a RISC-V person ...

> --- /dev/null
> +++ b/xen/arch/riscv/irq.c
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/*
> + * RISC-V Trap handlers
> + *
> + * Copyright (c) 2024 Vates
> + */
> +
> +#include <xen/bug.h>
> +#include <xen/init.h>
> +#include <xen/irq.h>
> +
> +static irq_desc_t irq_desc[NR_IRQS];
> +
> +int arch_init_one_irq_desc(struct irq_desc *desc)
> +{
> +    desc->arch.type = IRQ_TYPE_INVALID;
> +
> +    return 0;
> +}
> +
> +static int __init init_irq_data(void)
> +{
> +    unsigned int irq;
> +
> +    for ( irq = 0; irq < NR_IRQS; irq++ )
> +    {
> +        struct irq_desc *desc = irq_to_desc(irq);
> +        int rc = init_one_irq_desc(desc);
> +
> +        if ( rc )
> +            return rc;
> +
> +        desc->irq = irq;
> +        desc->action  = NULL;

Nit: You copied a stray blank from Arm code. Actually I wonder why it isn't
init_one_irq_desc() that clears this field. It also feels like ->irq would
better be set ahead of calling that function, like x86 has it.

Jan

Re: [PATCH v2 05/16] xen/asm-generic: introduce asm-generic/irq-dt.h

[Stefano Stabellini]

On Wed, 14 May 2025, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
> > Introduce defintions of IRQ_TYPE_* which correspond to the Xen internal
> > representation of the IRQ types and make them the same asthe existing
> > device tree defintions for convenience.
> > 
> > These defines are going to be re-used, at least, by RISC-V architecture.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Assuming an Arm ack would arrive, this looks like it's independent of the
> earlier patches, and hence could go in right away?

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v2 07/16] xen/riscv: introduce platform_get_irq()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> platform_get_irq() recieves information about device's irq ( type
> and irq number ) from device tree node and using this information
> update irq descriptor in irq_desc[] array.
> 
> Introduce dt_irq_xlate and initialize with aplic_irq_xlate() as
> it is used by dt_device_get_irq() which is called by
> platform_get_irq().
> 
> Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V2:
>  - Add cf_check for aplic_irq_xlate().
>  - Ident label in irq_set_type().
>  - Return proper -E... values for platform_get_irq().
> ---
>  xen/arch/riscv/aplic.c           | 20 +++++++++++++++
>  xen/arch/riscv/include/asm/irq.h |  3 +++
>  xen/arch/riscv/irq.c             | 42 ++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
> index caba8f8993..10ae81f7ac 100644
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -11,6 +11,7 @@
>  
>  #include <xen/errno.h>
>  #include <xen/init.h>
> +#include <xen/irq.h>
>  #include <xen/sections.h>
>  #include <xen/types.h>
>  
> @@ -21,6 +22,23 @@ static struct intc_info __ro_after_init aplic_info = {
>      .hw_version = INTC_APLIC,
>  };
>  
> +static int cf_check aplic_irq_xlate(const uint32_t *intspec,
> +                                    unsigned int intsize,
> +                                    unsigned int *out_hwirq,
> +                                    unsigned int *out_type)
> +{
> +    if ( intsize < 2 )
> +        return -EINVAL;
> +
> +    /* Mapping 1:1 */
> +    *out_hwirq = intspec[0];
> +
> +    if ( out_type )
> +        *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +    return 0;
> +}
> +
>  static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
>  {
>      if ( aplic_info.node )
> @@ -35,6 +53,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
>  
>      aplic_info.node = node;
>  
> +    dt_irq_xlate = aplic_irq_xlate;
> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
> index f609df466e..6223bbbed5 100644
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -30,6 +30,9 @@ 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);
>  
>  #endif /* ASM__RISCV__IRQ_H */
> diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
> index 26a8556b2c..4c518bbd97 100644
> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -7,11 +7,53 @@
>   */
>  
>  #include <xen/bug.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  
>  static irq_desc_t irq_desc[NR_IRQS];
>  
> +static bool irq_validate_new_type(unsigned int curr, unsigned int new)
> +{
> +    return (curr == IRQ_TYPE_INVALID || curr == new );

Nit: Stray blank. In fact you could omit the parentheses as well.

> +}
> +
> +static int irq_set_type(unsigned int irq, unsigned int type)
> +{
> +    unsigned long flags;
> +    struct irq_desc *desc = irq_to_desc(irq);
> +    int ret = -EBUSY;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    if ( !irq_validate_new_type(desc->arch.type, type) )
> +        goto err;
> +
> +    desc->arch.type = type;
> +
> +    ret = 0;
> +
> + err:
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    return ret;
> +}
> +
> +int platform_get_irq(const struct dt_device_node *device, int index)
> +{
> +    struct dt_irq dt_irq;
> +    int ret;
> +
> +    if ( (ret = dt_device_get_irq(device, index, &dt_irq)) != 0 )
> +        return ret;
> +
> +    if ( (ret = irq_set_type(dt_irq.irq, dt_irq.type)) != 0 )
> +        return ret;
> +
> +    return dt_irq.irq;

What guarantees the value to be at most INT_MAX (i.e. no silent conversion to
a negative value, signaling an error to the caller)? Actually, looking at
irq_set_type(), what guarantees irq_to_desc() there to not overrun irq_desc[]?
There are no bounds checks in aplic_irq_xlate().

Jan

Re: [PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation

[Jan Beulich]

(adding Bertrand as the one further DT maintainer, for a respective question
below)

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> Implements dt_processor_hartid()

There's no such function (anymore).

> to get the hart ID of the given
> device tree node and do some checks if CPU is available and given device
> tree node has proper riscv,isa property.
> 
> As a helper function dt_get_cpuid() is introduced to deal specifically
> with reg propery of a CPU device node.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V2:
>  - s/of_get_cpu_hwid()/dt_get_cpu_id().
>  - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>  - Add empty line before last return in dt_get_cpu_hwid().
>  - s/riscv_of_processor_hartid/dt_processor_cpuid().
>  - Use pointer-to_const for node argument of dt_processor_cpuid().
>  - Use for hart_id unsigned long type as according to the spec for RV128
>    mhartid register will be 128 bit long.
>  - Update commit message and subject.
>  - use 'CPU' instead of 'HART'.

Was this is good move? What is returned ...

> --- a/xen/arch/riscv/include/asm/smp.h
> +++ b/xen/arch/riscv/include/asm/smp.h
> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>  
>  void setup_tp(unsigned int cpuid);
>  
> +struct dt_device_node;
> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);

... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
not sure it's a hart ID either, so it may need further clarification (and I'd
expect RISC-V to have suitable terminology to tell apart the different entities).

> @@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
>      cpumask_set_cpu(0, &cpu_possible_map);
>      cpumask_set_cpu(0, &cpu_online_map);
>  }
> +
> +/**
> + * dt_get_cpuid - Get the cpuid from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + *
> + * Return: The cpuid for the CPU node or ~0ULL if not found.
> + */
> +static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
> +{
> +    const __be32 *cell;
> +    int ac;

This is bogus (should be unsigned int afaict), but dictated by ...

> +    uint32_t len;
> +
> +    ac = dt_n_addr_cells(cpun);

... the return value here and ...

> +    cell = dt_get_property(cpun, "reg", &len);
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> +        return ~0ULL;

(Nit: This doesn't match the return type of the function; same for
the function comment. Also, what if sizeof(*cell) * ac < len?)

> +    return dt_read_number(cell, ac);

... the function parameter type here. In fact, that function is raising
another question: If the "size" argument is outside of [0, 2], the value
returned is silently truncated.

More generally - are there any plans to make DT code signed-ness-correct?

> +/*
> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
> + * isn't an enabled and valid RISC-V hart node.
> + */
> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
> +{
> +    const char *isa;
> +
> +    if ( !dt_device_is_compatible(node, "riscv") )
> +    {
> +        printk("Found incompatible CPU\n");
> +        return -ENODEV;
> +    }
> +
> +    *cpuid = dt_get_cpuid(node);
> +    if ( *cpuid == ~0UL )
> +    {
> +        printk("Found CPU without CPU ID\n");
> +        return -ENODEV;
> +    }
> +
> +    if ( !dt_device_is_available(node))
> +    {
> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
> +        return -ENODEV;
> +    }
> +
> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
> +    {
> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
> +        return -ENODEV;
> +    }
> +
> +    if ( isa[0] != 'r' || isa[1] != 'v' )
> +    {
> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
> +        return -ENODEV;
> +    }
> +
> +    return 0;
> +}

I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
messages for all of the cases, but surely there are errno values better
representing the individual failure reasons?

Jan

Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/intc.h
> +++ b/xen/arch/riscv/include/asm/intc.h
> @@ -8,6 +8,8 @@
>  #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>  #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>  
> +#include <xen/irq.h>

If you need this include anyway, why ...

> @@ -17,6 +19,26 @@ struct intc_info {
>      const struct dt_device_node *node;
>  };
>  
> +struct irq_desc;

... this "forward" decl for something that's then already fully defined?
I can't, however, spot why xen/irq.h would be needed for anything ...

> +struct intc_hw_operations {
> +    /* Hold intc hw information */
> +    const struct intc_info *info;
> +    /* Initialize the intc and the boot CPU */
> +    int (*init)(void);
> +
> +    /* hw_irq_controller to enable/disable/eoi host irq */
> +    const hw_irq_controller *host_irq_type;
> +
> +    /* Set IRQ type */
> +    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);
> +
> +};
> +
>  void intc_preinit(void);
>  
> +void register_intc_ops(struct intc_hw_operations *ops);
> +
>  #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */

... throughout here.

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -5,6 +5,15 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  
> +#include <asm/intc.h>
> +
> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;

Nit: Attributes between type and identifier please. Also shouldn't both
this and ...

> +void __init register_intc_ops(struct intc_hw_operations *ops)

... the parameter here be pointer-to-const?

Jan

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Jan Beulich]

On 06.05.2025 18:51, 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_cpuid() 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 V2:
>  - Drop years in copyrights.

Did you, really?

>  - s/riscv_of_processor_hartid/dt_processor_cpuid.
>  - s/imsic_get_parent_hartid/imsic_get_parent_cpuid.
>    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             | 286 +++++++++++++++++++++++++++++
>  xen/arch/riscv/include/asm/imsic.h |  65 +++++++
>  3 files changed, 352 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..43d0c92cbd
> --- /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) Microchip Technology Inc.
> + * (c) 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;
> +
> +/* Callers aren't expected to changed imsic_cfg so return const. */
> +const struct imsic_config *imsic_get_config(void)
> +{
> +    return &imsic_cfg;
> +}
> +
> +static int __init imsic_get_parent_cpuid(const struct dt_device_node *node,
> +                                         unsigned int index,
> +                                         unsigned long *cpuid)
> +{
> +    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_cpuid(args.np->parent, cpuid);
> +
> +    return res;
> +}
> +
> +static int imsic_parse_node(const 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;
> +    }
> +
> +    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", node->name);
> +        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);
> +        if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs )
> +            imsic_cfg.hart_index_bits++;
> +    }
> +    tmp -= 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 */
> +    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", node->name);
> +        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", 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 */
> +    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) ||
> +         ((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 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);

Nit: indentation, similarly ...

> +    imsic_cfg.base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
> +                           imsic_cfg.group_index_shift);

... here.

> +    /* Find number of MMIO register sets */
> +    do {
> +        imsic_cfg.nr_mmios++;
> +    } while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL) );
> +
> +    return 0;
> +}
> +
> +int __init imsic_init(const struct dt_device_node *node)
> +{
> +    int rc;
> +    unsigned long reloff, cpuid;
> +    uint32_t nr_parent_irqs, index, nr_handlers = 0;

I can't spot why unsigned int wouldn't be suitable here. In fact e.g. ...

> +    paddr_t base_addr;
> +    unsigned int nr_mmios;
> +
> +    /* Parse IMSIC node */
> +    rc = imsic_parse_node(node, &nr_parent_irqs);

... this one wants to yield unsigned int * according to the function parameter's
type.

> +    if ( rc )
> +        return rc;
> +
> +    nr_mmios = imsic_cfg.nr_mmios;
> +
> +    /* Allocate MMIO resource array */
> +    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios);
> +    if ( !imsic_cfg.mmios )
> +        return -ENOMEM;
> +
> +    imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs);
> +    if ( !imsic_cfg.msi )
> +        return -ENOMEM;

Leaking the earlier successful allocation?

> +    /* Check MMIO register sets */
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +    {
> +        imsic_cfg.mmios[i].cpus = xzalloc_array(unsigned long,
> +                                                BITS_TO_LONGS(nr_parent_irqs));
> +        if ( !imsic_cfg.mmios[i].cpus )
> +            return -ENOMEM;

Leaking all earlier successful allocations?

> +        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",

Nit: Excess blank.

> +                   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);

As above, 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;
> +        }
> +    }
> +
> +    /* Configure handlers for target CPUs */
> +    for ( unsigned int i = 0; i < nr_parent_irqs; i++ )
> +    {
> +        rc = imsic_get_parent_cpuid(node, i, &cpuid);

Coming back to a comment I gave on the respective earlier patch: What you get back
here is a DT value, aiui. There's no translation to Xen CPU numbering space, as
would be required for e.g. ...

> +        if ( rc )
> +        {
> +            printk(XENLOG_WARNING "%s: cpu ID for parent irq%d not found\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( cpuid >= num_possible_cpus() )

... this. Are you using DT numbering without any translation, no matter that it
(I suppose) could be very sparse?

> +        {
> +            printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent irq%d\n",
> +                   node->name, cpuid, i);

Nit: i is unsigned int, so wants formatting with %u (also applicable elsewhere).

> +            continue;
> +        }
> +
> +        /* Find MMIO location of MSI page */
> +        index = nr_mmios;
> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ;
> +        for ( unsigned int j = 0; nr_mmios; j++ )

DYM "j < nr_mmios"?

> +        {
> +            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 >= nr_mmios )

Why is it that you need both "index" and "j"?

> +        {
> +            printk(XENLOG_WARNING "%s: MMIO not found for parent irq%d\n",
> +                   node->name, i);
> +            continue;
> +        }
> +
> +        if ( !IS_ALIGNED(imsic_cfg.msi[cpuid].base_addr + reloff, PAGE_SIZE) )
> +        {
> +            printk(XENLOG_WARNING "%s: MMIO address 0x%lx is not aligned on a page\n",

Please prefer to use %#lx, as we do elsewhere.

> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
> +            imsic_cfg.msi[cpuid].offset = 0;
> +            imsic_cfg.msi[cpuid].base_addr = 0;
> +            continue;
> +        }
> +
> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);

Depending on clarification on the number space used, this may want to be
cpumask_set_cpu() instead. Else I think this is simply __set_bit().

> +        imsic_cfg.msi[cpuid].base_addr = imsic_cfg.mmios[index].base_addr;
> +        imsic_cfg.msi[cpuid].offset = reloff;

How come it's cpuid that's used as array index here? Shouldn't this be i,
seeing that the array allocation is using nr_parent_irqs?

> +        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:
> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
> +        XFREE(imsic_cfg.mmios[i].cpus);

This can be just xfree(), as the array itself ...

> +    XFREE(imsic_cfg.mmios);

... is then also freed.

> +    XFREE(imsic_cfg.msi);
> +
> +    return rc;
> +}
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/imsic.h
> @@ -0,0 +1,65 @@
> +/* 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

This isn't the "minimum ID", but the "minimum permissible number of IDs" as per
its first use in imsic_parse_node(). Further uses there consider it a mask,
which makes me wonder whether the imsic_cfg.nr_ids field name is actually
correct: Isn't this the maximum valid ID rather than their count/number?

Jan

Re: [PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic.h

If already you have such in a comment, please have it be correct.

> + * 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 *regs;

This looks to also want __iomem.

> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -9,19 +9,121 @@
>   * 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"

Besides this, are there going to be any other files including this private
header? If not, why have the header in the first place?

>  #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(-1U, &aplic.regs->clrie[i]);

Imo it's better to use ~0U.

> +    /* Set interrupt type and default priority for all interrupts */
> +    for ( i = 1; i <= aplic_info.num_irqs; i++ )
> +    {
> +        writel(0, &aplic.regs->sourcecfg[i - 1]);

What guarantees the loop to not run past the array's size?

> +        /*
> +         * 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 - 1]);
> +    }

Seeing the subtractions of 1 here, why's the loop header not simply

    for ( i = 0; i < aplic_info.num_irqs; i++ )

(i.e. the more conventional form)?

> +    writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
> +}
> +
> +static int __init cf_check aplic_init(void)
> +{
> +    int rc;
> +    dt_phandle imsic_phandle;
> +    uint32_t irq_range[num_possible_cpus() * 2];

Are you going to have enough stack space when num_possible_cpus() is pretty
large?

> +    const __be32 *prop;
> +    uint64_t size, paddr;
> +    struct dt_device_node *imsic_node;

Pointer-to-const?

> +    const struct dt_device_node *node = aplic_info.node;
> +
> +    /* Check for associated imsic node */
> +    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
> +    if ( !rc )
> +        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 = dt_property_read_u32_array(imsic_node, "interrupts-extended",
> +                                    irq_range, ARRAY_SIZE(irq_range));
> +    if ( rc )
> +        panic("%s: unable to find interrupt-extended in %s node\n",
> +              __func__, imsic_node->full_name);
> +
> +    if ( irq_range[1] == IRQ_M_EXT )

How do you know the array has had 2 or ...

> +        /* Machine mode imsic node, ignore this aplic node */
> +        return 0;
> +
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
> +    {
> +        if ( irq_range[i + 1] != irq_range[1] )
> +            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
> +    }

... or even all of the slots populated? Anything not populated by the DT
function will still have the stack contents that had been left by earlier
call chains. (The loop also makes little sense to start from 0.)

I'm also puzzled by there not being any further use of the values later
in the function.

> +    rc = imsic_init(imsic_node);
> +    if ( rc )
> +        panic("%s: Failded to initialize IMSIC\n", node->full_name);
> +
> +    /* Find out number of interrupt sources */
> +    rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.num_irqs);
> +    if ( !rc )

Assigning a bool return value to an int local var, which generally hold
error codes, is confusing. I don't think you really need to use a local
variable here.

> --- /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, UL)
> +#define APLIC_DOMAINCFG_DM      BIT(2, UL)
> +
> +struct aplic_regs {
> +    uint32_t domaincfg;
> +    uint32_t sourcecfg[1023];
> +    uint8_t _reserved1[0xBC0];
> +
> +    uint32_t mmsiaddrcfg;
> +    uint32_t mmsiaddrcfgh;
> +    uint32_t smsiaddrcfg;
> +    uint32_t smsiaddrcfgh;
> +    uint8_t _reserved2[0x30];
> +
> +    uint32_t setip[32];
> +    uint8_t _reserved3[92];
> +
> +    uint32_t setipnum;
> +    uint8_t _reserved4[0x20];
> +
> +    uint32_t in_clrip[32];
> +    uint8_t _reserved5[92];
> +
> +    uint32_t clripnum;
> +    uint8_t _reserved6[32];
> +
> +    uint32_t setie[32];
> +    uint8_t _reserved7[92];
> +
> +    uint32_t setienum;
> +    uint8_t _reserved8[32];
> +
> +    uint32_t clrie[32];
> +    uint8_t _reserved9[92];
> +
> +    uint32_t clrienum;
> +    uint8_t _reserved10[32];
> +
> +    uint32_t setipnum_le;
> +    uint32_t setipnum_be;
> +    uint8_t _reserved11[4088];

I think you want to be consistent with the dimensions of at least all the
_reserved*[] fields - use decimal or use hex everywhere. Even better would
be if that was consistent across all array dimensions.

Jan

Re: [PATCH v2 12/16] xen/riscv: introduce intc_init() and helpers

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> 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>
> ---
> 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             | 45 +++++++++++++++++++++++++++++++
>  xen/arch/riscv/setup.c            |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
> index 2d55d74a2e..45a41147a6 100644
> --- a/xen/arch/riscv/include/asm/intc.h
> +++ b/xen/arch/riscv/include/asm/intc.h
> @@ -44,4 +44,8 @@ void intc_preinit(void);
>  
>  void register_intc_ops(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 122e7b32b5..15f358601d 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,45 @@ void __init intc_preinit(void)
>      else
>          panic("ACPI interrupt controller preinit() isn't implemented\n");
>  }
> +
> +void __init intc_init(void)
> +{
> +    ASSERT(intc_hw_ops);

What's the goal of this check (also elsewhere below)? You'll crash anyway ...

> +    if ( intc_hw_ops->init() )

... here if the point is still NULL, just like you will if the function
pointer is unpopulated (and hence NULL).

Preferably with all of these (but not the other assertions) dropped
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 13/16] xen/riscv: implementation of aplic and imsic operations

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- 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 *regs;
>  
> +    /* lock */
> +    spinlock_t lock;

Nit: I don't see much value in such entirely redundant comments. Useful
contents might be to say what the lock actually is intended to guard.

> @@ -119,9 +121,118 @@ static int __init cf_check aplic_init(void)
>      return 0;
>  }
>  
> +static void aplic_irq_enable(struct irq_desc *desc)
> +{
> +    unsigned long flags;
> +
> +    /*
> +     * 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));

Iirc I said so before: This being an IRQ-safe lock, ...

> +    spin_lock_irqsave(&aplic.lock, flags);

... there's no need to use spin_lock_irqsave() here; plain spin_lock()
will do (and avoid the need for the local variable). Same elsewhere,
obviously.

> +static void 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));
> +
> +    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
> +    hhxw = imsic->group_index_bits;
> +    lhxw = imsic->hart_index_bits;
> +    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);

Hmm, interesting, here you use the plain functions, but there's no
assertion as to desc->lock being held. Such aspects would better be
consistent throughout all hooks.

> @@ -159,6 +270,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
>  
>      dt_irq_xlate = aplic_irq_xlate;
>  
> +    spin_lock_init(&aplic.lock);

Can't you have the struct field have a suitable initializer?

> +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 = (last_id - id < chunk) ? 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)
> +{
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&imsic_cfg.lock, flags);
> +    /*
> +     * 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. (l)

What's this 'l' in parentheses here to indicate?

> +     */
> +    imsic_local_eix_update(irq, 1, false, true);
> +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
> +}
> +
> +void imsic_irq_disable(unsigned int irq)
> +{
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&imsic_cfg.lock, flags);
> +    imsic_local_eix_update(irq, 1, false, false);
> +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
> +}

The sole caller of the function has doubly turned off IRQs already; perhaps
no need to it a 3rd time, unless other callers are to appear? Same for
imsic_irq_enable() as it looks.

> @@ -274,6 +373,11 @@ int __init imsic_init(const struct dt_device_node *node)
>          goto imsic_init_err;
>      }
>  
> +    spin_lock_init(&imsic_cfg.lock);

Again - suitable initializer for the field instead?

> --- a/xen/arch/riscv/include/asm/aplic.h
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: MIT */
>  
>  /*
> - * xen/arch/riscv/asm/include/aplic.h
> + * xen/arch/riscv/aplic.h

Please get this right when/where the file is introduced.

Jan

Re: [PATCH v2 14/16] xen/riscv: add external interrupt handling for hypervisor mode

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> +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;
> +
> +    spin_lock(&aplic.lock);
> +
> +    switch(type)

Nit: style

> +    {
> +    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:
> +        fallthrough;

This is pointless (and hampering readability) when there are no other statements.

With both taken care of:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 15/16] xen/riscv: implement setup_irq()

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> @@ -58,6 +59,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

Didn't you indicate you'd drop this?

Jan

Re: [PATCH v2 16/16] xen/riscv: add basic UART support

[Jan Beulich]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -4,12 +4,16 @@
>  #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/percpu.h>

Why's this needed? I can't spot anything ...

> +#include <xen/serial.h>
>  #include <xen/shutdown.h>
>  #include <xen/smp.h>
> +#include <xen/timer.h>
>  #include <xen/vmap.h>
>  #include <xen/xvmalloc.h>
>  
> @@ -136,8 +140,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();

... relevant here. With the need clarified or with the #include dropped:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 02/16] xen/riscv: introduce smp_prepare_boot_cpu()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]


On 5/13/25 5:48 PM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> @@ -72,6 +72,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>   
>>       remove_identity_mapping();
>>   
>> +    smp_prepare_boot_cpu();
>> +
>>       set_processor_id(0);
>>   
>>       set_cpuid_to_hartid(0, bootcpu_id);
> Is this a good placement? I'd think that smp_prepare_boot_cpu() ought to be
> permitted to rely on set_processor_id() already having run, for example (even
> if right now there's no such dependency). Alternatively the set_processor_id()
> call may want to move into the new function?

Agree, logically it would be better to set processor id before smp_prepare_boot_cpu().

I'll move set_processor_id(0) inside smp_prepare_boot_cpu().

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1274 bytes --]

Re: [PATCH v2 03/16] xen/riscv: introduce support of Svpbmt extension

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]


On 5/13/25 6:00 PM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> Svpbmt extension is necessary for chaning the memory type for a page contains
>> a combination of attributes that indicate the cacheability, idempotency,
>> and ordering properties for access to that page.
> The title suggest use of the extension is optional.
>
>> --- a/xen/arch/riscv/Kconfig
>> +++ b/xen/arch/riscv/Kconfig
>> @@ -10,11 +10,25 @@ config RISCV
>>   config RISCV_64
>>   	def_bool y
>>   	select 64BIT
>> +	select HAS_SVPBMT
> Such redundant ...
>
>>   config ARCH_DEFCONFIG
>>   	string
>>   	default "arch/riscv/configs/tiny64_defconfig"
>>   
>> +config HAS_SVPBMT
>> +	bool
>> +	depends on RISCV_64
> ... dependencies are frowned upon, afaik. And it's pretty certainly not
> needed here.
>
>> +	help
>> +	  This config enables usage of Svpbmt ISA-extension ( Supervisor-mode:
>> +	  page-based memory types).
>> +
>> +	  The memory type for a page contains a combination of attributes
>> +	  that indicate the cacheability, idempotency, and ordering
>> +	  properties for access to that page.
>> +
>> +	  The Svpbmt extension is only available on 64-bit cpus.
> I don't mind the help text, but for a prompt-less option it's of little
> use (beyond what a comment could also achieve).

I'll drop "depends on RISCV_64" for HAS_SVOBMT and leave only 'select HAS_SVPBMT'
and move the help text to commit messaage.

>
>> --- a/xen/arch/riscv/include/asm/page.h
>> +++ b/xen/arch/riscv/include/asm/page.h
>> @@ -46,6 +46,8 @@
>>   #define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
>>   
>>   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
>> +#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PMBT_IO)
>> +#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PMBT_NOCACHE)
> Hmm, odd - NOCACHE doesn't really mean "no cache" then? I think this
> would require a comment then.

According to the table (Svpbmt Memory Type definitions) in the comment below these
definitions both of them (PTE_PMBT_IO and PTE_PMBT_NOCACHE) are non-cachable.

I wasn't sure what and for what should be used so I did in sync with Arm which
defines "#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)" where
MT_DEVICE_nGnRE is equivalent of PTE_PMBT_IO.

I can add the comment above definition of PAGE_HYPERVISOR_NOCACHE:
/* Non-cacheable, non-idempotent, strongly-ordered I/O memory */.

Something similar then I can add for PAGE_HYPERVISOR_WC:
/* Non-cacheable, idempotent, weakly-ordered Main Memory */

>
>> @@ -56,8 +58,21 @@
>>   #define PTE_SMALL       BIT(10, UL)
>>   #define PTE_POPULATE    BIT(11, UL)
>>   
>> +/*
>> + * [62:61] Svpbmt Memory Type definitions:
>> + *
>> + *  00 - PMA    Normal Cacheable, No change to implied PMA memory type
>> + *  01 - NC     Non-cacheable, idempotent, weakly-ordered Main Memory
>> + *  10 - IO     Non-cacheable, non-idempotent, strongly-ordered I/O memory
>> + *  11 - Rsvd   Reserved for future standard use
>> + */
>> +#define PTE_PMBT_NOCACHE    BIT(61, UL)
>> +#define PTE_PMBT_IO         BIT(62, UL)
> Unlike PTE_SMALL and PTE_POPULATE these are arch-defined; I think they
> want to move up to where the other arch-defined bits are, thus also
> maping them appear before their first use.

Sure, I'll move them up.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 4785 bytes --]

Re: [PATCH v2 04/16] xen/riscv: add ioremap_*() variants using ioremap_attr()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]


On 5/14/25 4:32 PM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> @@ -583,3 +584,36 @@ void *__init arch_vmap_virt_end(void)
>>   {
>>       return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>>   }
>> +
>> +static void *ioremap_attr(paddr_t start, size_t len, pte_attr_t attributes)
>> +{
>> +    mfn_t mfn = _mfn(PFN_DOWN(start));
>> +    unsigned int offs = start & (PAGE_SIZE - 1);
>> +    unsigned int nr = PFN_UP(offs + len);
>> +    void *ptr = __vmap(&mfn, nr, 1, 1, attributes, VMAP_DEFAULT);
>> +
>> +    if ( ptr == NULL )
>> +        return NULL;
>> +
>> +    return ptr + offs;
>> +}
>> +
>> +void __iomem *ioremap_nocache(paddr_t start, size_t len)
>> +{
>> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_NOCACHE);
>> +}
> Why do you need both this and ...
>
>> +void __iomem *ioremap_cache(paddr_t start, size_t len)
>> +{
>> +    return ioremap_attr(start, len, PAGE_HYPERVISOR);
>> +}
>> +
>> +void __iomem *ioremap_wc(paddr_t start, size_t len)
>> +{
>> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_WC);
>> +}
>> +
>> +void *ioremap(paddr_t pa, size_t len)
>> +{
>> +    return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
>> +}
> ... this? And why's the 1st parameter named differently for this last
> one? Can't they all be in sync in this regard?

Originally, I thought|ioremap_nocache()| was needed because it is used in
common code. However, I now realize that the calls to|ioremap_nocache()|
in|xen/drivers/char| are also ARM-specific. I assume all the
UART drivers currently using|ioremap_nocache|() are intended for ARM.

I'll drop|ioremap_nocache()| for RISC-V, and I think it makes sense to
create a separate patch to clean this up for ARM as well.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2622 bytes --]

Re: [PATCH v2 05/16] xen/asm-generic: introduce asm-generic/irq-dt.h

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]


On 5/14/25 4:36 PM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> Introduce defintions of IRQ_TYPE_* which correspond to the Xen internal
>> representation of the IRQ types and make them the same asthe existing
>> device tree defintions for convenience.
>>
>> These defines are going to be re-used, at least, by RISC-V architecture.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks!

>
> Assuming an Arm ack would arrive, this looks like it's independent of the
> earlier patches, and hence could go in right away?

Yes, it is independent.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1477 bytes --]

Re: [PATCH v2 06/16] xen/riscv: introduce init_IRQ()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]


On 5/14/25 4:49 PM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/irq.h
>> +++ b/xen/arch/riscv/include/asm/irq.h
>> @@ -3,6 +3,11 @@
>>   #define ASM__RISCV__IRQ_H
>>   
>>   #include <xen/bug.h>
>> +#include <xen/device_tree.h>
>> +
>> +#include <asm/irq-dt.h>
>> +
>> +#define NR_IRQS 1024
> Is this arbitrary or arch-induced? Imo it wants saying in a brief comment either
> way. Then again maybe it's entirely obvious for a RISC-V person ...

1024 it is number of interrupt sources for PLIC and APLIC. I will add the comment above:
/*
  * According to the AIA spec:
  *   The maximum number of interrupt sources an APLIC may support is 1023.
  *
  * The same is true for PLIC.
  *
  * Interrupt Source 0 is reserved and shall never generate an interrupt.
  */
#define NR_CPUS 1024

Probably, it makes sense to change it to 1023 as interrupt 0 isn't really used.

>
>> --- /dev/null
>> +++ b/xen/arch/riscv/irq.c
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +/*
>> + * RISC-V Trap handlers
>> + *
>> + * Copyright (c) 2024 Vates
>> + */
>> +
>> +#include <xen/bug.h>
>> +#include <xen/init.h>
>> +#include <xen/irq.h>
>> +
>> +static irq_desc_t irq_desc[NR_IRQS];
>> +
>> +int arch_init_one_irq_desc(struct irq_desc *desc)
>> +{
>> +    desc->arch.type = IRQ_TYPE_INVALID;
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init init_irq_data(void)
>> +{
>> +    unsigned int irq;
>> +
>> +    for ( irq = 0; irq < NR_IRQS; irq++ )
>> +    {
>> +        struct irq_desc *desc = irq_to_desc(irq);
>> +        int rc = init_one_irq_desc(desc);
>> +
>> +        if ( rc )
>> +            return rc;
>> +
>> +        desc->irq = irq;
>> +        desc->action  = NULL;
> Nit: You copied a stray blank from Arm code. Actually I wonder why it isn't
> init_one_irq_desc() that clears this field.

I can come up with the patch which does desc->action = NULL in init_one_irq_desc().
But considering that irq_desc[] is zero-initialized then perhaps there is no any
sense for desc->action = NULL, at all.

> It also feels like ->irq would
> better be set ahead of calling that function, like x86 has it.

But what is the difference with an order of writing a value to ->irq? It isn't
really used in init_one_irq_desc(). Or could ->irq be used in arch_init_one_irq_desc()
for something?

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3297 bytes --]

Re: [PATCH v2 06/16] xen/riscv: introduce init_IRQ()

[Jan Beulich]

On 16.05.2025 13:53, Oleksii Kurochko wrote:
> On 5/14/25 4:49 PM, Jan Beulich wrote:
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/irq.h
>>> +++ b/xen/arch/riscv/include/asm/irq.h
>>> @@ -3,6 +3,11 @@
>>>   #define ASM__RISCV__IRQ_H
>>>   
>>>   #include <xen/bug.h>
>>> +#include <xen/device_tree.h>
>>> +
>>> +#include <asm/irq-dt.h>
>>> +
>>> +#define NR_IRQS 1024
>> Is this arbitrary or arch-induced? Imo it wants saying in a brief comment either
>> way. Then again maybe it's entirely obvious for a RISC-V person ...
> 
> 1024 it is number of interrupt sources for PLIC and APLIC. I will add the comment above:
> /*
>   * According to the AIA spec:
>   *   The maximum number of interrupt sources an APLIC may support is 1023.
>   *
>   * The same is true for PLIC.
>   *
>   * Interrupt Source 0 is reserved and shall never generate an interrupt.
>   */
> #define NR_CPUS 1024

s/CPU/IRQ I suppose.

> Probably, it makes sense to change it to 1023 as interrupt 0 isn't really used.

I'd recommend against that. It would likely make things more difficult when
range-checking IRQ numbers.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/irq.c
>>> @@ -0,0 +1,45 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +
>>> +/*
>>> + * RISC-V Trap handlers
>>> + *
>>> + * Copyright (c) 2024 Vates
>>> + */
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/init.h>
>>> +#include <xen/irq.h>
>>> +
>>> +static irq_desc_t irq_desc[NR_IRQS];
>>> +
>>> +int arch_init_one_irq_desc(struct irq_desc *desc)
>>> +{
>>> +    desc->arch.type = IRQ_TYPE_INVALID;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __init init_irq_data(void)
>>> +{
>>> +    unsigned int irq;
>>> +
>>> +    for ( irq = 0; irq < NR_IRQS; irq++ )
>>> +    {
>>> +        struct irq_desc *desc = irq_to_desc(irq);
>>> +        int rc = init_one_irq_desc(desc);
>>> +
>>> +        if ( rc )
>>> +            return rc;
>>> +
>>> +        desc->irq = irq;
>>> +        desc->action  = NULL;
>> Nit: You copied a stray blank from Arm code. Actually I wonder why it isn't
>> init_one_irq_desc() that clears this field.
> 
> I can come up with the patch which does desc->action = NULL in init_one_irq_desc().
> But considering that irq_desc[] is zero-initialized then perhaps there is no any
> sense for desc->action = NULL, at all.

Oh, yes, indeed.

>> It also feels like ->irq would
>> better be set ahead of calling that function, like x86 has it.
> 
> But what is the difference with an order of writing a value to ->irq? It isn't
> really used in init_one_irq_desc().

That's the present state of things, yes. There or ...

> Or could ->irq be used in arch_init_one_irq_desc()
> for something?

... there it could be used e.g. for a log message. Maybe even just a transient
debugging one. And there's no (proper) way to re-establish the IRQ number from
the desc pointer, at least not in arch-independent code.

Jan

Re: [PATCH v2 07/16] xen/riscv: introduce platform_get_irq()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]


On 5/15/25 9:33 AM, Jan Beulich wrote:
>> +int platform_get_irq(const struct dt_device_node *device, int index)
>> +{
>> +    struct dt_irq dt_irq;
>> +    int ret;
>> +
>> +    if ( (ret = dt_device_get_irq(device, index, &dt_irq)) != 0 )
>> +        return ret;
>> +
>> +    if ( (ret = irq_set_type(dt_irq.irq, dt_irq.type)) != 0 )
>> +        return ret;
>> +
>> +    return dt_irq.irq;
> What guarantees the value to be at most INT_MAX (i.e. no silent conversion to
> a negative value, signaling an error to the caller)? Actually, looking at
> irq_set_type(), what guarantees irq_to_desc() there to not overrun irq_desc[]?
> There are no bounds checks in aplic_irq_xlate().

I'm afraid that both aren't guaranteed. I think to have the following in platform_get_irq()
should be enough:
     BUILD_BUG_ON(NR_IRQS > INT_MAX);

     if ( dt_irq.irq >= NR_IRQS )
         panic("irq%d is bigger then NR_IRQS(%d)\n", dt_irq.irq, NR_IRQS);

Probably, the first could be dropped as I'm not sure that anyone will use such big
number for NR_IRQS.

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 1549 bytes --]

Re: [PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 8553 bytes --]


On 5/15/25 9:56 AM, Jan Beulich wrote:
> (adding Bertrand as the one further DT maintainer, for a respective question
> below)
>
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> Implements dt_processor_hartid()
> There's no such function (anymore).
>
>> to get the hart ID of the given
>> device tree node and do some checks if CPU is available and given device
>> tree node has proper riscv,isa property.
>>
>> As a helper function dt_get_cpuid() is introduced to deal specifically
>> with reg propery of a CPU device node.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> Changes in V2:
>>   - s/of_get_cpu_hwid()/dt_get_cpu_id().
>>   - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>>   - Add empty line before last return in dt_get_cpu_hwid().
>>   - s/riscv_of_processor_hartid/dt_processor_cpuid().
>>   - Use pointer-to_const for node argument of dt_processor_cpuid().
>>   - Use for hart_id unsigned long type as according to the spec for RV128
>>     mhartid register will be 128 bit long.
>>   - Update commit message and subject.
>>   - use 'CPU' instead of 'HART'.
> Was this is good move? What is returned ...
>
>> --- a/xen/arch/riscv/include/asm/smp.h
>> +++ b/xen/arch/riscv/include/asm/smp.h
>> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>>   
>>   void setup_tp(unsigned int cpuid);
>>   
>> +struct dt_device_node;
>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
> ... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
> not sure it's a hart ID either, so it may need further clarification (and I'd
> expect RISC-V to have suitable terminology to tell apart the different entities).

 From device tree point of view (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt)
it is just hart which is defined as a hardware execution context, which contains all the state mandated by
the RISC-V ISA: a PC and some registers.
And also based on this for DT binding:
   For example, my Intel laptop is
   described as having one socket with two cores, each of which has two hyper
   threads.  Therefore this system has four harts.
They are using just harts and in DT it will look like 4 node with a number in reg property which they
call hart. So from DT point of view hartid is pretty fine to use.

 From specification point of view (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology):
   A RISC-V hardware platform can contain one or more RISC-V-compatible processing cores together
   with other non-RISC-V-compatible cores, fixed-function accelerators, various physical memory
   structures, I/O devices, and an interconnect structure to allow the components to communicate.

   A component is termed a core if it contains an independent instruction fetch unit. A RISC-V-
   compatible core might support multiple RISC-V-compatible hardware threads, or harts, through
   multithreading.
and (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts):
   From the perspective of software running in a given execution environment, a hart is a resource that
   autonomously fetches and executes RISC-V instructions within that execution environment. In this
   respect, a hart behaves like a hardware thread resource even if time-multiplexed onto real hardware by
   the execution environment. Some EEIs support the creation and destruction of additional harts, for
   example, via environment calls to fork new harts.

DT's CPU node should be refer to core plus hardware thread (or threads in case of multithreading)
in reg property to be close to the spec(?) but basically in DT they IMO just describe what in the sepc
is called an independent instruction fetch unit.

I still think that it is fine just to use hart_id. If to be close more to a spec the unit_id(?)
but it is more confusing IMO with what is use in RISC-V's DT binding.

>
>> @@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
>>       cpumask_set_cpu(0, &cpu_possible_map);
>>       cpumask_set_cpu(0, &cpu_online_map);
>>   }
>> +
>> +/**
>> + * dt_get_cpuid - Get the cpuid from a CPU device node
>> + *
>> + * @cpun: CPU number(logical index) for which device node is required
>> + *
>> + * Return: The cpuid for the CPU node or ~0ULL if not found.
>> + */
>> +static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
>> +{
>> +    const __be32 *cell;
>> +    int ac;
> This is bogus (should be unsigned int afaict), but dictated by ...
>
>> +    uint32_t len;
>> +
>> +    ac = dt_n_addr_cells(cpun);
> ... the return value here and ...
>
>> +    cell = dt_get_property(cpun, "reg", &len);
>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
>> +        return ~0ULL;
> (Nit: This doesn't match the return type of the function; same for
> the function comment. Also, what if sizeof(*cell) * ac < len?)
>
>> +    return dt_read_number(cell, ac);
> ... the function parameter type here.

I think that we can declare ac as unsigned int even dt_n_addr_cells
return int as basically it returns be32_to_cpu(*ip) which return unsigned
int and it isn't expected to be a big value so overflow of INT_MAX shouldn't
happen and it won't affect a size argument of dt_read_number().

> In fact, that function is raising
> another question: If the "size" argument is outside of [0, 2], the value
> returned is silently truncated.

Usually address-cells is 1 or 2, so it isn't expected to be higher (but DT tells
that the value is u32 so it could be higher then 2). And I think it could be also
explained by bitness of an architecture.
I think I see address-cells equal to 3 for something connected to PCI, but
probably another one functions should be used to read.

>
> More generally - are there any plans to make DT code signed-ness-correct?
>
>> +/*
>> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
>> + * isn't an enabled and valid RISC-V hart node.
>> + */
>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>> +{
>> +    const char *isa;
>> +
>> +    if ( !dt_device_is_compatible(node, "riscv") )
>> +    {
>> +        printk("Found incompatible CPU\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    *cpuid = dt_get_cpuid(node);
>> +    if ( *cpuid == ~0UL )
>> +    {
>> +        printk("Found CPU without CPU ID\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( !dt_device_is_available(node))
>> +    {
>> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
>> +    {
>> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>> +    {
>> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
>> +        return -ENODEV;
>> +    }
>> +
>> +    return 0;
>> +}
> I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
> messages for all of the cases, but surely there are errno values better
> representing the individual failure reasons?

I will update that to:

@@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
  int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
  {
      const char *isa;
+    int ret;
  
      if ( !dt_device_is_compatible(node, "riscv") )
      {
@@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
      if ( *cpuid == ~0UL )
      {
          printk("Found CPU without CPU ID\n");
-        return -ENODEV;
+        return -EINVAL;
      }
  
      if ( !dt_device_is_available(node))
@@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
          return -ENODEV;
      }
  
-    if ( dt_property_read_string(node, "riscv,isa", &isa) )
+    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
      {
          printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
-        return -ENODEV;
+        return ret;
      }
  
      if ( isa[0] != 'r' || isa[1] != 'v' )
      {
          printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
-        return -ENODEV;
+        return -EINVAL;
      }
  
      return 0;

I think it's better now.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 10824 bytes --]

Re: [PATCH v2 07/16] xen/riscv: introduce platform_get_irq()

[Jan Beulich]

On 16.05.2025 16:04, Oleksii Kurochko wrote:
> 
> On 5/15/25 9:33 AM, Jan Beulich wrote:
>>> +int platform_get_irq(const struct dt_device_node *device, int index)
>>> +{
>>> +    struct dt_irq dt_irq;
>>> +    int ret;
>>> +
>>> +    if ( (ret = dt_device_get_irq(device, index, &dt_irq)) != 0 )
>>> +        return ret;
>>> +
>>> +    if ( (ret = irq_set_type(dt_irq.irq, dt_irq.type)) != 0 )
>>> +        return ret;
>>> +
>>> +    return dt_irq.irq;
>> What guarantees the value to be at most INT_MAX (i.e. no silent conversion to
>> a negative value, signaling an error to the caller)? Actually, looking at
>> irq_set_type(), what guarantees irq_to_desc() there to not overrun irq_desc[]?
>> There are no bounds checks in aplic_irq_xlate().
> 
> I'm afraid that both aren't guaranteed. I think to have the following in platform_get_irq()
> should be enough:
>      BUILD_BUG_ON(NR_IRQS > INT_MAX);
> 
>      if ( dt_irq.irq >= NR_IRQS )
>          panic("irq%d is bigger then NR_IRQS(%d)\n", dt_irq.irq, NR_IRQS);
> 
> Probably, the first could be dropped as I'm not sure that anyone will use such big
> number for NR_IRQS.

I'd say better keep it, even if largely for doc purposes.

Jan

Re: [PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation

[Jan Beulich]

On 16.05.2025 18:02, Oleksii Kurochko wrote:
> 
> On 5/15/25 9:56 AM, Jan Beulich wrote:
>> (adding Bertrand as the one further DT maintainer, for a respective question
>> below)
>>
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> Implements dt_processor_hartid()
>> There's no such function (anymore).
>>
>>> to get the hart ID of the given
>>> device tree node and do some checks if CPU is available and given device
>>> tree node has proper riscv,isa property.
>>>
>>> As a helper function dt_get_cpuid() is introduced to deal specifically
>>> with reg propery of a CPU device node.
>>>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>>> ---
>>> Changes in V2:
>>>   - s/of_get_cpu_hwid()/dt_get_cpu_id().
>>>   - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>>>   - Add empty line before last return in dt_get_cpu_hwid().
>>>   - s/riscv_of_processor_hartid/dt_processor_cpuid().
>>>   - Use pointer-to_const for node argument of dt_processor_cpuid().
>>>   - Use for hart_id unsigned long type as according to the spec for RV128
>>>     mhartid register will be 128 bit long.
>>>   - Update commit message and subject.
>>>   - use 'CPU' instead of 'HART'.
>> Was this is good move? What is returned ...
>>
>>> --- a/xen/arch/riscv/include/asm/smp.h
>>> +++ b/xen/arch/riscv/include/asm/smp.h
>>> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>>>   
>>>   void setup_tp(unsigned int cpuid);
>>>   
>>> +struct dt_device_node;
>>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
>> ... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
>> not sure it's a hart ID either, so it may need further clarification (and I'd
>> expect RISC-V to have suitable terminology to tell apart the different entities).
> 
>  From device tree point of view (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt)
> it is just hart which is defined as a hardware execution context, which contains all the state mandated by
> the RISC-V ISA: a PC and some registers.
> And also based on this for DT binding:
>    For example, my Intel laptop is
>    described as having one socket with two cores, each of which has two hyper
>    threads.  Therefore this system has four harts.
> They are using just harts and in DT it will look like 4 node with a number in reg property which they
> call hart. So from DT point of view hartid is pretty fine to use.
> 
>  From specification point of view (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology):
>    A RISC-V hardware platform can contain one or more RISC-V-compatible processing cores together
>    with other non-RISC-V-compatible cores, fixed-function accelerators, various physical memory
>    structures, I/O devices, and an interconnect structure to allow the components to communicate.
> 
>    A component is termed a core if it contains an independent instruction fetch unit. A RISC-V-
>    compatible core might support multiple RISC-V-compatible hardware threads, or harts, through
>    multithreading.
> and (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts):
>    From the perspective of software running in a given execution environment, a hart is a resource that
>    autonomously fetches and executes RISC-V instructions within that execution environment. In this
>    respect, a hart behaves like a hardware thread resource even if time-multiplexed onto real hardware by
>    the execution environment. Some EEIs support the creation and destruction of additional harts, for
>    example, via environment calls to fork new harts.
> 
> DT's CPU node should be refer to core plus hardware thread (or threads in case of multithreading)
> in reg property to be close to the spec(?) but basically in DT they IMO just describe what in the sepc
> is called an independent instruction fetch unit.
> 
> I still think that it is fine just to use hart_id. If to be close more to a spec the unit_id(?)
> but it is more confusing IMO with what is use in RISC-V's DT binding.

Based on what you quoted above I think "hart ID" is indeed appropriate here.

>>> +/*
>>> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
>>> + * isn't an enabled and valid RISC-V hart node.
>>> + */
>>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>>> +{
>>> +    const char *isa;
>>> +
>>> +    if ( !dt_device_is_compatible(node, "riscv") )
>>> +    {
>>> +        printk("Found incompatible CPU\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    *cpuid = dt_get_cpuid(node);
>>> +    if ( *cpuid == ~0UL )
>>> +    {
>>> +        printk("Found CPU without CPU ID\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( !dt_device_is_available(node))
>>> +    {
>>> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
>>> +    {
>>> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>>> +    {
>>> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>> I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
>> messages for all of the cases, but surely there are errno values better
>> representing the individual failure reasons?
> 
> I will update that to:
> 
> @@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
>   int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>   {
>       const char *isa;
> +    int ret;
>   
>       if ( !dt_device_is_compatible(node, "riscv") )
>       {
> @@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>       if ( *cpuid == ~0UL )
>       {
>           printk("Found CPU without CPU ID\n");
> -        return -ENODEV;
> +        return -EINVAL;

EINVAL is the most commonly used error code; I'd therefore recommend to
avoid its use unless it is indeed properly describing the situation
(invalid argument, i.e. invalid value _passed in_). Here ENODATA might
be a suitable replacement, for example.

Jan

>       }
>   
>       if ( !dt_device_is_available(node))
> @@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>           return -ENODEV;
>       }
>   
> -    if ( dt_property_read_string(node, "riscv,isa", &isa) )
> +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
>       {
>           printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
> -        return -ENODEV;
> +        return ret;
>       }
>   
>       if ( isa[0] != 'r' || isa[1] != 'v' )
>       {
>           printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
> -        return -ENODEV;
> +        return -EINVAL;
>       }
>   
>       return 0;
> 
> I think it's better now.
> 
> Thanks.
> 
> ~ Oleksii
> 

Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]


On 5/15/25 10:06 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/intc.h
>> +++ b/xen/arch/riscv/include/asm/intc.h
>> @@ -8,6 +8,8 @@
>>   #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>>   #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>>   
>> +#include <xen/irq.h>
> If you need this include anyway, why ...
>
>> @@ -17,6 +19,26 @@ struct intc_info {
>>       const struct dt_device_node *node;
>>   };
>>   
>> +struct irq_desc;
> ... this "forward" decl for something that's then already fully defined?
> I can't, however, spot why xen/irq.h would be needed for anything ...

forward decl for irq_desc could be really dropped.

Inclusion of xen/irq.h was added because of hw_irq_controller which is defined as:
   typedef const struct hw_interrupt_type hw_irq_controller;

And I'm not sure how to do forward declaration properly in this case. Just use
an explicit definition of hw_irq_controller for host_irq_type member of struct
intc_hw_operations seems as not the best one option:
   struct hw_interrupt_type;
  
   struct intc_hw_operations {
       ...
       // const hw_irq_controller *host_irq_type;
       const struct hw_interrupt_type *host_irq_type;

It seems like the best one option is to do the following:
   typedef const struct hw_interrupt_type hw_irq_controller; in asm/intc.h.
I will follow it then.

>> --- a/xen/arch/riscv/intc.c
>> +++ b/xen/arch/riscv/intc.c
>> @@ -5,6 +5,15 @@
>>   #include <xen/init.h>
>>   #include <xen/lib.h>
>>   
>> +#include <asm/intc.h>
>> +
>> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
> Nit: Attributes between type and identifier please. Also shouldn't both
> this and ...
>
>> +void __init register_intc_ops(struct intc_hw_operations *ops)
> ... the parameter here be pointer-to-const?

Then|intc_hw_ops| should also be marked as|const| (with the removal of|__ro_after_init|),
otherwise a compilation error will occur (something like/"assignment discards 'const' qualifier"/).

Additionally,|__ro_after_init| should be replaced with|const| for|aplic_ops| in future
patches.

Let me know which approach you prefer. I prefer using|const|, as
|intc_hw_operations| and|aplic_ops| are not expected to change after initialization.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3931 bytes --]

Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Jan Beulich]

On 19.05.2025 11:16, Oleksii Kurochko wrote:
> 
> On 5/15/25 10:06 AM, Jan Beulich wrote:
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/intc.h
>>> +++ b/xen/arch/riscv/include/asm/intc.h
>>> @@ -8,6 +8,8 @@
>>>   #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>   #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>   
>>> +#include <xen/irq.h>
>> If you need this include anyway, why ...
>>
>>> @@ -17,6 +19,26 @@ struct intc_info {
>>>       const struct dt_device_node *node;
>>>   };
>>>   
>>> +struct irq_desc;
>> ... this "forward" decl for something that's then already fully defined?
>> I can't, however, spot why xen/irq.h would be needed for anything ...
> 
> forward decl for irq_desc could be really dropped.
> 
> Inclusion of xen/irq.h was added because of hw_irq_controller which is defined as:
>    typedef const struct hw_interrupt_type hw_irq_controller;
> 
> And I'm not sure how to do forward declaration properly in this case. Just use
> an explicit definition of hw_irq_controller for host_irq_type member of struct
> intc_hw_operations seems as not the best one option:
>    struct hw_interrupt_type;

This isn't needed for the use below.

>    struct intc_hw_operations {
>        ...
>        // const hw_irq_controller *host_irq_type;
>        const struct hw_interrupt_type *host_irq_type;

It might be that something like this is already done elsewhere in the tree,
so not really an issue imo if a 2nd instance appeared.

> It seems like the best one option is to do the following:
>    typedef const struct hw_interrupt_type hw_irq_controller; in asm/intc.h.
> I will follow it then.

Misra may dislike this.

>>> --- a/xen/arch/riscv/intc.c
>>> +++ b/xen/arch/riscv/intc.c
>>> @@ -5,6 +5,15 @@
>>>   #include <xen/init.h>
>>>   #include <xen/lib.h>
>>>   
>>> +#include <asm/intc.h>
>>> +
>>> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
>> Nit: Attributes between type and identifier please. Also shouldn't both
>> this and ...
>>
>>> +void __init register_intc_ops(struct intc_hw_operations *ops)
>> ... the parameter here be pointer-to-const?
> 
> Then|intc_hw_ops| should also be marked as|const| (with the removal of|__ro_after_init|),

Why remove the attribute?

> otherwise a compilation error will occur (something like/"assignment discards 'const' qualifier"/).
> 
> Additionally,|__ro_after_init| should be replaced with|const| for|aplic_ops| in future
> patches.

Same question here then.

Jan

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 8080 bytes --]


On 5/15/25 10:42 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, 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_cpuid() 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 V2:
>>   - Drop years in copyrights.
> Did you, really?

Oops, missed to drop it in asm/imsic.h. Thanks.

>> +    if ( rc )
>> +        return rc;
>> +
>> +    nr_mmios = imsic_cfg.nr_mmios;
>> +
>> +    /* Allocate MMIO resource array */
>> +    imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios);
>> +    if ( !imsic_cfg.mmios )
>> +        return -ENOMEM;
>> +
>> +    imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs);
>> +    if ( !imsic_cfg.msi )
>> +        return -ENOMEM;
> Leaking the earlier successful allocation?
>
>> +    /* Check MMIO register sets */
>> +    for ( unsigned int i = 0; i < nr_mmios; i++ )
>> +    {
>> +        imsic_cfg.mmios[i].cpus = xzalloc_array(unsigned long,
>> +                                                BITS_TO_LONGS(nr_parent_irqs));
>> +        if ( !imsic_cfg.mmios[i].cpus )
>> +            return -ENOMEM;
> Leaking all earlier successful allocations?

In this cases should be:
     {
         rc = -ENOMEM;
         goto imsic_init_err;

>> +        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 ( unsigned int i = 0; i < nr_parent_irqs; i++ )
>> +    {
>> +        rc = imsic_get_parent_cpuid(node, i, &cpuid);
> Coming back to a comment I gave on the respective earlier patch: What you get back
> here is a DT value, aiui. There's no translation to Xen CPU numbering space, as
> would be required for e.g. ...
>
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_WARNING "%s: cpu ID for parent irq%d not found\n",
>> +                   node->name, i);
>> +            continue;
>> +        }
>> +
>> +        if ( cpuid >= num_possible_cpus() )
> ... this. Are you using DT numbering without any translation, no matter that it
> (I suppose) could be very sparse?

Agree, it should translation of cpuid to Xen CPU numbering space as cpuid could be
sparsed according to the RISC-V spec, which guarantees only that cpuid 0 will be present,
all other could be any number.

I thought about to update that with:
    xen_cpuid = hartid_to_cpuid(hartid);

    if ( xen_cpuid >= num_possible_cpus() )
    {
         printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent irq%u\n",
                node->name, hartid, i);
         continue;
    }
       ...

   /* defined in asm/smp.h */
   static inline unsigned long hartid_to_cpuid(unsigned long hartid)
   {
     for ( unsigned int cpuid = 0; cpuid < ARRAY_SIZE(pcpu_info); cpuid++ )
     {
         if ( hartid == cpuid_to_hartid(cpuid) )
             return cpuid;
     }

     return NR_CPUS;
   }
   
(the proper name of variable 'cpuid' I think we will agree in the discussion of one of the
earlier patches.)

But then it will be an issue that if hart_id (from DT) hasn't been assigned to Xen's cpuid,
then IMSIC's msi[] and mmio[] won't be configured properly.
Probably this is not an issue and this assignment of cpuid to hartid could be done before
imsic_init() in case of CONFIG_SMP=y.

>> +            continue;
>> +        }
>> +
>> +        /* Find MMIO location of MSI page */
>> +        index = nr_mmios;
>> +        reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ;
>> +        for ( unsigned int j = 0; nr_mmios; j++ )
> DYM "j < nr_mmios"?

Yes, the condition should be corrected. Interesting why I am not faced an issue with such
condition, nr_mmios shouldn't be zero (I'll double check) and Linux kernel has the same
condition:
   https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-riscv-imsic-state.c#L907C31-L908C1
It seems like LK wants to have a fix too.

>> +        {
>> +            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 >= nr_mmios )
> Why is it that you need both "index" and "j"?

There is no need. I will drop 'j'.

>> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
>> +            imsic_cfg.msi[cpuid].offset = 0;
>> +            imsic_cfg.msi[cpuid].base_addr = 0;
>> +            continue;
>> +        }
>> +
>> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);
> Depending on clarification on the number space used, this may want to be
> cpumask_set_cpu() instead. Else I think this is simply __set_bit().

Considering that cpuid is taken from DT and the value in device tree is what we are
using as hartid and hartid could be according any number from 0 to sizeof(unsigned long)
then we can't use cpuid for msi[] as overflow could happen, it seems like we should use
an id from Xen space. (so basically xen_cpuid which I mentioned above)

>
>> +        imsic_cfg.msi[cpuid].base_addr = imsic_cfg.mmios[index].base_addr;
>> +        imsic_cfg.msi[cpuid].offset = reloff;
> How come it's cpuid that's used as array index here? Shouldn't this be i,
> seeing that the array allocation is using nr_parent_irqs?

Agree, something wrong is here. As I mentioned above, I think, Xen cpu space should be used here.

>> +    XFREE(imsic_cfg.msi);
>> +
>> +    return rc;
>> +}
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/imsic.h
>> @@ -0,0 +1,65 @@
>> +/* 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
> This isn't the "minimum ID", but the "minimum permissible number of IDs" as per
> its first use in imsic_parse_node(). Further uses there consider it a mask,
> which makes me wonder whether the imsic_cfg.nr_ids field name is actually
> correct: Isn't this the maximum valid ID rather than their count/number?

imsic_cfg.nr_ids it is a value of interrupt identities supported by IMSIC interrupt file according to
DT binding:
   riscv,num-ids:
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 63
     maximum: 2047
     description:
       Number of interrupt identities supported by IMSIC interrupt file.

 From some point of view it could be called as maximum valid ID specifically for a platform, but the number
of ids looks better to me.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 11118 bytes --]

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]


On 5/15/25 10:42 AM, Jan Beulich wrote:
>> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
>> +            imsic_cfg.msi[cpuid].offset = 0;
>> +            imsic_cfg.msi[cpuid].base_addr = 0;
>> +            continue;
>> +        }
>> +
>> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);
> Depending on clarification on the number space used, this may want to be
> cpumask_set_cpu() instead. Else I think this is simply __set_bit().

cpumask_set_cpu() requires cpumask_t which uses static definition but we are
using dynamic allocation.
So it seems like bitmap_set() is the  best one option or reworking to static
allocation will require.
Am I missing something?

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 1264 bytes --]

Re: [PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 4742 bytes --]


On 5/15/25 11:06 AM, Jan Beulich wrote:
>> --- a/xen/arch/riscv/aplic.c
>> +++ b/xen/arch/riscv/aplic.c
>> @@ -9,19 +9,121 @@
>>    * 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"
> Besides this, are there going to be any other files including this private
> header? If not, why have the header in the first place?

Yes, structure aplic_priv is going to be reused in vaplic.c (which isn't introduce yet):
   https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/vaplic.c#L56

>> +    /* Set interrupt type and default priority for all interrupts */
>> +    for ( i = 1; i <= aplic_info.num_irqs; i++ )
>> +    {
>> +        writel(0, &aplic.regs->sourcecfg[i - 1]);
> What guarantees the loop to not run past the array's size?

riscv,aplic binding:
   https://github.com/torvalds/linux/blob/a5806cd506af5a7c19bcd596e4708b5c464bfd21/Documentation/devicetree/bindings/interrupt-controller/riscv%2Caplic.yaml#L57
Should I add an ASSERT() or panic() at the moment where num_irqs are
initialized to double check a binding?

>
>> +        /*
>> +         * 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 - 1]);
>> +    }
> Seeing the subtractions of 1 here, why's the loop header not simply
>
>      for ( i = 0; i < aplic_info.num_irqs; i++ )
>
> (i.e. the more conventional form)?

To follow the definition of aplic's binding mentioned about where minimum is 1.
But I think we can use convention form.

>
>> +    writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
>> +}
>> +
>> +static int __init cf_check aplic_init(void)
>> +{
>> +    int rc;
>> +    dt_phandle imsic_phandle;
>> +    uint32_t irq_range[num_possible_cpus() * 2];
> Are you going to have enough stack space when num_possible_cpus() is pretty
> large?

When num_possible_cpus() will be pretty large then it will better to allocate irq_range[]
dynamically.

Does it make sense to re-write now?

>> +    const struct dt_device_node *node = aplic_info.node;
>> +
>> +    /* Check for associated imsic node */
>> +    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
>> +    if ( !rc )
>> +        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 = dt_property_read_u32_array(imsic_node, "interrupts-extended",
>> +                                    irq_range, ARRAY_SIZE(irq_range));
>> +    if ( rc )
>> +        panic("%s: unable to find interrupt-extended in %s node\n",
>> +              __func__, imsic_node->full_name);
>> +
>> +    if ( irq_range[1] == IRQ_M_EXT )
> How do you know the array has had 2 or ...
>
>> +        /* Machine mode imsic node, ignore this aplic node */
>> +        return 0;
>> +
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
>> +    {
>> +        if ( irq_range[i + 1] != irq_range[1] )
>> +            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
>> +    }
> ... or even all of the slots populated? Anything not populated by the DT
> function will still have the stack contents that had been left by earlier
> call chains. (The loop also makes little sense to start from 0.)

Do you mean I asked allocated irq_range[8*2] but DT node has only irq_range[4*2]?
Then it will be really an issue. And out-of-range access will occur in
dt_property_read_variable_u32_array(). I need another way to check then...

>
> I'm also puzzled by there not being any further use of the values later
> in the function.

Yes, because we need this values only to check that DT node's property is
correctly created.

>
>> +    rc = imsic_init(imsic_node);
>> +    if ( rc )
>> +        panic("%s: Failded to initialize IMSIC\n", node->full_name);
>> +
>> +    /* Find out number of interrupt sources */
>> +    rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.num_irqs);
>> +    if ( !rc )
> Assigning a bool return value to an int local var, which generally hold
> error codes, is confusing. I don't think you really need to use a local
> variable here.

Considering that I am panicing for all the case where rc is used and rc isn't printed
then we can really just drop rc.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 7258 bytes --]

Re: [PATCH v2 04/16] xen/riscv: add ioremap_*() variants using ioremap_attr()

[Teddy Astie]

Hello,

Le 06/05/2025 à 18:53, Oleksii Kurochko a écrit :
> Introduce ioremap_attr() as a shared helper to implement architecture-specific
> +#include <xen/vmap.h>
>
>   #include <asm/early_printk.h>
>   #include <asm/csr.h>
> @@ -583,3 +584,36 @@ void *__init arch_vmap_virt_end(void)
>   {
>       return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>   }
> +
> +static void *ioremap_attr(paddr_t start, size_t len, pte_attr_t attributes)

you probably want __iomem here

> +{
> +    mfn_t mfn = _mfn(PFN_DOWN(start));
> +    unsigned int offs = start & (PAGE_SIZE - 1);
> +    unsigned int nr = PFN_UP(offs + len);
> +    void *ptr = __vmap(&mfn, nr, 1, 1, attributes, VMAP_DEFAULT);
> +
> +    if ( ptr == NULL )
> +        return NULL;
> +
> +    return ptr + offs;
> +}
> +
> +void __iomem *ioremap_nocache(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_NOCACHE);
> +}
> +
> +void __iomem *ioremap_cache(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR);
> +}
> +
> +void __iomem *ioremap_wc(paddr_t start, size_t len)
> +{
> +    return ioremap_attr(start, len, PAGE_HYPERVISOR_WC);
> +}
> +
> +void *ioremap(paddr_t pa, size_t len)

also here

> +{
> +    return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
> +}

Teddy


Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Jan Beulich]

On 19.05.2025 17:19, Oleksii Kurochko wrote:
> On 5/15/25 10:42 AM, Jan Beulich wrote:
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/imsic.h
>>> @@ -0,0 +1,65 @@
>>> +/* 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
>> This isn't the "minimum ID", but the "minimum permissible number of IDs" as per
>> its first use in imsic_parse_node(). Further uses there consider it a mask,
>> which makes me wonder whether the imsic_cfg.nr_ids field name is actually
>> correct: Isn't this the maximum valid ID rather than their count/number?
> 
> imsic_cfg.nr_ids it is a value of interrupt identities supported by IMSIC interrupt file according to
> DT binding:
>    riscv,num-ids:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      minimum: 63
>      maximum: 2047
>      description:
>        Number of interrupt identities supported by IMSIC interrupt file.

Unless this count accounts for 0 being invalid (and hence isn't counted), I'm
still confused: With the maximum value this would mean 2046 is still valid,
but 2047 isn't anymore. When normally such a boundary would be at an exact
power of 2, i.e. between 2047 and 2048 here.

Jan

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Jan Beulich]

On 19.05.2025 17:26, Oleksii Kurochko wrote:
> On 5/15/25 10:42 AM, Jan Beulich wrote:
>>> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
>>> +            imsic_cfg.msi[cpuid].offset = 0;
>>> +            imsic_cfg.msi[cpuid].base_addr = 0;
>>> +            continue;
>>> +        }
>>> +
>>> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);
>> Depending on clarification on the number space used, this may want to be
>> cpumask_set_cpu() instead. Else I think this is simply __set_bit().
> 
> cpumask_set_cpu() requires cpumask_t which uses static definition but we are
> using dynamic allocation.

But you're aware of cpumask_var_t (and respective allocation functions)?

Jan

> So it seems like bitmap_set() is the  best one option or reworking to static
> allocation will require.
> Am I missing something?
> 
> ~ Oleksii
> 
> 

Re: [PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Jan Beulich]

On 19.05.2025 18:09, Oleksii Kurochko wrote:
> On 5/15/25 11:06 AM, Jan Beulich wrote:
>>> +    /* Set interrupt type and default priority for all interrupts */
>>> +    for ( i = 1; i <= aplic_info.num_irqs; i++ )
>>> +    {
>>> +        writel(0, &aplic.regs->sourcecfg[i - 1]);
>> What guarantees the loop to not run past the array's size?
> 
> riscv,aplic binding:
>    https://github.com/torvalds/linux/blob/a5806cd506af5a7c19bcd596e4708b5c464bfd21/Documentation/devicetree/bindings/interrupt-controller/riscv%2Caplic.yaml#L57
> Should I add an ASSERT() or panic() at the moment where num_irqs are
> initialized to double check a binding?

I may be paranoid, but I don't really trust anything coming from DT. Hence
yes, just like you do in various other situations, perhaps best to panic()
if too large a value is read. Or, if that's feasible, simply cap at the
compiled-in count.

>>> +static int __init cf_check aplic_init(void)
>>> +{
>>> +    int rc;
>>> +    dt_phandle imsic_phandle;
>>> +    uint32_t irq_range[num_possible_cpus() * 2];
>> Are you going to have enough stack space when num_possible_cpus() is pretty
>> large?
> 
> When num_possible_cpus() will be pretty large then it will better to allocate irq_range[]
> dynamically.
> 
> Does it make sense to re-write now?

Well, this kind of runtime-sized stack allocation should go away anyway.
Plus you don't want to leave a trap like this in the code. Whether using
dynamic allocation is the choice to address those concerns you'll need
to judge.

>>> +    const struct dt_device_node *node = aplic_info.node;
>>> +
>>> +    /* Check for associated imsic node */
>>> +    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
>>> +    if ( !rc )
>>> +        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 = dt_property_read_u32_array(imsic_node, "interrupts-extended",
>>> +                                    irq_range, ARRAY_SIZE(irq_range));
>>> +    if ( rc )
>>> +        panic("%s: unable to find interrupt-extended in %s node\n",
>>> +              __func__, imsic_node->full_name);
>>> +
>>> +    if ( irq_range[1] == IRQ_M_EXT )
>> How do you know the array has had 2 or ...
>>
>>> +        /* Machine mode imsic node, ignore this aplic node */
>>> +        return 0;
>>> +
>>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
>>> +    {
>>> +        if ( irq_range[i + 1] != irq_range[1] )
>>> +            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
>>> +    }
>> ... or even all of the slots populated? Anything not populated by the DT
>> function will still have the stack contents that had been left by earlier
>> call chains. (The loop also makes little sense to start from 0.)
> 
> Do you mean I asked allocated irq_range[8*2] but DT node has only irq_range[4*2]?
> Then it will be really an issue. And out-of-range access will occur in
> dt_property_read_variable_u32_array(). I need another way to check then...

I described the opposite situation (not the full array being populated),
but yes - both are a problem.

Jan

Re: [PATCH v2 12/16] xen/riscv: introduce intc_init() and helpers

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3852 bytes --]


On 5/15/25 11:29 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> 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>
>> ---
>> 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             | 45 +++++++++++++++++++++++++++++++
>>   xen/arch/riscv/setup.c            |  2 ++
>>   3 files changed, 51 insertions(+)
>>
>> diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
>> index 2d55d74a2e..45a41147a6 100644
>> --- a/xen/arch/riscv/include/asm/intc.h
>> +++ b/xen/arch/riscv/include/asm/intc.h
>> @@ -44,4 +44,8 @@ void intc_preinit(void);
>>   
>>   void register_intc_ops(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 122e7b32b5..15f358601d 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,45 @@ void __init intc_preinit(void)
>>       else
>>           panic("ACPI interrupt controller preinit() isn't implemented\n");
>>   }
>> +
>> +void __init intc_init(void)
>> +{
>> +    ASSERT(intc_hw_ops);
> What's the goal of this check (also elsewhere below)? You'll crash anyway ...

Agree, that it could be dropped.

The goal was that it will a little bit easier to find a place where NULL
pointer de-reference  will/could happen.

Then it make sense to 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.

>
>> +    if ( intc_hw_ops->init() )
> ... here if the point is still NULL, just like you will if the function
> pointer is unpopulated (and hence NULL).

Here, I think it would be better to rewrite to:
   void __init intc_init(void)
   {
       if ( !intc_hw_ops->init )
           return;

       if ( intc_hw_ops->init() )
           panic("Failed to initialize the interrupt controller drivers\n");
   }
For the case, if an interrupt controller doesn't want to do some initialization.
(but I am not sure if it makes sense, likely an interrupt controller will want
to do some initialization. Then it makes do drop the first if (...)).

>
> Preferably with all of these (but not the other assertions) dropped
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

~ Oleksii



[-- Attachment #2: Type: text/html, Size: 4957 bytes --]

Re: [PATCH v2 13/16] xen/riscv: implementation of aplic and imsic operations

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3239 bytes --]


On 5/15/25 11:44 AM, Jan Beulich wrote:
>> @@ -159,6 +270,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
>>   
>>       dt_irq_xlate = aplic_irq_xlate;
>>   
>> +    spin_lock_init(&aplic.lock);
> Can't you have the struct field have a suitable initializer?

Sure, I will use struct initializer:
   static struct aplic_priv aplic = {
       .lock = SPIN_LOCK_UNLOCKED,
   };

>> +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 = (last_id - id < chunk) ? 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)
>> +{
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&imsic_cfg.lock, flags);
>> +    /*
>> +     * 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. (l)
> What's this 'l' in parentheses here to indicate?

I don't really remember, it seems like I want to point to the spec, but
then just make a quote from the spec instead. I'll just drop it.

>> +     */
>> +    imsic_local_eix_update(irq, 1, false, true);
>> +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
>> +}
>> +
>> +void imsic_irq_disable(unsigned int irq)
>> +{
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&imsic_cfg.lock, flags);
>> +    imsic_local_eix_update(irq, 1, false, false);
>> +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
>> +}
> The sole caller of the function has doubly turned off IRQs already; perhaps
> no need to it a 3rd time, unless other callers are to appear? Same for
> imsic_irq_enable() as it looks.

I checked a code in private branches and it seems like these functions are called
only in aplic_irq_{enable,disable}(), so we could do, at least,spin_lock(&imsic_cfg.lock)
+ ASSERT(!local_irq_is_enabled());

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 4091 bytes --]

Re: [PATCH v2 14/16] xen/riscv: add external interrupt handling for hypervisor mode

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]


On 5/15/25 11:54 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> +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;
>> +
>> +    spin_lock(&aplic.lock);
>> +
>> +    switch(type)
> Nit: style
>
>> +    {
>> +    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:
>> +        fallthrough;
> This is pointless (and hampering readability) when there are no other statements.

Oh, okay, it should be:
   case IRQ_TYPE_NONE:
   case IRQ_TYPE_INVALID:
I thought fallthrough should be used always in such cases.
Anyway, I'll drop it.

>
> With both taken care of:
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

I am going also to add "ASSERT(spin_is_locked(&desc->lock));" at the start of this
function to be algined with other callbacks which uses spin_{un}lock(&aplic.lock).

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2909 bytes --]

Re: [PATCH v2 15/16] xen/riscv: implement setup_irq()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]


On 5/15/25 11:57 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> @@ -58,6 +59,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
> Didn't you indicate you'd drop this?

To one of replies I wrote that probably something (eg RISC-V's IOMMU) will want to setup
multiple handler for the same interrupt. But I'm not sure yet. I can drop for now and
return back when it will be really a use case.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1684 bytes --]

Re: [PATCH v2 16/16] xen/riscv: add basic UART support

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]


On 5/15/25 11:59 AM, Jan Beulich wrote:
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/setup.c
>> +++ b/xen/arch/riscv/setup.c
>> @@ -4,12 +4,16 @@
>>   #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/percpu.h>
> Why's this needed? I can't spot anything ...

It should be dropped. This rudiment left when I called percpu_init_areas().

>
>> +#include <xen/serial.h>
>>   #include <xen/shutdown.h>
>>   #include <xen/smp.h>
>> +#include <xen/timer.h>
>>   #include <xen/vmap.h>
>>   #include <xen/xvmalloc.h>
>>   
>> @@ -136,8 +140,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();
> ... relevant here. With the need clarified or with the #include dropped:
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2124 bytes --]

Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3126 bytes --]


On 5/19/25 3:16 PM, Jan Beulich wrote:
> On 19.05.2025 11:16, Oleksii Kurochko wrote:
>> On 5/15/25 10:06 AM, Jan Beulich wrote:
>>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/include/asm/intc.h
>>>> +++ b/xen/arch/riscv/include/asm/intc.h
>>>> @@ -8,6 +8,8 @@
>>>>    #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>    #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>    
>>>> +#include <xen/irq.h>
>>> If you need this include anyway, why ...
>>>
>>>> @@ -17,6 +19,26 @@ struct intc_info {
>>>>        const struct dt_device_node *node;
>>>>    };
>>>>    
>>>> +struct irq_desc;
>>> ... this "forward" decl for something that's then already fully defined?
>>> I can't, however, spot why xen/irq.h would be needed for anything ...
>> forward decl for irq_desc could be really dropped.
>>
>> Inclusion of xen/irq.h was added because of hw_irq_controller which is defined as:
>>     typedef const struct hw_interrupt_type hw_irq_controller;
>>
>> And I'm not sure how to do forward declaration properly in this case. Just use
>> an explicit definition of hw_irq_controller for host_irq_type member of struct
>> intc_hw_operations seems as not the best one option:
>>     struct hw_interrupt_type;
> This isn't needed for the use below.

Shouldn't I tell the compiler that definition of hw_interrupt_type struct exist
somewhere else?

>
>>     struct intc_hw_operations {
>>         ...
>>         // const hw_irq_controller *host_irq_type;
>>         const struct hw_interrupt_type *host_irq_type;
> It might be that something like this is already done elsewhere in the tree,
> so not really an issue imo if a 2nd instance appeared.

It is really happing for several places in x86 code.

>
>> It seems like the best one option is to do the following:
>>     typedef const struct hw_interrupt_type hw_irq_controller; in asm/intc.h.
>> I will follow it then.
> Misra may dislike this.

Then this is not an option. I will use then the option above.

>
>>>> --- a/xen/arch/riscv/intc.c
>>>> +++ b/xen/arch/riscv/intc.c
>>>> @@ -5,6 +5,15 @@
>>>>    #include <xen/init.h>
>>>>    #include <xen/lib.h>
>>>>    
>>>> +#include <asm/intc.h>
>>>> +
>>>> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
>>> Nit: Attributes between type and identifier please. Also shouldn't both
>>> this and ...
>>>
>>>> +void __init register_intc_ops(struct intc_hw_operations *ops)
>>> ... the parameter here be pointer-to-const?
>> Then|intc_hw_ops| should also be marked as|const| (with the removal of|__ro_after_init|),
> Why remove the attribute?

My understanding is that if it is marked as 'const' then it automatically mean that it is read-only
always before and after init, so '__ro_after_init' is useless.

>
>> otherwise a compilation error will occur (something like/"assignment discards 'const' qualifier"/).
>>
>> Additionally,|__ro_after_init| should be replaced with|const| for|aplic_ops| in future
>> patches.
> Same question here then.

Just wanted to be in sync. If I have intc_hw_ops marked as const, then the thing which will be used
to set intc_hw_ops wants to be also const.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 5604 bytes --]

Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Jan Beulich]

On 20.05.2025 16:04, Oleksii Kurochko wrote:
> On 5/19/25 3:16 PM, Jan Beulich wrote:
>> On 19.05.2025 11:16, Oleksii Kurochko wrote:
>>> On 5/15/25 10:06 AM, Jan Beulich wrote:
>>>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/include/asm/intc.h
>>>>> +++ b/xen/arch/riscv/include/asm/intc.h
>>>>> @@ -8,6 +8,8 @@
>>>>>    #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>>    #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>>    
>>>>> +#include <xen/irq.h>
>>>> If you need this include anyway, why ...
>>>>
>>>>> @@ -17,6 +19,26 @@ struct intc_info {
>>>>>        const struct dt_device_node *node;
>>>>>    };
>>>>>    
>>>>> +struct irq_desc;
>>>> ... this "forward" decl for something that's then already fully defined?
>>>> I can't, however, spot why xen/irq.h would be needed for anything ...
>>> forward decl for irq_desc could be really dropped.
>>>
>>> Inclusion of xen/irq.h was added because of hw_irq_controller which is defined as:
>>>     typedef const struct hw_interrupt_type hw_irq_controller;
>>>
>>> And I'm not sure how to do forward declaration properly in this case. Just use
>>> an explicit definition of hw_irq_controller for host_irq_type member of struct
>>> intc_hw_operations seems as not the best one option:
>>>     struct hw_interrupt_type;
>> This isn't needed for the use below.
> 
> Shouldn't I tell the compiler that definition of hw_interrupt_type struct exist
> somewhere else?

The above decl merely introduces the type into (global) scope. The same is
achieved by ...

>>>     struct intc_hw_operations {
>>>         ...
>>>         // const hw_irq_controller *host_irq_type;
>>>         const struct hw_interrupt_type *host_irq_type;

... this. The case where the earlier decl matters is when a type is used
as a function parameter in a prototype. There, if not previously introduced
into global scope, the scope would be limited to that of the function decl
(and hence a type conflict would result when later the same type is
introduced into global scope).

>>>>> --- a/xen/arch/riscv/intc.c
>>>>> +++ b/xen/arch/riscv/intc.c
>>>>> @@ -5,6 +5,15 @@
>>>>>    #include <xen/init.h>
>>>>>    #include <xen/lib.h>
>>>>>    
>>>>> +#include <asm/intc.h>
>>>>> +
>>>>> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
>>>> Nit: Attributes between type and identifier please. Also shouldn't both
>>>> this and ...
>>>>
>>>>> +void __init register_intc_ops(struct intc_hw_operations *ops)
>>>> ... the parameter here be pointer-to-const?
>>> Then|intc_hw_ops| should also be marked as|const| (with the removal of|__ro_after_init|),
>> Why remove the attribute?
> 
> My understanding is that if it is marked as 'const' then it automatically mean that it is read-only
> always before and after init, so '__ro_after_init' is useless.

You need to separate properties of the (pointer type) variable, and what
that pointer points to. __ro_after_init applies to the variable, whereas
the const asked for is to apply to the pointed-to type. (This is more
obvious when you place the attribute as requested. Hence why we want that
particular placement.)

Jan

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]


On 5/19/25 8:32 PM, Jan Beulich wrote:
> On 19.05.2025 17:19, Oleksii Kurochko wrote:
>> On 5/15/25 10:42 AM, Jan Beulich wrote:
>>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/include/asm/imsic.h
>>>> @@ -0,0 +1,65 @@
>>>> +/* 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
>>> This isn't the "minimum ID", but the "minimum permissible number of IDs" as per
>>> its first use in imsic_parse_node(). Further uses there consider it a mask,
>>> which makes me wonder whether the imsic_cfg.nr_ids field name is actually
>>> correct: Isn't this the maximum valid ID rather than their count/number?
>> imsic_cfg.nr_ids it is a value of interrupt identities supported by IMSIC interrupt file according to
>> DT binding:
>>     riscv,num-ids:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       minimum: 63
>>       maximum: 2047
>>       description:
>>         Number of interrupt identities supported by IMSIC interrupt file.
> Unless this count accounts for 0 being invalid (and hence isn't counted), I'm
> still confused: With the maximum value this would mean 2046 is still valid,
> but 2047 isn't anymore. When normally such a boundary would be at an exact
> power of 2, i.e. between 2047 and 2048 here.

2047 is inclusive according to the AIA spec:
   The number of interrupt identities supported by an interrupt file
   (and hence the number of active bits in each array) is one less than a multiple
   of 64, and may be a minimum of 63 and a maximum of 2047.
   ...
   When an interrupt file supports N distinct interrupt identities,
   valid identity numbers are between 1 and N 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. Identity 0 is just ignored.

It is still  not a power of two but it was the AIA spec tells us.

Also, this maximum identity number of 2047 is consistent with related fields like
the EIID (External Interrupt Identity) field used in APLICs when forwarding MSIs,
which specifies the MSI data value that becomes the minor identity at the target
hart's interrupt file.
The EIID field is typically an 11-bit field, able to hold values from 0 through
2047.
Since identity 0 is invalid, the entire range of valid identity numbers (1-2047)
fits within the values representable by an 11-bit field.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3574 bytes --]

Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]


On 5/19/25 8:33 PM, Jan Beulich wrote:
> On 19.05.2025 17:26, Oleksii Kurochko wrote:
>> On 5/15/25 10:42 AM, Jan Beulich wrote:
>>>> +                   node->name, imsic_cfg.msi[cpuid].base_addr + reloff);
>>>> +            imsic_cfg.msi[cpuid].offset = 0;
>>>> +            imsic_cfg.msi[cpuid].base_addr = 0;
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);
>>> Depending on clarification on the number space used, this may want to be
>>> cpumask_set_cpu() instead. Else I think this is simply __set_bit().
>> cpumask_set_cpu() requires cpumask_t which uses static definition but we are
>> using dynamic allocation.
> But you're aware of cpumask_var_t (and respective allocation functions)?

Now yes, thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1541 bytes --]