Re: [Buildroot] [PATCH 3/9] arch/powerpc: Enable powerpc64le only on CPUs that support it

[Arnout Vandecappelle <arnout@mind.be>]

On 20/09/2022 19:09, Cédric Le Goater wrote:
> On 9/18/22 12:23, Arnout Vandecappelle wrote:
>>
>>
>> On 29/07/2022 02:08, Joel Stanley wrote:
>>> Invalid configurations lead to build failures, such as trying to enable
>>> ppc64le for the ppc970:
>>>
>>>    
>>> http://autobuild.buildroot.net/results/fda886768fce25ccd9b52b635ff5b13da7ba2d0c/
>>>
>>> In order to run a ppc64le userspace a kernel that runs in this mode is
>>> required. The only CPU supported in buildroot that can boot a ppc64le
>>> kernel is Power8, so mark all of the other 64-bit capable CPUs as not
>>> supporting ppc64le.
>>>
>>> This drops the comment about libc, which is true but doesn't tell the
>>> whole story.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>
>>   Applied to master with some changes, see below.
>>
>>> ---
>>>   arch/Config.in.powerpc | 11 +++++++----
>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/Config.in.powerpc b/arch/Config.in.powerpc
>>> index 8d392bfde814..7c6ae84348e9 100644
>>> --- a/arch/Config.in.powerpc
>>> +++ b/arch/Config.in.powerpc
>>> @@ -71,11 +71,9 @@ config BR2_powerpc_604e
>>>       depends on !BR2_ARCH_IS_64
>>>   config BR2_powerpc_620
>>>       bool "620"
>>> -    # No C library supports this variant on ppc64le
>>>       depends on !BR2_powerpc64le
>>>   config BR2_powerpc_630
>>>       bool "630"
>>> -    # No C library supports this variant on ppc64le
>>>       depends on !BR2_powerpc64le
>>>   config BR2_powerpc_740
>>>       bool "740"
>>> @@ -102,6 +100,7 @@ config BR2_powerpc_860
>>>       depends on !BR2_ARCH_IS_64
>>>   config BR2_powerpc_970
>>>       bool "970"
>>> +    depends on !BR2_powerpc64le
>>
>>   As suggested by Thomas, I changed this to positive logic. I'll also push a 
>> follow-up patch that changes the rest of the file to positive logic.
>>
>>   Could you (or any other powerpc "expert") review the conditions to be sure 
>> the architecture selection options are correct now?
>>
>>>       select BR2_POWERPC_CPU_HAS_ALTIVEC
>>>   config BR2_powerpc_8540
>>>       bool "8540 / e500v1"
>>> @@ -129,21 +128,25 @@ config BR2_powerpc_e6500
>>>       select BR2_POWERPC_CPU_HAS_ALTIVEC
>>>   config BR2_powerpc_power4
>>>       bool "power4"
>>> -    # No C library supports this variant on ppc64le
>>>       depends on !BR2_powerpc64le
>>>   config BR2_powerpc_power5
>>>       bool "power5"
>>> -    # No C library supports this variant on ppc64le
>>>       depends on !BR2_powerpc64le
>>> +    depends on BR2_ARCH_IS_64
>>
>>   AFAIU, all POWER CPUs are able to run in 32-bit mode 
> 
> yes. The question is more : can we generate correctly 32-bit software
> for all these CPUs ? firmware, kernel and userspace. I doubt we have
> correct support for older CPUs.

  That is indeed the question. However, we have offered this possibility for 
years. So if it is in fact working, people may be relying on it. So unless you 
have some actual evidence that it doesn't work, I don't think we should remove it.

  I also notice that you removed 32-bit support for the Power CPUs, but didn't 
touch the non-IBM CPUs like e5500 which is used in some Freescale/NXP SoCs. And 
indeed, IIRC I had a customer a few years ago who was running a 32-bit kernel 
and userspace on such a CPU.


>> and I believe it's even possible to run a 64-bit kernel with 32-bit userspace 
>> like on ARM. Is this not true?
> 
> It is. This is checked on older CPUs, like G5, under QEMU but 32bit is
> bit-rotting in many places.
> 
>>
>>   If POWER5+ really can't run in 32-bit mode (or this is not supported by the 
>> kernel), 
> 
> I think there is an issue with the use of the rfi instruction in 32-bit
> mode which was removed from the ISA of server CPUs. You can not generate
> a 32bit kernel for such CPUs AFAIR

  So if you're sure of this, feel free to send a patch removing the BR2_powerpc 
part of the relevant CPUs, with some evidence in the commit log why this 
wouldn't work anyway. Ideal evidence would be that the build of the kernel 
fails, but something less concrete can work as well.


>> are you sure that this *is* possible on POWER4?
> 
> POWER4 support was removed from Linux ~4 years ago.

  Ah, in that case we should probably remove POWER4 entirely.


  Regards,
  Arnout

>>   In either case, if the current conditions are not correct, please send a 
>> follow-up patch to fix them even more.
> 
> It is safer to build all 64-bit (kernel + userspace) for server CPUs.
> 
>>>   config BR2_powerpc_power6
>>>       bool "power6"
>>> +    depends on !BR2_powerpc64le
>>> +    depends on BR2_ARCH_IS_64
>>
>>   So here I wrote the condition as
>>
>>      depends on BR2_powerpc || BR2_powerpc64
>>
>> because I do believe it supports 32-bit.
> 
> These definitions can be problematic :
> 
>    config BR2_powerpc_powerx
>        bool "powerx"
>        depends on BR2_powerpc || BR2_powerpc64
> 
> because they imply that the kernel could be built in 32bit. See above.
> 
> Thanks,
> 
> C.
> 
>>
>>   Regards,
>>   Arnout
>>
>>>       select BR2_POWERPC_CPU_HAS_ALTIVEC
>>>   config BR2_powerpc_power7
>>>       bool "power7"
>>> +    depends on !BR2_powerpc64le
>>> +    depends on BR2_ARCH_IS_64
>>>       select BR2_POWERPC_CPU_HAS_ALTIVEC
>>>       select BR2_POWERPC_CPU_HAS_VSX
>>>   config BR2_powerpc_power8
>>>       bool "power8"
>>> +    depends on BR2_ARCH_IS_64
>>>       select BR2_POWERPC_CPU_HAS_ALTIVEC
>>>       select BR2_POWERPC_CPU_HAS_VSX
>>>   endchoice
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot