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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 2 May 2023, Ayan Kumar Halder wrote:
> On some of the Arm32 based systems (eg Cortex-R52), smpboot is supported.
> 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
> 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"
> 
> 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.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 
> The dts snippet with which this has been validated is :-
> 
>     cpus {
>         #address-cells = <0x02>;
>         #size-cells = <0x00>;
> 
>         cpu-map {
> 
>             cluster0 {
> 
>                 core0 {
> 
>                     thread0 {
>                         cpu = <0x02>;
>                     };
>                 };
>                 core1 {
> 
>                     thread0 {
>                         cpu = <0x03>;
>                     };
>                 };
>             };
>         };
> 
>         cpu@0 {
>             device_type = "cpu";
>             compatible = "arm,armv8";
>             reg = <0x00 0x00>;
>             phandle = <0x02>;
>         };
> 
>         cpu@1 {
>             device_type = "cpu";
>             compatible = "arm,armv8";
>             reg = <0x00 0x01>;
>             enable-method = "spin-table";
>             cpu-release-addr = <0xEB58C010>;
>             phandle = <0x03>;
>         };
>     };
> 
> Although currently I have tested this on Cortex-R52, I feel this may be 
> helpful
> to enable smp on other Arm32 based systems as well. Happy to hear opinions.

I think you are right


>  xen/arch/arm/arm32/smpboot.c | 84 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c
> index 518e9f9c7e..feb249d3f8 100644
> --- a/xen/arch/arm/arm32/smpboot.c
> +++ b/xen/arch/arm/arm32/smpboot.c
> @@ -1,24 +1,100 @@
>  #include <xen/device_tree.h>
>  #include <xen/init.h>
>  #include <xen/smp.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
>  #include <asm/platform.h>
>  
> +struct smp_enable_ops {
> +        int             (*prepare_cpu)(int);
> +};

coding style


> +static uint32_t cpu_release_addr[NR_CPUS];
> +static struct smp_enable_ops smp_enable_ops[NR_CPUS];

they could be __initdata


>  int __init arch_smp_init(void)
>  {
>      return platform_smp_init();
>  }
>  
> -int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
> +static int __init smp_spin_table_cpu_up(int cpu)
> +{
> +    uint32_t __iomem *release;
> +
> +    if (!cpu_release_addr[cpu])

code style


> +    {
> +        printk("CPU%d: No release addr\n", cpu);
> +        return -ENODEV;
> +    }
> +
> +    release = ioremap_nocache(cpu_release_addr[cpu], 4);
> +    if ( !release )
> +    {
> +        dprintk(XENLOG_ERR, "CPU%d: Unable to map release address\n", cpu);
> +        return -EFAULT;
> +    }
> +
> +    writel(__pa(init_secondary), release);
> +
> +    iounmap(release);

I think we need a wmb() ?


> +    sev();
> +
> +    return 0;
> +}
> +
> +static void __init smp_spin_table_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.
> +    if ( !dt_property_read_u32(dn, "cpu-release-addr", 
> &cpu_release_addr[cpu]) )

It looks like cpu-release-addr could be u64 or u32. Can we detect the
size of the property and act accordingly? If the address is u64 and
above 4GB it is fine to abort.


> +    {
> +        printk("CPU%d has no cpu-release-addr\n", cpu);
> +        return;
> +    }
> +
> +    smp_enable_ops[cpu].prepare_cpu = smp_spin_table_cpu_up;
> +}
> +
> +static int __init dt_arch_cpu_init(int cpu, struct dt_device_node *dn)
> +{
> +    const char *enable_method;
> +
> +    /*
> +     * Refer Documentation/devicetree/bindings/arm/cpus.yaml, it says on
> +     * ARM 32-bit systems this property is optional.
>       */
> +    enable_method = dt_get_property(dn, "enable-method", NULL);
> +    if (!enable_method)

coding style


> +    {
> +        return 0;
> +    }
> +
> +    if ( !strcmp(enable_method, "spin-table") )
> +        smp_spin_table_init(cpu, dn);
> +    else
> +    {
> +        printk("CPU%d has unknown enable method \"%s\"\n", cpu, 
> enable_method);
> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  
> +int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
> +{
> +    return dt_arch_cpu_init(cpu, dn);
> +}
> +
>  int arch_cpu_up(int cpu)
>  {
> -    return platform_cpu_up(cpu);
> +    int ret = 0;
> +
> +    if ( smp_enable_ops[cpu].prepare_cpu )
> +        ret = smp_enable_ops[cpu].prepare_cpu(cpu);
> +
> +    if ( !ret )
> +        return platform_cpu_up(cpu);

I think this should be:

    if ( smp_enable_ops[cpu].prepare_cpu )
        ret = smp_enable_ops[cpu].prepare_cpu(cpu);
    else
        ret = platform_cpu_up(cpu);




> +    return ret;
>  }
>  
>  void arch_cpu_up_finish(void)
> -- 
> 2.17.1
> 
>