[PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Oleksii Kurochko]

preinit_xen_time() does two things:
1. Parse timebase-frequency properpy of /cpus node to initialize
   cpu_khz variable.
2. Initialize xen_start_clock_cycles with the current time counter
   value.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Update SPDX tag for time.c
 - s/read_mostly/__ro_after_init for boot_count variable.
 - Add declaration of boot_count to asm/time.h.
 - Rename boot_count to xen_start_clock_cycles.
 - Stray double blank in the defintion of timer_ids.
 - s/u32/uint32_t.
 - Drop full stop.
 - Update the commit message.
 - s/__initconst/__initconstrel for timer_ids[].
---
 xen/arch/riscv/Makefile           |  1 +
 xen/arch/riscv/include/asm/time.h |  5 ++++
 xen/arch/riscv/setup.c            |  2 ++
 xen/arch/riscv/stubs.c            |  2 --
 xen/arch/riscv/time.c             | 39 +++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/time.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index b0c8270a99..82016a957a 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -9,6 +9,7 @@ obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smp.o
 obj-y += stubs.o
+obj-y += time.o
 obj-y += traps.o
 obj-y += vm_event.o
 
diff --git a/xen/arch/riscv/include/asm/time.h b/xen/arch/riscv/include/asm/time.h
index fc1572e9b4..c24745508b 100644
--- a/xen/arch/riscv/include/asm/time.h
+++ b/xen/arch/riscv/include/asm/time.h
@@ -5,6 +5,9 @@
 #include <xen/bug.h>
 #include <asm/csr.h>
 
+/* Clock cycles count at Xen startup */
+extern unsigned long xen_start_clock_cycles;
+
 struct vcpu;
 
 static inline void force_update_vcpu_system_time(struct vcpu *v)
@@ -19,6 +22,8 @@ static inline cycles_t get_cycles(void)
     return csr_read(CSR_TIME);
 }
 
+void preinit_xen_time(void);
+
 #endif /* ASM__RISCV__TIME_H */
 
 /*
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index b0e587678e..836ad16fed 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -126,6 +126,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     riscv_fill_hwcap();
 
+    preinit_xen_time();
+
     printk("All set up\n");
 
     machine_halt();
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 5951b0ce91..caa133de84 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -27,8 +27,6 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
 
 /* time.c */
 
