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@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.
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);
+};
+
+static uint32_t cpu_release_addr[NR_CPUS];
+static struct smp_enable_ops smp_enable_ops[NR_CPUS];
+
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])
+ {
+ 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);
+
+ 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]) )
+ {
+ 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)
+ {
+ 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);
+
+ return ret;
}
void arch_cpu_up_finish(void)
--
2.17.1Hi, Title: Did you mean "Enable spin table"? 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. > 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. > 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. Can you point me to the discussion/patch where this would be added? > > 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. Cheers, -- Julien Grall
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,
>
Hi Ayan, 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? > > >> 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. Cheers, -- Julien Grall
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,
>
Hi Ayan,
On 04/05/2023 09:57, Ayan Kumar Halder wrote:
>
> 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.
I was probably not very clear in my previous message. What I don't want
is for Xen to use unofficial bindings.
IOW, if the existing binding doesn't allow the spin-table on arm32 (IMHO
it is not clear) and it makes sense to use them, then we should first
consider to send a patch against the documentation and merge it before
Xen can use the properties.
>
> 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.
For clarification, are you saying the bindings below is not yet
official? If so, then this should be first discussed with the
Device-Tree folks.
> 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";
The enable-method "spin-table" is generic and expect property like
cpu-release-addr. Yet...
> amd-cpu-release-addr = <0xEB58C010>; /* might also use
> "secondary-boot-reg" */
> amd-cpu-reset-addr = <0xEB58C000>;
> amd-cpu-reset-delay = <0xF00000>;
> amd-cpu-re
... these are all AMD properties. What are the reasons to not use the
generic "spin-table" bindings?
If you can't use it, then I think you should define a new type of
enable-method.
> 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 ?
I would first like to understand a bit more about how the bindings were
created and whether this was discussed with the Device-Tree community.
Cheers,
--
Julien Grall
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@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
>
>
Hi Stefano,
On 03/05/2023 00:55, Stefano Stabellini wrote:
>> + {
>> + 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() ?
I am not sure why we would need a wmb() here. Instead, looking at the
Linux version, I think we are missing a cache flush (so does on arm64)
which would be necessary if the CPU waiting for the release doesn't have
cache enabled.
Cheers,
--
Julien Grall
On Wed, 3 May 2023, Julien Grall wrote:
> Hi Stefano,
>
> On 03/05/2023 00:55, Stefano Stabellini wrote:
> > > + {
> > > + 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() ?
>
> I am not sure why we would need a wmb() here.
The code does:
writel(__pa(init_secondary), release);
iounmap
sev();
I was thinking of trying to make sure the write is completed before
issuing a sev(). Technically it is possible for the CPU to reorder the
sev() before the write as there is no explicit dependency between the
two?
> Instead, looking at the Linux
> version, I think we are missing a cache flush (so does on arm64) which would
> be necessary if the CPU waiting for the release doesn't have cache enabled.
I thought about it as well but here the patch is calling
ioremap_nocache(), so cache flushes should not be needed?
Hi Stefano,
On 03/05/2023 18:54, Stefano Stabellini wrote:
> On Wed, 3 May 2023, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/05/2023 00:55, Stefano Stabellini wrote:
>>>> + {
>>>> + 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() ?
>>
>> I am not sure why we would need a wmb() here.
>
> The code does:
>
> writel(__pa(init_secondary), release);
> iounmap
> sev();
>
> I was thinking of trying to make sure the write is completed before
> issuing a sev(). Technically it is possible for the CPU to reorder the
> sev() before the write as there is no explicit dependency between the
> two?
I would assume that iounmap() would contain an wmb(). But I guess this
is not something we should rely on.
>
>> Instead, looking at the Linux
>> version, I think we are missing a cache flush (so does on arm64) which would
>> be necessary if the CPU waiting for the release doesn't have cache enabled.
>
> I thought about it as well but here the patch is calling
> ioremap_nocache(), so cache flushes should not be needed?
Hmmm... You are right, we are using ioremap_nocache(). I got confused
because Linux is using ioremap_cache(). I am now wondering whether we
are using the correct helper.
AFAIU, spin table is part of the reserved memory section in the
Device-Tree. From section 5.3 in [1], the memory could be mapped
cacheable by the other end. So for correctness, it seems to me that we
would need to use ioremap_cache() + cache flush.
BTW, writel_relaxed() should be sufficient here as there is no ordering
necessary with any previous write.
Cheers,
[1]
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
--
Julien Grall
On Wed, 3 May 2023, Julien Grall wrote:
> Hi Stefano,
>
> On 03/05/2023 18:54, Stefano Stabellini wrote:
> > On Wed, 3 May 2023, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 03/05/2023 00:55, Stefano Stabellini wrote:
> > > > > + {
> > > > > + 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() ?
> > >
> > > I am not sure why we would need a wmb() here.
> >
> > The code does:
> >
> > writel(__pa(init_secondary), release);
> > iounmap
> > sev();
> >
> > I was thinking of trying to make sure the write is completed before
> > issuing a sev(). Technically it is possible for the CPU to reorder the
> > sev() before the write as there is no explicit dependency between the
> > two?
>
> I would assume that iounmap() would contain an wmb(). But I guess this is not
> something we should rely on.
>
> >
> > > Instead, looking at the Linux
> > > version, I think we are missing a cache flush (so does on arm64) which
> > > would
> > > be necessary if the CPU waiting for the release doesn't have cache
> > > enabled.
> >
> > I thought about it as well but here the patch is calling
> > ioremap_nocache(), so cache flushes should not be needed?
>
> Hmmm... You are right, we are using ioremap_nocache(). I got confused because
> Linux is using ioremap_cache(). I am now wondering whether we are using the
> correct helper.
>
> AFAIU, spin table is part of the reserved memory section in the Device-Tree.
> From section 5.3 in [1], the memory could be mapped cacheable by the other
> end. So for correctness, it seems to me that we would need to use
> ioremap_cache() + cache flush.
Good point
> BTW, writel_relaxed() should be sufficient here as there is no ordering
> necessary with any previous write.
© 2016 - 2024 Red Hat, Inc.