diff mbox series

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

Message ID 20220729000904.1295295-4-joel@jms.id.au
State Changes Requested
Headers show
Series powerpc: Fix ppc64le configurations | expand

Commit Message

Joel Stanley July 29, 2022, 12:08 a.m. UTC 
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>
---
 arch/Config.in.powerpc | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Cédric Le Goater July 29, 2022, 6:52 a.m. UTC | #1 
On 7/29/22 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.

I don't think we care much but Power7 could run also in LE.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> 
> 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>
> ---
>   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
>   	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
>   config BR2_powerpc_power6
>   	bool "power6"
> +	depends on !BR2_powerpc64le
> +	depends on BR2_ARCH_IS_64
>   	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
Yann E. MORIN July 29, 2022, 9:33 p.m. UTC | #2 
Joel, All,

On 2022-07-29 09:38 +0930, Joel Stanley spake thusly:
> 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.

Ah, this is where you hide incomatible CPUs for ppc64le. Good. This
should go before your generic CPU patch (patch 2).

Regards,
Yann E. MORIN.

> 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>
> ---
>  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
>  	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

This one is not strictly aout ppc64le. I.e. power5 is not an 32-bit CPU
at all.

Not: I am fine if the change is done in a single patch, but then the
commit should probably be a bit more generic, like

    arch/powerpc: fix variants dependencies

    Only allow variants in the case they are really workable:

      * the only variant in Buildroot that can be a ppc64le is power8

      * 970, power5, power6, and power7, pwer8 are 64-bit only

>  config BR2_powerpc_power6
>  	bool "power6"
> +	depends on !BR2_powerpc64le
> +	depends on BR2_ARCH_IS_64

Ditto, and for the rest too.

>  	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
> -- 
> 2.35.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Yann E. MORIN July 29, 2022, 9:37 p.m. UTC | #3 
Cédric, Joel, All,

On 2022-07-29 08:52 +0200, Cédric Le Goater spake thusly:
> On 7/29/22 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.
> I don't think we care much but Power7 could run also in LE.

Joel seemed to state that only the power8 can be LE, so who's right? ;-)

In any case, if maore variants turn out to be LE-compatible, it is still
possible to enable htem in followup patches.

Regards,
Yann E. MORIN.

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> >
> >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>
> >---
> >  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
> >  	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
> >  config BR2_powerpc_power6
> >  	bool "power6"
> >+	depends on !BR2_powerpc64le
> >+	depends on BR2_ARCH_IS_64
> >  	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
Cédric Le Goater July 30, 2022, 9:18 p.m. UTC | #4 
On 7/29/22 23:37, Yann E. MORIN wrote:
> Cédric, Joel, All,
> 
> On 2022-07-29 08:52 +0200, Cédric Le Goater spake thusly:
>> On 7/29/22 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.
>> I don't think we care much but Power7 could run also in LE.
> 
> Joel seemed to state that only the power8 can be LE, so who's right? ;-)
>
> In any case, if maore variants turn out to be LE-compatible, it is still
> possible to enable htem in followup patches.

Honestly we don't really care about Power7/LE. It never came out of
the lab. Power8 was the first released, let's keep it that way.

C.
Yann E. MORIN July 31, 2022, 6:43 a.m. UTC | #5 
Cédric, All,

On 2022-07-30 23:18 +0200, Cédric Le Goater spake thusly:
> On 7/29/22 23:37, Yann E. MORIN wrote:
> >On 2022-07-29 08:52 +0200, Cédric Le Goater spake thusly:
> >>On 7/29/22 02:08, Joel Stanley wrote:
> >>>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.
> >>I don't think we care much but Power7 could run also in LE.
> >Joel seemed to state that only the power8 can be LE, so who's right? ;-)
> Honestly we don't really care about Power7/LE. It never came out of
> the lab. Power8 was the first released, let's keep it that way.

That's perfectly fine with me. Thanks!

Regards,
Yann E. MORIN.
Thomas Petazzoni Aug. 3, 2022, 9:32 p.m. UTC | #6 
Hello,

Can we try to use positive logic here? See below what I mean.

On Fri, 29 Jul 2022 09:38:58 +0930
Joel Stanley <joel@jms.id.au> wrote:

> 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

So powerpc_620 is usable as PowerPC 32-bit, or PowerPC 64-bit BE ? If
so, then:

	depends on BR2_powerpc || BR2_powerpc64

>  config BR2_powerpc_630
>  	bool "630"
> -	# No C library supports this variant on ppc64le
>  	depends on !BR2_powerpc64le

Ditto.

>  config BR2_powerpc_740
>  	bool "740"
> @@ -102,6 +100,7 @@ config BR2_powerpc_860
>  	depends on !BR2_ARCH_IS_64

Ditto.

>  config BR2_powerpc_970
>  	bool "970"
> +	depends on !BR2_powerpc64le

Ditto.

>  	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

And here:

	depends on BR2_powerpc64

>  config BR2_powerpc_power6
>  	bool "power6"
> +	depends on !BR2_powerpc64le
> +	depends on BR2_ARCH_IS_64

Ditto:

	depends on BR2_powerpc64

>  	select BR2_POWERPC_CPU_HAS_ALTIVEC
>  config BR2_powerpc_power7
>  	bool "power7"
> +	depends on !BR2_powerpc64le
> +	depends on BR2_ARCH_IS_64

Ditto:

	depends on BR2_powerpc64

>  	select BR2_POWERPC_CPU_HAS_ALTIVEC
>  	select BR2_POWERPC_CPU_HAS_VSX
>  config BR2_powerpc_power8
>  	bool "power8"
> +	depends on BR2_ARCH_IS_64

And hgere:

	depends on BR2_powerpc64 || BR2_powerpc64le

It would be useful to do that in a consistent way for all PowerPC cores
in this choice, so that it is very clear for which of powerpc,
powerpc64 and powerpc64le each PowerPC core is valid.

Thanks!

Thomas
Arnout Vandecappelle Sept. 18, 2022, 10:23 a.m. UTC | #7 
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 and I believe it's even 
possible to run a 64-bit kernel with 32-bit userspace like on ARM. Is this not true?

  If POWER5+ really can't run in 32-bit mode (or this is not supported by the 
kernel), are you sure that this *is* possible on POWER4?

  In either case, if the current conditions are not correct, please send a 
follow-up patch to fix them even more.


>   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.

  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
Arnout Vandecappelle Sept. 18, 2022, 10:24 a.m. UTC | #8 
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.

  One more thing: powerpc_generic still has that comment, shouldn't it be 
removed there as well?

  Regards,
  Arnout

> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
[snip]
Cédric Le Goater Sept. 20, 2022, 5:09 p.m. UTC | #9 
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.

> 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

> are you sure that this *is* possible on POWER4?

POWER4 support was removed from Linux ~4 years ago.
  
>   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
Arnout Vandecappelle Sept. 20, 2022, 8:32 p.m. UTC | #10 
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
>
diff mbox series

Patch

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
 	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
 config BR2_powerpc_power6
 	bool "power6"
+	depends on !BR2_powerpc64le
+	depends on BR2_ARCH_IS_64
 	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