-unsigned long __ro_after_init cpu_khz;  /* CPU clock frequency in kHz. */
-
 s_time_t get_s_time(void)
 {
     BUG_ON("unimplemented");
diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c
new file mode 100644
index 0000000000..ae8efa3c59
--- /dev/null
+++ b/xen/arch/riscv/time.c
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <xen/device_tree.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/sections.h>
+
+unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
+unsigned long __ro_after_init xen_start_clock_cycles;
+
+static __initdata struct dt_device_node *timer;
+
+/* Set up the timer on the boot CPU (early init function) */
+static void __init preinit_dt_xen_time(void)
+{
+    static const struct dt_device_match __initconstrel timer_ids[] =
+    {
+        DT_MATCH_PATH("/cpus"),
+        { /* sentinel */ },
+    };
+    uint32_t rate;
+
+    timer = dt_find_matching_node(NULL, timer_ids);
+    if ( !timer )
+        panic("Unable to find a compatible timer in the device tree\n");
+
+    dt_device_set_used_by(timer, DOMID_XEN);
+
+    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
+        panic("Unable to find clock frequency\n");
+
+    cpu_khz = rate / 1000;
+}
+
+void __init preinit_xen_time(void)
+{
+    preinit_dt_xen_time();
+
+    xen_start_clock_cycles = get_cycles();
+}
-- 
2.48.1

[PATCH v2 2/4] automation: select APLIC and IMSIC to handle both wired interrupts and MSIs

[Oleksii Kurochko]

By default, the `aia` option is set to "none" which selects the SiFive PLIC for
handling wired interrupts. However, since PLIC is now considered obsolete and
will not be supported by Xen now, APLIC and IMSIC are selected instead to manage
both wired interrupts and MSIs.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---

Tests results:
  https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/1734636979

Based on the source code of Qemu the support of AIA should be present from 7.0.0:
   28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")

```
$ git tag --contains 28d8c281200f20a060c456c81fd1564f3d119fda
staging-mjt-test
trivial-patches-pull-request
v7.0.0
v7.0.0-rc0
v7.0.0-rc1
v7.0.0-rc2
v7.0.0-rc3
v7.0.0-rc4
v7.1.0
...
```

And in Xen's GitLab CI it is used 7.2.11:
```
$ CONTAINER_NO_PULL=1 CONTAINER=bookworm-riscv64 ./automation/scripts/containerize 
*** Launching container ...   
user@6a1d1f0077fe:/build$ qemu-system-riscv64 --version
QEMU emulator version 7.2.11 (Debian 1:7.2+dfsg-7+deb12u6)
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
```

---
Changes in v2:
 - Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 automation/scripts/qemu-smoke-riscv64.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index 8f755d0a6a..b2e112c942 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -6,7 +6,7 @@ set -ex -o pipefail
 rm -f smoke.serial
 
 export TEST_CMD="qemu-system-riscv64 \
-    -M virt \
+    -M virt,aia=aplic-imsic \
     -smp 1 \
     -nographic \
     -m 2g \
-- 
2.48.1

[PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Oleksii Kurochko]

Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
Interrupt Controller (APLIC) in Xen:
 - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
   ensuring that only one APLIC instance is supported in S mode.
 - Initialize APLIC's correspoinding DT node.
 - Declaring the DT device match table for APLIC.
 - Setting `aplic_info.hw_version` during its declaration.
 - Declaring an APLIC device.

Since Microchip originally developed aplic.c [1], an internal discussion
with them led to the decision to use the MIT license instead of the default
GPL-2.0-only.

[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d

Signed-off-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - drop initialization of .node member to NULL of aplic_info.
 - s/initcont/initconstrel for aplic_dt_match and put initconstrel between
   type and identifier.
 - Update identation for DT_DEVICE_START(aplic, ...).
 - Update the commit message.
---
 xen/arch/riscv/Makefile           |  1 +
 xen/arch/riscv/aplic.c            | 49 +++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/intc.h | 20 +++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 xen/arch/riscv/aplic.c
 create mode 100644 xen/arch/riscv/include/asm/intc.h

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 82016a957a..dd5fd25c7d 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,3 +1,4 @@
+obj-y += aplic.o
 obj-y += cpufeature.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += entry.o
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
new file mode 100644
index 0000000000..856b3bf84f
--- /dev/null
+++ b/xen/arch/riscv/aplic.c
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/aplic.c
+ *
+ * RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) 2023-2024 Microchip.
+ * Copyright (c) 2024-2025 Vates
+ */
+
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/types.h>
+
+#include <asm/device.h>
+#include <asm/intc.h>
+
+static struct intc_info aplic_info = {
+    .hw_version = INTC_APLIC
+};
+
+static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
+{
+    if ( aplic_info.node )
+    {
+        printk("XEN doesn't support more than one S mode APLIC\n");
+        return -ENODEV;
+    }
+
+    /* don't process if APLIC node is not for S mode */
+    if ( dt_get_property(node, "riscv,children", NULL) )
+        return -ENODEV;
+
+    aplic_info.node = node;
+
+    return 0;
+}
+
+static const struct dt_device_match __initconstrel aplic_dt_match[] =
+{
+    DT_MATCH_COMPATIBLE("riscv,aplic"),
+    { /* sentinel */ },
+};
+
+DT_DEVICE_START(aplic, "APLIC", DEVICE_INTERRUPT_CONTROLLER)
+    .dt_match = aplic_dt_match,
+    .init = aplic_preinit,
+DT_DEVICE_END
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
new file mode 100644
index 0000000000..ff9bb33896
--- /dev/null
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * (c) 2023-2024 Microchip Technology Inc.
+ * (c) 2024 Vates
+ */
+
+#ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
+#define ASM__RISCV__INTERRUPT_CONTOLLER_H
+
+enum intc_version {
+    INTC_APLIC,
+};
+
+struct intc_info {
+    enum intc_version hw_version;
+    const struct dt_device_node *node;
+};
+
+#endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
-- 
2.48.1

[PATCH v2 4/4] xen/riscv: introduce intc_preinit()

[Oleksii Kurochko]

Currently, only the device tree method is available to locate and perform
pre-initialization steps for the interrupt controller (at the moment, only
one interrupt controller is going to be supported). When `acpi_disabled`
is true, the system will scan for a node with the "interrupt-controller"
property and then call `device_init()` to validate if it is an expected
interrupt controller and if yes then save this node for further usage.

If `acpi_disabled` is false, the system will panic, as ACPI support is not
yet implemented for RISC-V.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Update the commit message.
---
 xen/arch/riscv/Makefile           |  1 +
 xen/arch/riscv/include/asm/intc.h |  2 ++
 xen/arch/riscv/intc.c             | 14 ++++++++++++++
 xen/arch/riscv/setup.c            |  3 +++
 4 files changed, 20 insertions(+)
 create mode 100644 xen/arch/riscv/intc.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index dd5fd25c7d..0c6c4a38a3 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 += intc.o
 obj-y += mm.o
 obj-y += pt.o
 obj-$(CONFIG_RISCV_64) += riscv64/
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index ff9bb33896..52ba196d87 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -17,4 +17,6 @@ struct intc_info {
     const struct dt_device_node *node;
 };
 
+void intc_preinit(void);
+
 #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
new file mode 100644
index 0000000000..4061a3c457
--- /dev/null
+++ b/xen/arch/riscv/intc.c
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/acpi.h>
+#include <xen/device_tree.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+
+void __init intc_preinit(void)
+{
+    if ( acpi_disabled )
+        intc_dt_preinit();
+    else
+        panic("ACPI interrupt controller preinit() isn't implemented\n");
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 836ad16fed..4e416f6e44 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -16,6 +16,7 @@
 #include <asm/cpufeature.h>
 #include <asm/early_printk.h>
 #include <asm/fixmap.h>
+#include <asm/intc.h>
 #include <asm/sbi.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
@@ -128,6 +129,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     preinit_xen_time();
 
+    intc_preinit();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.48.1

Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Jan Beulich]

On 25.03.2025 18:36, Oleksii Kurochko wrote:
> preinit_xen_time() does two things:
> 1. Parse timebase-frequency properpy of /cpus node to initialize
>    cpu_khz variable.
> 2. Initialize xen_start_clock_cycles with the current time counter
>    value.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in v2:
>  - Update SPDX tag for time.c
>  - s/read_mostly/__ro_after_init for boot_count variable.
>  - Add declaration of boot_count to asm/time.h.
>  - Rename boot_count to xen_start_clock_cycles.

I'm not going to insist on another name change, but I'm having a hard time
seeing why almost any global variable in Xen would need to have a xen_
prefix. "start" also can be ambiguous, so imo boot_clock_cycles would have
been best.

> --- /dev/null
> +++ b/xen/arch/riscv/time.c
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#include <xen/device_tree.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/sections.h>
> +
> +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
> +unsigned long __ro_after_init xen_start_clock_cycles;

For the theoretical at this point case of support RV32, will unsigned long
be wide enough? I.e. is the counter only 32 bits wide when XLEN=32?

> +static __initdata struct dt_device_node *timer;

Please can __initdata (and alike) live at its canonical place, between
type and identifier?

It's also unclear at this point why this needs to be a file scope variable.

> +/* Set up the timer on the boot CPU (early init function) */
> +static void __init preinit_dt_xen_time(void)
> +{
> +    static const struct dt_device_match __initconstrel timer_ids[] =
> +    {
> +        DT_MATCH_PATH("/cpus"),
> +        { /* sentinel */ },
> +    };
> +    uint32_t rate;
> +
> +    timer = dt_find_matching_node(NULL, timer_ids);
> +    if ( !timer )
> +        panic("Unable to find a compatible timer in the device tree\n");
> +
> +    dt_device_set_used_by(timer, DOMID_XEN);
> +
> +    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
> +        panic("Unable to find clock frequency\n");
> +
> +    cpu_khz = rate / 1000;

"rate" being only 32 bits wide, what about clock rates above 4GHz? Or is
this some external clock running at a much lower rate than the CPU would?

> +}
> +
> +void __init preinit_xen_time(void)
> +{
> +    preinit_dt_xen_time();
> +
> +    xen_start_clock_cycles = get_cycles();
> +}

I take it that more stuff is going to be added to this function? Else I
wouldn't see why preinit_dt_xen_time() needed to be a separate function.

Jan

Re: [PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Jan Beulich]

On 25.03.2025 18:36, Oleksii Kurochko wrote:
> Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
> Interrupt Controller (APLIC) in Xen:
>  - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
>    ensuring that only one APLIC instance is supported in S mode.
>  - Initialize APLIC's correspoinding DT node.
>  - Declaring the DT device match table for APLIC.
>  - Setting `aplic_info.hw_version` during its declaration.
>  - Declaring an APLIC device.
> 
> Since Microchip originally developed aplic.c [1], an internal discussion
> with them led to the decision to use the MIT license instead of the default
> GPL-2.0-only.
> 
> [1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
> 
> Signed-off-by: Romain Caritey <Romain.Caritey@microchip.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

You recall that From: != 1st S-o-b is unusual, and wants some explanation.
IOW it's unclear who the original author of this patch is.

> --- /dev/null
> +++ b/xen/arch/riscv/aplic.c
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic.c
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) 2023-2024 Microchip.
> + * Copyright (c) 2024-2025 Vates
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/types.h>
> +
> +#include <asm/device.h>
> +#include <asm/intc.h>
> +
> +static struct intc_info aplic_info = {
> +    .hw_version = INTC_APLIC
> +};

Is this going to be written to (much) post-init? IOW - __read_mostly or
even __ro_after_init?

With authorship clarified and this variable adjusted according to whatever
the longer term use of it is
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 4/4] xen/riscv: introduce intc_preinit()

