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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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.

> 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.
The logic that we will use for secondary cpu booting is similar to the 
"spin-table" mechanism used in arm64/smpboot.c.
This is :-

1. Write the address of init_secondary() to cpu-release-address register 
of the secondary cpu. (In my current patch, I attempt to achieve this.)
2. Write to the configuration register of the secondary cpu to bring it 
out of reset.
This is the corresponding patch (yet to be cleaned) which will be used 
to do step 2.
--- a/xen/arch/arm/platforms/amd-versal-net.c
+++ b/xen/arch/arm/platforms/amd-versal-net.c
@@ -36,6 +36,47 @@ static int versal_net_init_time(void)
     return 0;
 }

+static __init void versal_net_populate_plat_regs(void)
+{
+    /* TODO :- Parse 0xEB58C000 ie CORE_1_CFG0 from dtb */
+}
+
+static __init int versal_net_init(void)
+{
+    versal_net_populate_plat_regs();
+
+    return 0;
+}
+
+static __init int versasl_net_smp_init(void)
+{
+    return 0;
+}
+
+static __init int versal_net_cpu_up(int cpu)
+{
+    uint32_t __iomem *cpu_rel_addr = ioremap_nocache(0xEB58C000, 4);
+    uint32_t i = 0;
+
+    writel(1, cpu_rel_addr);
+
+    /* Delay has been added due to some platform nuance */
+    __iowmb();
+    for (i=0; i<0xF000000; i++)
+        __asm __volatile("nop");
+
+    writel(0, cpu_rel_addr);
+
+    /* Delay has been added due to some platform nuance */
+    __iowmb();
+    for (i=0; i<0xF000000; i++)
+        __asm __volatile("nop");
+
+    iounmap(cpu_rel_addr);
+
+    return 0;
+}
+
 static const char * const versal_net_dt_compat[] __initconst =
 {
     "xlnx,versal-net",
@@ -44,5 +85,8 @@ static const char * const versal_net_dt_compat[] 
__initconst =
 PLATFORM_START(versal_net, "XILINX VERSAL-NET")
     .compatible = versal_net_dt_compat,
+    .init = versal_net_init,
+    .smp_init = versasl_net_smp_init,
+    .cpu_up = versal_net_cpu_up,
     .init_time = versal_net_init_time,
 PLATFORM_END

> > 2. psci is not currently supported as a value for 'enable-method'.
> >
> > 3. update_identity_mapping() is not invoked as we are not sure if it is
> > required.
> This is not necessary at the moment for 32-bit. This may change in the 
> future as we make the 32-bit boot code more compliant. For now, I 
> would not add it.
Ack.

- Ayan

> Cheers,