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

[Julien Grall <julien@xxxxxxx>]

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