[Jan Beulich]

On 25.03.2025 18:37, Oleksii Kurochko wrote:
> Currently, only the device tree method is available to locate and perform
> pre-initialization steps for the interrupt controller (at the moment, only
> one interrupt controller is going to be supported). When `acpi_disabled`
> is true, the system will scan for a node with the "interrupt-controller"
> property and then call `device_init()` to validate if it is an expected
> interrupt controller and if yes then save this node for further usage.
> 
> If `acpi_disabled` is false, the system will panic, as ACPI support is not
> yet implemented for RISC-V.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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

Re: [PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Oleksii Kurochko]

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


On 3/26/25 4:19 PM, Jan Beulich wrote:
> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>> Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
>> Interrupt Controller (APLIC) in Xen:
>>   - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
>>     ensuring that only one APLIC instance is supported in S mode.
>>   - Initialize APLIC's correspoinding DT node.
>>   - Declaring the DT device match table for APLIC.
>>   - Setting `aplic_info.hw_version` during its declaration.
>>   - Declaring an APLIC device.
>>
>> Since Microchip originally developed aplic.c [1], an internal discussion
>> with them led to the decision to use the MIT license instead of the default
>> GPL-2.0-only.
>>
>> [1]https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
>>
>> Signed-off-by: Romain Caritey<Romain.Caritey@microchip.com>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> You recall that From: != 1st S-o-b is unusual, and wants some explanation.
> IOW it's unclear who the original author of this patch is.

