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

Commit Message

Oleksii Kurochko March 25, 2025, 5:36 p.m. UTC

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 <[e-mail]>
Signed-off-by: Oleksii Kurochko <[e-mail]>
---
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

Comments

Jan Beulich March 26, 2025, 3:19 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Oleksii Kurochko <[e-mail]>

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 <[e-mail]>

Jan

Oleksii Kurochko March 26, 2025, 4:49 p.m. UTC | #2

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<[e-mail]>
>> Signed-off-by: Oleksii Kurochko<[e-mail]>
> 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<[e-mail]>

Thanks.

~ Oleksii

Jan Beulich March 27, 2025, 7:39 a.m. UTC | #3

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<[e-mail]>
>>> Signed-off-by: Oleksii Kurochko<[e-mail]>
>> 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

Oleksii Kurochko March 27, 2025, 9:50 a.m. UTC | #4

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<[e-mail]>
>>>> Signed-off-by: Oleksii Kurochko<[e-mail]>
>>> 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

Patch

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 */