Re: [RFC PATCH] xen/arm: arm32: Enable smpboot on Arm32 based systems

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 03/05/2023 18:43, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 03/05/2023 17:49, Ayan Kumar Halder wrote:
> > On 03/05/2023 08:40, Julien Grall wrote:
> > > Hi,
> > Hi Julien,
> > > Title: Did you mean "Enable spin table"?
> > Yes, that would be more concrete.
> > > On 02/05/2023 11:58, Ayan Kumar Halder wrote:
> > > > On some of the Arm32 based systems (eg Cortex-R52), smpboot is 
> > > > supported.
> > > Same here.
> > Yes
> > > > In these systems PSCI may not always be supported. In case of 
> > > > Cortex-R52, there
> > > > is no EL3 or secure mode. Thus, PSCI is not supported as it 
> > > > requires EL3.
> > > > Thus, we use 'spin-table' mechanism to boot the secondary cpus. The 
> > > > primary
> > > > cpu provides the startup address of the secondary cores. This 
> > > > address is
> > > > provided using the 'cpu-release-addr' property.
> > > >
> > > > To support smpboot, we have copied the code from 
> > > > xen/arch/arm/arm64/smpboot.c
> > > I would rather prefer if we don't duplicate the code but instead 
> > > move the logic in common code.
> > Ack
> > > > with the following changes :-
> > > >
> > > > 1. 'enable-method' is an optional property. Refer to the comment in
> > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.yaml 
> > > >
> > > > "      # On ARM 32-bit systems this property is optional"
> > > Looking at this list, "spin-table" doesn't seem to be supported
> > > for 32-bit systems. 
> > However, looking at 
> > https://developer.arm.com/documentation/den0013/d/Multi-core-processors/Booting-SMP-systems/SMP-boot-in-Linux 
> > , it seems "spin-table" is a valid boot mechanism for Armv7 cpus.
> I am not able to find the associated code in Linux 32-bit. Do you have 
> any pointer?
Unfortunately, no.

I see that in linux, "spin-table" support is added in 
arch/arm64/kernel/smp_spin_table.c. So, there seems to be no Arm32 
support for this.
> > > Can you point me to the discussion/patch where this would be added?
> > Actually, this is the first discussion I am having with regards to 
> > adding a "spin-table" support on Arm32.
> I was asking for the discussion on the Device-Tree/Linux ML or code.
> I don't really want to do a "spin-table" support if this is not even 
> supported in Linux.
I see your point. But that brings me to my next question, how do I parse 
cpu node specific properties for Arm32 cpus.
In 
https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.yaml 
, I see some of the properties valid for Arm32 cpus.
For example:-

secondary-boot-reg
rockchip,pmu

Also, it says "additionalProperties: true" which means I can add platform 
specific properties under the cpu node. Please correct me if mistaken.

My cpu nodes will look like this :-

        cpu@1 {
            device_type = "cpu";
            compatible = "arm,armv8";
            reg = <0x00 0x01>;
            enable-method = "spin-table";
            amd-cpu-release-addr = <0xEB58C010>; /* might also use 
"secondary-boot-reg" */
            amd-cpu-reset-addr = <0xEB58C000>;
            amd-cpu-reset-delay = <0xF00000>;
            amd-cpu-re
            phandle = <0x03>;
        };

        cpu@2 {
            device_type = "cpu";
            compatible = "arm,armv8";
            reg = <0x00 0x02>;
            enable-method = "spin-table";
            amd-cpu-release-addr = <0xEB59C010>; /* might also use 
"secondary-boot-reg" */
            amd-cpu-reset-addr = <0xEB59C000>;
            amd-cpu-reset-delay = <0xF00000>;
            amd-cpu-re
            phandle = <0x03>;
        };

If the reasoning makes sense, then does the following proposed change 
looks sane ?
diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c
index e7368665d5..0b281edb9d 100644
--- a/xen/arch/arm/arm32/smpboot.c
+++ b/xen/arch/arm/arm32/smpboot.c
@@ -10,10 +10,7 @@ int __init arch_smp_init(void)

 int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
 {
-    /* Not needed on ARM32, as there is no relevant information in
-     * the CPU device tree node for ARMv7 CPUs.
-     */
-    return 0;
+    return platform_cpu_init(cpu, dn);
 }

 int arch_cpu_up(int cpu)
diff --git a/xen/arch/arm/include/asm/platform.h 
b/xen/arch/arm/include/asm/platform.h
index d03b261ce8..5cd7af4d0e 100644
--- a/xen/arch/arm/include/asm/platform.h
+++ b/xen/arch/arm/include/asm/platform.h
@@ -18,6 +18,7 @@ struct platform_desc {
     /* SMP */
     int (*smp_init)(void);
     int (*cpu_up)(int cpu);
+    int (*cpu_init)(int cpu, struct dt_device_node *dn);
 #endif
     /* Specific mapping for dom0 */
     int (*specific_mapping)(struct domain *d);
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index a820665020..ed54b68c20 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -95,6 +95,14 @@ int __init platform_specific_mapping(struct domain *d)
 }

 #ifdef CONFIG_ARM_32
+int __init platform_cpu_init(int cpu, struct dt_device_node *dn)
+{
+    if ( platform && platform->cpu_init )
+        return platform_cpu_init(cpu, dn); /* parse platform specific 
cpu properties */
+
+    return 0;
+}
+
 int platform_cpu_up(int cpu)
 {
     if ( psci_ver )


Let me know your thoughts, please.

Kind regards,

Ayan

> Cheers,