I'm not 100% sure who should be the author. Such patch doesn't exist before but I took the changes
based on the changes mentioned in commit message as [1].

If you think that the author should be Romain, I am okay with that.

>
>> --- /dev/null
>> +++ b/xen/arch/riscv/aplic.c
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +/*
>> + * xen/arch/riscv/aplic.c
>> + *
>> + * RISC-V Advanced Platform-Level Interrupt Controller support
>> + *
>> + * Copyright (c) 2023-2024 Microchip.
>> + * Copyright (c) 2024-2025 Vates
>> + */
>> +
>> +#include <xen/errno.h>
>> +#include <xen/init.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/device.h>
>> +#include <asm/intc.h>
>> +
>> +static struct intc_info aplic_info = {
>> +    .hw_version = INTC_APLIC
>> +};
> Is this going to be written to (much) post-init? IOW - __read_mostly or
> even __ro_after_init?

I think that __read_mostly would be better because intc_info structure in the future
will contain member "void *private". And in `private` it can be a data which can
be changed. For example, `private` can contain an aplic_priv structure:
struct aplic_priv {
     /* number of irqs */
     uint32_t   nr_irqs;

     /* base physical address and size */
     paddr_t    paddr_start;
     paddr_t    paddr_end;
     uint64_t   size;

     /* registers */
     struct aplic_regs   *regs;

