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.hal...@amd.com>
> ---
>
> 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
>
>