     /* imsic configuration */
     const struct imsic_config *imsic_cfg;
};

and regs from aplic_priv structure can be changed in runtime.

>
> With authorship clarified and this variable adjusted according to whatever
> the longer term use of it is
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

~ Oleksii

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

Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Oleksii Kurochko]

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


On 3/26/25 4:13 PM, Jan Beulich wrote:
> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>> preinit_xen_time() does two things:
>> 1. Parse timebase-frequency properpy of /cpus node to initialize
>>     cpu_khz variable.
>> 2. Initialize xen_start_clock_cycles with the current time counter
>>     value.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> Changes in v2:
>>   - Update SPDX tag for time.c
>>   - s/read_mostly/__ro_after_init for boot_count variable.
>>   - Add declaration of boot_count to asm/time.h.
>>   - Rename boot_count to xen_start_clock_cycles.
> I'm not going to insist on another name change, but I'm having a hard time
> seeing why almost any global variable in Xen would need to have a xen_
> prefix. "start" also can be ambiguous, so imo boot_clock_cycles would have
> been best.

I can change that during the work on the version of this patch.

>
>> --- /dev/null
>> +++ b/xen/arch/riscv/time.c
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#include <xen/device_tree.h>
>> +#include <xen/init.h>
>> +#include <xen/lib.h>
>> +#include <xen/sections.h>
>> +
>> +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
>> +unsigned long __ro_after_init xen_start_clock_cycles;
> For the theoretical at this point case of support RV32, will unsigned long
> be wide enough? I.e. is the counter only 32 bits wide when XLEN=32?

No, it will be really an issue and the type should be uint64_t for this variable
because on RV32I the timeh CSR is a read-only shadow of the upper 32 bits of the
memory-mapped mtime register, while time shadows only the lower 32 bits of mtime.
So on RV32 it is still 64-bit long and requires two reads to get cycle value.

>
>> +static __initdata struct dt_device_node *timer;
> Please can __initdata (and alike) live at its canonical place, between
> type and identifier?
>
> It's also unclear at this point why this needs to be a file scope variable.

Because this variable is used and will be used only in preinit_dt_xen_time().
But I think we could move the declaration of this variable to preinit_dt_xen_time()
as it is used only during preinit_dt_xen_time().

>
>> +/* Set up the timer on the boot CPU (early init function) */
>> +static void __init preinit_dt_xen_time(void)
>> +{
>> +    static const struct dt_device_match __initconstrel timer_ids[] =
>> +    {
>> +        DT_MATCH_PATH("/cpus"),
>> +        { /* sentinel */ },
>> +    };
>> +    uint32_t rate;
>> +
>> +    timer = dt_find_matching_node(NULL, timer_ids);
>> +    if ( !timer )
>> +        panic("Unable to find a compatible timer in the device tree\n");
>> +
>> +    dt_device_set_used_by(timer, DOMID_XEN);
>> +
>> +    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
>> +        panic("Unable to find clock frequency\n");
>> +
>> +    cpu_khz = rate / 1000;
> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is
> this some external clock running at a much lower rate than the CPU would?

It is the frequency of the hardware timer that drives the 
|mtime|register, it defines the frequency (in Hz) at which the timer 
increments.


>
>> +}
>> +
>> +void __init preinit_xen_time(void)
>> +{
>> +    preinit_dt_xen_time();
>> +
>> +    xen_start_clock_cycles = get_cycles();
>> +}
> I take it that more stuff is going to be added to this function? Else I
> wouldn't see why preinit_dt_xen_time() needed to be a separate function.

Actually, I checked my latest downstream branch and I haven't added nothing
extra to this function.
Only one thing that I want to add is ACPI case:
     if ( acpi_disabled )
         preinit_dt_xen_time();
     else
         panic("TBD\n"); /* preinit_acpi_xen_time(); */

~ Oleksii

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

Re: [PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Jan Beulich]

On 26.03.2025 17:49, Oleksii Kurochko wrote:
> On 3/26/25 4:19 PM, Jan Beulich wrote:
>> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>>> Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
>>> Interrupt Controller (APLIC) in Xen:
>>>   - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
>>>     ensuring that only one APLIC instance is supported in S mode.
>>>   - Initialize APLIC's correspoinding DT node.
>>>   - Declaring the DT device match table for APLIC.
>>>   - Setting `aplic_info.hw_version` during its declaration.
>>>   - Declaring an APLIC device.
>>>
>>> Since Microchip originally developed aplic.c [1], an internal discussion
>>> with them led to the decision to use the MIT license instead of the default
>>> GPL-2.0-only.
>>>
>>> [1]https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
>>>
>>> Signed-off-by: Romain Caritey<Romain.Caritey@microchip.com>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> You recall that From: != 1st S-o-b is unusual, and wants some explanation.
>> IOW it's unclear who the original author of this patch is.
> 
> I'm not 100% sure who should be the author. Such patch doesn't exist before but I took the changes
> based on the changes mentioned in commit message as [1].
> 
> If you think that the author should be Romain, I am okay with that.

I can't sensibly form an opinion here. This needs settling between him and you.
From your reply I'm not even convinced his S-o-b is legitimately there then.
You may want to use another, less standard tag in such a case (like the
Co-developed-by: that I've seen in use here and there) to still give credit to
him.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/aplic.c
>>> @@ -0,0 +1,49 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +
>>> +/*
>>> + * xen/arch/riscv/aplic.c
>>> + *
>>> + * RISC-V Advanced Platform-Level Interrupt Controller support
>>> + *
>>> + * Copyright (c) 2023-2024 Microchip.
>>> + * Copyright (c) 2024-2025 Vates
>>> + */
>>> +
>>> +#include <xen/errno.h>
>>> +#include <xen/init.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/device.h>
>>> +#include <asm/intc.h>
>>> +
>>> +static struct intc_info aplic_info = {
>>> +    .hw_version = INTC_APLIC
>>> +};
>> Is this going to be written to (much) post-init? IOW - __read_mostly or
>> even __ro_after_init?
> 
> I think that __read_mostly would be better because intc_info structure in the future
> will contain member "void *private". And in `private` it can be a data which can
> be changed.

You mean the pointer can change? Or merely what it points to, i.e. ...

> For example, `private` can contain an aplic_priv structure:
> struct aplic_priv {
>      /* number of irqs */
>      uint32_t   nr_irqs;
> 
>      /* base physical address and size */
>      paddr_t    paddr_start;
>      paddr_t    paddr_end;
>      uint64_t   size;
> 
>      /* registers */
>      struct aplic_regs   *regs;
> 
>      /* imsic configuration */
>      const struct imsic_config *imsic_cfg;
> };
> 
> and regs from aplic_priv structure can be changed in runtime.

... the contents of such a struct? In this latter case the struct instance
here can still be __ro_after_init as long as the pointer is set from an
__init function.

Jan

Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Jan Beulich]

On 26.03.2025 20:49, Oleksii Kurochko wrote:
> On 3/26/25 4:13 PM, Jan Beulich wrote:
>> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>>> +/* Set up the timer on the boot CPU (early init function) */
>>> +static void __init preinit_dt_xen_time(void)
>>> +{
>>> +    static const struct dt_device_match __initconstrel timer_ids[] =
>>> +    {
>>> +        DT_MATCH_PATH("/cpus"),
>>> +        { /* sentinel */ },
>>> +    };
>>> +    uint32_t rate;
>>> +
>>> +    timer = dt_find_matching_node(NULL, timer_ids);
>>> +    if ( !timer )
>>> +        panic("Unable to find a compatible timer in the device tree\n");
>>> +
>>> +    dt_device_set_used_by(timer, DOMID_XEN);
>>> +
>>> +    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
>>> +        panic("Unable to find clock frequency\n");
>>> +
>>> +    cpu_khz = rate / 1000;
>> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is
>> this some external clock running at a much lower rate than the CPU would?
> 
> It is the frequency of the hardware timer that drives the 
> |mtime|register, it defines the frequency (in Hz) at which the timer 
> increments.

And that timer can't plausibly run at more than 4 GHz?

>>> +void __init preinit_xen_time(void)
>>> +{
>>> +    preinit_dt_xen_time();
>>> +
>>> +    xen_start_clock_cycles = get_cycles();
>>> +}
>> I take it that more stuff is going to be added to this function? Else I
>> wouldn't see why preinit_dt_xen_time() needed to be a separate function.
> 
> Actually, I checked my latest downstream branch and I haven't added nothing
> extra to this function.
> Only one thing that I want to add is ACPI case:
>      if ( acpi_disabled )
>          preinit_dt_xen_time();
>      else
>          panic("TBD\n"); /* preinit_acpi_xen_time(); */

That's probably a good enough reason then to keep the function separate.

Jan

Re: [PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Oleksii Kurochko]

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


On 3/27/25 8:39 AM, Jan Beulich wrote:
> On 26.03.2025 17:49, Oleksii Kurochko wrote:
>> On 3/26/25 4:19 PM, Jan Beulich wrote:
>>> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>>>> Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
>>>> Interrupt Controller (APLIC) in Xen:
>>>>    - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
>>>>      ensuring that only one APLIC instance is supported in S mode.
>>>>    - Initialize APLIC's correspoinding DT node.
>>>>    - Declaring the DT device match table for APLIC.
>>>>    - Setting `aplic_info.hw_version` during its declaration.
>>>>    - Declaring an APLIC device.
>>>>
>>>> Since Microchip originally developed aplic.c [1], an internal discussion
>>>> with them led to the decision to use the MIT license instead of the default
>>>> GPL-2.0-only.
>>>>
>>>> [1]https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
>>>>
>>>> Signed-off-by: Romain Caritey<Romain.Caritey@microchip.com>
>>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>>> You recall that From: != 1st S-o-b is unusual, and wants some explanation.
>>> IOW it's unclear who the original author of this patch is.
>> I'm not 100% sure who should be the author. Such patch doesn't exist before but I took the changes
>> based on the changes mentioned in commit message as [1].
>>
>> If you think that the author should be Romain, I am okay with that.
> I can't sensibly form an opinion here. This needs settling between him and you.
>  From your reply I'm not even convinced his S-o-b is legitimately there then.
> You may want to use another, less standard tag in such a case (like the
> Co-developed-by: that I've seen in use here and there) to still give credit to
> him.

I'll have a conversation with Romain and then just re-send the patch.

>
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/aplic.c
>>>> @@ -0,0 +1,49 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +
>>>> +/*
>>>> + * xen/arch/riscv/aplic.c
>>>> + *
>>>> + * RISC-V Advanced Platform-Level Interrupt Controller support
>>>> + *
>>>> + * Copyright (c) 2023-2024 Microchip.
>>>> + * Copyright (c) 2024-2025 Vates
>>>> + */
>>>> +
>>>> +#include <xen/errno.h>
>>>> +#include <xen/init.h>
>>>> +#include <xen/types.h>
>>>> +
>>>> +#include <asm/device.h>
>>>> +#include <asm/intc.h>
>>>> +
>>>> +static struct intc_info aplic_info = {
>>>> +    .hw_version = INTC_APLIC
>>>> +};
>>> Is this going to be written to (much) post-init? IOW - __read_mostly or
>>> even __ro_after_init?
>> I think that __read_mostly would be better because intc_info structure in the future
>> will contain member "void *private". And in `private` it can be a data which can
>> be changed.
> You mean the pointer can change? Or merely what it points to, i.e. ...
>
>> For example, `private` can contain an aplic_priv structure:
>> struct aplic_priv {
>>       /* number of irqs */
>>       uint32_t   nr_irqs;
>>
>>       /* base physical address and size */
>>       paddr_t    paddr_start;
>>       paddr_t    paddr_end;
>>       uint64_t   size;
>>
>>       /* registers */
>>       struct aplic_regs   *regs;
>>
>>       /* imsic configuration */
>>       const struct imsic_config *imsic_cfg;
>> };
>>
>> and regs from aplic_priv structure can be changed in runtime.
> ... the contents of such a struct? In this latter case the struct instance
> here can still be __ro_after_init as long as the pointer is set from an
> __init function.

I meant that the data will be changed. The pointer will be initialized once
in the following way:
static struct intc_info aplic_info = {
     .hw_version = INTC_APLIC,
     .private = &aplic
};

I will update for both structures __ro_after_init() then.

Thanks for clarification.

~ Oleksii

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

Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Oleksii Kurochko]

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


On 3/27/25 8:42 AM, Jan Beulich wrote:
> On 26.03.2025 20:49, Oleksii Kurochko wrote:
>> On 3/26/25 4:13 PM, Jan Beulich wrote:
>>> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>>>> +/* Set up the timer on the boot CPU (early init function) */
>>>> +static void __init preinit_dt_xen_time(void)
>>>> +{
>>>> +    static const struct dt_device_match __initconstrel timer_ids[] =
>>>> +    {
>>>> +        DT_MATCH_PATH("/cpus"),
>>>> +        { /* sentinel */ },
>>>> +    };
>>>> +    uint32_t rate;
>>>> +
>>>> +    timer = dt_find_matching_node(NULL, timer_ids);
>>>> +    if ( !timer )
>>>> +        panic("Unable to find a compatible timer in the device tree\n");
>>>> +
>>>> +    dt_device_set_used_by(timer, DOMID_XEN);
>>>> +
>>>> +    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
>>>> +        panic("Unable to find clock frequency\n");
>>>> +
>>>> +    cpu_khz = rate / 1000;
>>> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is
>>> this some external clock running at a much lower rate than the CPU would?
>> It is the frequency of the hardware timer that drives the
>> |mtime|register, it defines the frequency (in Hz) at which the timer
>> increments.
> And that timer can't plausibly run at more than 4 GHz?

I haven't seen yet such big timer frequency.

But if to look at device tree spec:
timebase-frequency: Specifies the current frequency at which the
timebase and decrementer registers are updated (in Hertz).
The value is a <prop-encoded-array> in one of two forms:
• A 32-bit integer consisting of one <u32>
   specifying the frequency.
• A 64-bit integer represented as a <u64>.

Interesting that Linux kernel reads timebase-frequency as u32:
    u32 prop;
    ...
    if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
and then saves it to:
    riscv_timebase = prop;
where riscv_timebase is declared as unsigned long.

I think it can be left as it is now as if timebase-frequency will be u64 then
it means that it will be needed to read two u32 values and in this case dt_property_read_u32()
will return 0 and the panic will occur.

~ Oleksii

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

Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Jan Beulich]

On 27.03.2025 12:48, Oleksii Kurochko wrote:
> I think it can be left as it is now as if timebase-frequency will be u64 then
> it means that it will be needed to read two u32 values and in this case dt_property_read_u32()
> will return 0 and the panic will occur.

Fair enough; please say a word on this aspect in the description, though.

Jan