[PATCH] tests: Build correct sandbox configuration on 32bit

[PATCH] tests: Build correct sandbox configuration on 32bit

[Michal Suchanek]

Currently sandbox configuration defautls to 64bit and there is no
automation for building 32bit sandbox on 32bit hosts.

cpp does not know about target specification, code needs to be compiled
to determine integer width.

Add a test program that prints the integer width, and a make target that
aligns the sandbox configuration with the result.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---

 Makefile              |  6 ++++++
 doc/arch/sandbox.rst  | 16 +++++++++++-----
 test/py/conftest.py   |  1 +
 tools/Makefile        |  2 ++
 tools/bits-per-long.c | 14 ++++++++++++++
 5 files changed, 34 insertions(+), 5 deletions(-)
 create mode 100644 tools/bits-per-long.c

diff --git a/Makefile b/Makefile
index 3866cc62f9..e5463573f3 100644
--- a/Makefile
+++ b/Makefile
@@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
 cross_tools: export CROSS_BUILD_TOOLS=y
 cross_tools: tools ;
 
+PHONY += set_host_bits
+set_host_bits: tools
+	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
+	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
+	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
+
 .PHONY : CHANGELOG
 CHANGELOG:
 	git log --no-merges U-Boot-1_1_5.. | \
diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
index 068d4a3be4..d751205eba 100644
--- a/doc/arch/sandbox.rst
+++ b/doc/arch/sandbox.rst
@@ -33,9 +33,11 @@ machines.
 
 There are two versions of the sandbox: One using 32-bit-wide integers, and one
 using 64-bit-wide integers. The 32-bit version can be build and run on either
-32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
-default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
-integers can only be built on 64-bit hosts.
+32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
+default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
+integers can only be built on 64-bit hosts. There is no automation for ensuring
+32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
+config.
 
 Note that standalone/API support is not available at present.
 
@@ -51,7 +53,9 @@ Basic Operation
 
 To run sandbox U-Boot use something like::
 
-   make sandbox_defconfig all
+   make sandbox_defconfig
+   make set_host_bits
+   make all
    ./u-boot
 
 Note: If you get errors about 'sdl-config: Command not found' you may need to
@@ -59,7 +63,9 @@ install libsdl2.0-dev or similar to get SDL support. Alternatively you can
 build sandbox without SDL (i.e. no display/keyboard support) by removing
 the CONFIG_SANDBOX_SDL line in include/configs/sandbox.h or using::
 
-   make sandbox_defconfig all NO_SDL=1
+   make sandbox_defconfig
+   make set_host_bits
+   make all NO_SDL=1
    ./u-boot
 
 U-Boot will start on your computer, showing a sandbox emulation of the serial
diff --git a/test/py/conftest.py b/test/py/conftest.py
index 304e93164a..3d1fd6883a 100644
--- a/test/py/conftest.py
+++ b/test/py/conftest.py
@@ -104,6 +104,7 @@ def run_build(config, source_dir, build_dir, board_type, log):
             o_opt = ''
         cmds = (
             ['make', o_opt, '-s', board_type + '_defconfig'],
+            ['make', o_opt, '-s', 'set_host_bits'],
             ['make', o_opt, '-s', '-j{}'.format(os.cpu_count())],
         )
         name = 'make'
diff --git a/tools/Makefile b/tools/Makefile
index 34a1aa7a8b..d6b585953d 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -68,6 +68,8 @@ HOSTCFLAGS_img2srec.o := -pedantic
 hostprogs-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes
 HOSTCFLAGS_xway-swap-bytes.o := -pedantic
 
+hostprogs-y += bits-per-long
+
 hostprogs-y += mkenvimage
 mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
 
diff --git a/tools/bits-per-long.c b/tools/bits-per-long.c
new file mode 100644
index 0000000000..7630e1623f
--- /dev/null
+++ b/tools/bits-per-long.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+
+int main(int argc, char **argv)
+{
+	unsigned long testvar = ~0UL;
+	unsigned int i;
+
+	for (i = 0; testvar; i++, testvar >>= 1)
+		;
+
+	return printf("%u\n", i);
+}
+
-- 
2.37.3

Re: [PATCH] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/13/22 22:28, Michal Suchanek wrote:
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> cpp does not know about target specification, code needs to be compiled
> to determine integer width.
>
> Add a test program that prints the integer width, and a make target that
> aligns the sandbox configuration with the result.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>
>   Makefile              |  6 ++++++
>   doc/arch/sandbox.rst  | 16 +++++++++++-----
>   test/py/conftest.py   |  1 +
>   tools/Makefile        |  2 ++
>   tools/bits-per-long.c | 14 ++++++++++++++
>   5 files changed, 34 insertions(+), 5 deletions(-)
>   create mode 100644 tools/bits-per-long.c
>
> diff --git a/Makefile b/Makefile
> index 3866cc62f9..e5463573f3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
>   cross_tools: export CROSS_BUILD_TOOLS=y
>   cross_tools: tools ;
>
> +PHONY += set_host_bits
> +set_host_bits: tools
> +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
> +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
> +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
> +
>   .PHONY : CHANGELOG
>   CHANGELOG:
>   	git log --no-merges U-Boot-1_1_5.. | \
> diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
> index 068d4a3be4..d751205eba 100644
> --- a/doc/arch/sandbox.rst
> +++ b/doc/arch/sandbox.rst
> @@ -33,9 +33,11 @@ machines.
>
>   There are two versions of the sandbox: One using 32-bit-wide integers, and one
>   using 64-bit-wide integers. The 32-bit version can be build and run on either
> -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
> -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
> -integers can only be built on 64-bit hosts.
> +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
> +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
> +integers can only be built on 64-bit hosts. There is no automation for ensuring
> +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
> +config.
>
>   Note that standalone/API support is not available at present.
>
> @@ -51,7 +53,9 @@ Basic Operation
>
>   To run sandbox U-Boot use something like::
>
> -   make sandbox_defconfig all
> +   make sandbox_defconfig
> +   make set_host_bits
> +   make all

Thanks for addressing the problem of sandbox bitness.

We should not make building the sandbox more complicated. You could
integrate building set_host_bits into an existing target like u-boot.cfg:.

Overall an approach with an external program is too complicated.
CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
CONFIG_SANDBOX_BITS_PER_LONG.

We could add

#ifndef __LP64__
#undef SANDBOX_BITS_PER_LONG
#define SANDBOX_BITS_PER_LONG 32
#endif

to the top of arch/sandbox/include/asm/posix_types.h and use

#if defined(CONFIG_HOST_64BIT) && defined(__LP64__)

in drivers/misc/swap_case.c to solve the problem. This demonstrates that
CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.

Eliminating them and only using __LP64__ is the right approach.

@Simon:
We should add sandbox_defconfig built with -m32 to our Gitlab CI testing
after fixing the incompatibilities in the unit tests.

>      ./u-boot
>
>   Note: If you get errors about 'sdl-config: Command not found' you may need to
> @@ -59,7 +63,9 @@ install libsdl2.0-dev or similar to get SDL support. Alternatively you can
>   build sandbox without SDL (i.e. no display/keyboard support) by removing
>   the CONFIG_SANDBOX_SDL line in include/configs/sandbox.h or using::
>
> -   make sandbox_defconfig all NO_SDL=1
> +   make sandbox_defconfig
> +   make set_host_bits
> +   make all NO_SDL=1
>      ./u-boot
>
>   U-Boot will start on your computer, showing a sandbox emulation of the serial
> diff --git a/test/py/conftest.py b/test/py/conftest.py
> index 304e93164a..3d1fd6883a 100644
> --- a/test/py/conftest.py
> +++ b/test/py/conftest.py
> @@ -104,6 +104,7 @@ def run_build(config, source_dir, build_dir, board_type, log):
>               o_opt = ''
>           cmds = (
>               ['make', o_opt, '-s', board_type + '_defconfig'],
> +            ['make', o_opt, '-s', 'set_host_bits'],
>               ['make', o_opt, '-s', '-j{}'.format(os.cpu_count())],
>           )
>           name = 'make'
> diff --git a/tools/Makefile b/tools/Makefile
> index 34a1aa7a8b..d6b585953d 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -68,6 +68,8 @@ HOSTCFLAGS_img2srec.o := -pedantic
>   hostprogs-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes
>   HOSTCFLAGS_xway-swap-bytes.o := -pedantic
>
> +hostprogs-y += bits-per-long
> +
>   hostprogs-y += mkenvimage
>   mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
>
> diff --git a/tools/bits-per-long.c b/tools/bits-per-long.c
> new file mode 100644
> index 0000000000..7630e1623f
> --- /dev/null
> +++ b/tools/bits-per-long.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +
> +int main(int argc, char **argv)
> +{
> +	unsigned long testvar = ~0UL;
> +	unsigned int i;
> +
> +	for (i = 0; testvar; i++, testvar >>= 1)
> +		;
> +
> +	return printf("%u\n", i);

return printf("%zd\n", 8 * sizeof(long));

> +}
> +

Please avoid empty lines at the end files.

Best regards

Heinrich

Re: [PATCH] tests: Build correct sandbox configuration on 32bit

[Michal Suchánek]

On Fri, Oct 14, 2022 at 05:05:26AM +0200, Heinrich Schuchardt wrote:
> On 10/13/22 22:28, Michal Suchanek wrote:
> > Currently sandbox configuration defautls to 64bit and there is no
> > automation for building 32bit sandbox on 32bit hosts.
> > 
> > cpp does not know about target specification, code needs to be compiled
> > to determine integer width.
> > 
> > Add a test program that prints the integer width, and a make target that
> > aligns the sandbox configuration with the result.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > 
> >   Makefile              |  6 ++++++
> >   doc/arch/sandbox.rst  | 16 +++++++++++-----
> >   test/py/conftest.py   |  1 +
> >   tools/Makefile        |  2 ++
> >   tools/bits-per-long.c | 14 ++++++++++++++
> >   5 files changed, 34 insertions(+), 5 deletions(-)
> >   create mode 100644 tools/bits-per-long.c
> > 
> > diff --git a/Makefile b/Makefile
> > index 3866cc62f9..e5463573f3 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
> >   cross_tools: export CROSS_BUILD_TOOLS=y
> >   cross_tools: tools ;
> > 
> > +PHONY += set_host_bits
> > +set_host_bits: tools
> > +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
> > +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
> > +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
> > +
> >   .PHONY : CHANGELOG
> >   CHANGELOG:
> >   	git log --no-merges U-Boot-1_1_5.. | \
> > diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
> > index 068d4a3be4..d751205eba 100644
> > --- a/doc/arch/sandbox.rst
> > +++ b/doc/arch/sandbox.rst
> > @@ -33,9 +33,11 @@ machines.
> > 
> >   There are two versions of the sandbox: One using 32-bit-wide integers, and one
> >   using 64-bit-wide integers. The 32-bit version can be build and run on either
> > -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
> > -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
> > -integers can only be built on 64-bit hosts.
> > +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
> > +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
> > +integers can only be built on 64-bit hosts. There is no automation for ensuring
> > +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
> > +config.
> > 
> >   Note that standalone/API support is not available at present.
> > 
> > @@ -51,7 +53,9 @@ Basic Operation
> > 
> >   To run sandbox U-Boot use something like::
> > 
> > -   make sandbox_defconfig all
> > +   make sandbox_defconfig
> > +   make set_host_bits
> > +   make all
> 
> Thanks for addressing the problem of sandbox bitness.
> 
> We should not make building the sandbox more complicated. You could
> integrate building set_host_bits into an existing target like u-boot.cfg:.
> 
> Overall an approach with an external program is too complicated.
> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
> CONFIG_SANDBOX_BITS_PER_LONG.
And for making SANDBOX64 depend on 64bit build.
> 
> We could add
> 
> #ifndef __LP64__
> #undef SANDBOX_BITS_PER_LONG
> #define SANDBOX_BITS_PER_LONG 32
> #endif

If we are willing to depend on this define which is clearly named as
compiler-internal we could do similar to cc-option to run something like
$(CC) -dM -E - < /dev/null | grep -q _LP64

> 
> to the top of arch/sandbox/include/asm/posix_types.h and use
> 
> #if defined(CONFIG_HOST_64BIT) && defined(__LP64__)
> 
> in drivers/misc/swap_case.c to solve the problem. This demonstrates that
> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.

Not really.

Thanks

Michal

Re: [PATCH] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi Michal,

On Thu, 13 Oct 2022 at 14:29, Michal Suchanek <msuchanek@suse.de> wrote:
>
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> cpp does not know about target specification, code needs to be compiled
> to determine integer width.
>
> Add a test program that prints the integer width, and a make target that
> aligns the sandbox configuration with the result.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>
>  Makefile              |  6 ++++++
>  doc/arch/sandbox.rst  | 16 +++++++++++-----
>  test/py/conftest.py   |  1 +
>  tools/Makefile        |  2 ++
>  tools/bits-per-long.c | 14 ++++++++++++++
>  5 files changed, 34 insertions(+), 5 deletions(-)
>  create mode 100644 tools/bits-per-long.c

This needs to be automatic, so that it builds the 32-bit version on
32-bit hosts, 64-bit version on 64-bit hosts.

See here for my attempt. I suspect it just needs your bits_per_long
thing brought in, but in any case I hope it gives you inspiration.

https://patchwork.ozlabs.org/project/uboot/patch/20220123195514.3152022-4-sjg@chromium.org/

Basically we should be able to build sandbox on any platform and it
should just work, without manual configuration.

Regards,
Simon

[PATCH v2] tests: Build correct sandbox configuration on 32bit

[Michal Suchanek]

Currently sandbox configuration defautls to 64bit and there is no
automation for building 32bit sandbox on 32bit hosts.

Use _LP64 macro as heuristic for detecting 64bit targets.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---

Changes in v2:
simplify and move detection to kconfig

---
 arch/sandbox/Kconfig    | 18 +++---------------
 scripts/Kconfig.include |  4 ++++
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
index 852a7c8bf2..35508c6b29 100644
--- a/arch/sandbox/Kconfig
+++ b/arch/sandbox/Kconfig
@@ -13,7 +13,7 @@ config SYS_CPU
 config SANDBOX64
 	bool "Use 64-bit addresses"
 	select PHYS_64BIT
-	select HOST_64BIT
+	depends on HOST_64BIT
 
 config SANDBOX_RAM_SIZE_MB
 	int "RAM size in MiB"
@@ -41,23 +41,11 @@ config SYS_CONFIG_NAME
 	default "sandbox_spl" if SANDBOX_SPL
 	default "sandbox" if !SANDBOX_SPL
 
-choice
-	prompt "Run sandbox on 32/64-bit host"
-	default HOST_64BIT
-	help
-	  Sandbox can be built on 32-bit and 64-bit hosts.
-	  The default is to build on a 64-bit host and run
-	  on a 64-bit host. If you want to run sandbox on
-	  a 32-bit host, change it here.
-
 config HOST_32BIT
-	bool "32-bit host"
-	depends on !PHYS_64BIT
+	def_bool ! $(cc-define,_LP64)
 
 config HOST_64BIT
-	bool "64-bit host"
-
-endchoice
+	def_bool $(cc-define,_LP64)
 
 config SANDBOX_CRASH_RESET
 	bool "Reset on crash"
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index dad5583451..b7598ca5d9 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -22,6 +22,10 @@ success = $(if-success,$(1),y,n)
 # Return y if the compiler supports <flag>, n otherwise
 cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
 
+# $(cc-define,<macro>)
+# Return y if the compiler defines <macro>, n otherwise
+cc-define = $(success,$(CC) -dM -E -x c /dev/null | grep -q '^#define \<$(1)\>')
+
 # $(ld-option,<flag>)
 # Return y if the linker supports <flag>, n otherwise
 ld-option = $(success,$(LD) -v $(1))
-- 
2.37.3

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/14/22 22:52, Michal Suchanek wrote:
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> Use _LP64 macro as heuristic for detecting 64bit targets.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>

Please, explain why you think a Kconfig level patch is preferable to
what I proposed in

[PATCH] sandbox: Eliminate CONFIG_HOST_32/64BIT
https://lists.denx.de/pipermail/u-boot/2022-October/497236.html

> ---
>
> Changes in v2:
> simplify and move detection to kconfig
>
> ---
>   arch/sandbox/Kconfig    | 18 +++---------------
>   scripts/Kconfig.include |  4 ++++
>   2 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
> index 852a7c8bf2..35508c6b29 100644
> --- a/arch/sandbox/Kconfig
> +++ b/arch/sandbox/Kconfig
> @@ -13,7 +13,7 @@ config SYS_CPU
>   config SANDBOX64
>   	bool "Use 64-bit addresses"
>   	select PHYS_64BIT
> -	select HOST_64BIT
> +	depends on HOST_64BIT
>
>   config SANDBOX_RAM_SIZE_MB
>   	int "RAM size in MiB"
> @@ -41,23 +41,11 @@ config SYS_CONFIG_NAME
>   	default "sandbox_spl" if SANDBOX_SPL
>   	default "sandbox" if !SANDBOX_SPL
>
> -choice
> -	prompt "Run sandbox on 32/64-bit host"
> -	default HOST_64BIT
> -	help
> -	  Sandbox can be built on 32-bit and 64-bit hosts.
> -	  The default is to build on a 64-bit host and run
> -	  on a 64-bit host. If you want to run sandbox on
> -	  a 32-bit host, change it here.
> -
>   config HOST_32BIT
> -	bool "32-bit host"
> -	depends on !PHYS_64BIT
> +	def_bool ! $(cc-define,_LP64)
>
>   config HOST_64BIT
> -	bool "64-bit host"
> -
> -endchoice
> +	def_bool $(cc-define,_LP64)
>
>   config SANDBOX_CRASH_RESET
>   	bool "Reset on crash"
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index dad5583451..b7598ca5d9 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include

This include is copied from Linux. From time to time we synchronize the
Kconfig framework from Linux. So we should avoid U-Boot specific changes
here.

Best regards

Heinrich

> @@ -22,6 +22,10 @@ success = $(if-success,$(1),y,n)
>   # Return y if the compiler supports <flag>, n otherwise
>   cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
>
> +# $(cc-define,<macro>)
> +# Return y if the compiler defines <macro>, n otherwise
> +cc-define = $(success,$(CC) -dM -E -x c /dev/null | grep -q '^#define \<$(1)\>')
> +
>   # $(ld-option,<flag>)
>   # Return y if the linker supports <flag>, n otherwise
>   ld-option = $(success,$(LD) -v $(1))

Re: [PATCH] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/14/22 10:43, Michal Suchánek wrote:
> On Fri, Oct 14, 2022 at 05:05:26AM +0200, Heinrich Schuchardt wrote:
>> On 10/13/22 22:28, Michal Suchanek wrote:
>>> Currently sandbox configuration defautls to 64bit and there is no
>>> automation for building 32bit sandbox on 32bit hosts.
>>>
>>> cpp does not know about target specification, code needs to be compiled
>>> to determine integer width.
>>>
>>> Add a test program that prints the integer width, and a make target that
>>> aligns the sandbox configuration with the result.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>>
>>>    Makefile              |  6 ++++++
>>>    doc/arch/sandbox.rst  | 16 +++++++++++-----
>>>    test/py/conftest.py   |  1 +
>>>    tools/Makefile        |  2 ++
>>>    tools/bits-per-long.c | 14 ++++++++++++++
>>>    5 files changed, 34 insertions(+), 5 deletions(-)
>>>    create mode 100644 tools/bits-per-long.c
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 3866cc62f9..e5463573f3 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
>>>    cross_tools: export CROSS_BUILD_TOOLS=y
>>>    cross_tools: tools ;
>>>
>>> +PHONY += set_host_bits
>>> +set_host_bits: tools
>>> +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
>>> +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
>>> +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
>>> +
>>>    .PHONY : CHANGELOG
>>>    CHANGELOG:
>>>    	git log --no-merges U-Boot-1_1_5.. | \
>>> diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
>>> index 068d4a3be4..d751205eba 100644
>>> --- a/doc/arch/sandbox.rst
>>> +++ b/doc/arch/sandbox.rst
>>> @@ -33,9 +33,11 @@ machines.
>>>
>>>    There are two versions of the sandbox: One using 32-bit-wide integers, and one
>>>    using 64-bit-wide integers. The 32-bit version can be build and run on either
>>> -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
>>> -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
>>> -integers can only be built on 64-bit hosts.
>>> +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
>>> +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
>>> +integers can only be built on 64-bit hosts. There is no automation for ensuring
>>> +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
>>> +config.
>>>
>>>    Note that standalone/API support is not available at present.
>>>
>>> @@ -51,7 +53,9 @@ Basic Operation
>>>
>>>    To run sandbox U-Boot use something like::
>>>
>>> -   make sandbox_defconfig all
>>> +   make sandbox_defconfig
>>> +   make set_host_bits
>>> +   make all
>>
>> Thanks for addressing the problem of sandbox bitness.
>>
>> We should not make building the sandbox more complicated. You could
>> integrate building set_host_bits into an existing target like u-boot.cfg:.
>>
>> Overall an approach with an external program is too complicated.
>> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
>> CONFIG_SANDBOX_BITS_PER_LONG.
> And for making SANDBOX64 depend on 64bit build.

Sandbox64 is about the width of phys_addr_t and not about the bitness of
the build. sandbox64_defconfig builds fine on ilp32 and many aspects
work there.

We should test that 64bit phys_addr_t works on ilp32 systems. Sandbox64
would we the right way to do this.

Best regards

Heinrich

>>
>> We could add
>>
>> #ifndef __LP64__
>> #undef SANDBOX_BITS_PER_LONG
>> #define SANDBOX_BITS_PER_LONG 32
>> #endif
>
> If we are willing to depend on this define which is clearly named as
> compiler-internal we could do similar to cc-option to run something like
> $(CC) -dM -E - < /dev/null | grep -q _LP64
>
>>
>> to the top of arch/sandbox/include/asm/posix_types.h and use
>>
>> #if defined(CONFIG_HOST_64BIT) && defined(__LP64__)
>>
>> in drivers/misc/swap_case.c to solve the problem. This demonstrates that
>> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.
>
> Not really.
>
> Thanks
>
> Michal

Re: [PATCH] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/14/22 17:56, Simon Glass wrote:
> Hi Michal,
>
> On Thu, 13 Oct 2022 at 14:29, Michal Suchanek <msuchanek@suse.de> wrote:
>>
>> Currently sandbox configuration defautls to 64bit and there is no
>> automation for building 32bit sandbox on 32bit hosts.
>>
>> cpp does not know about target specification, code needs to be compiled
>> to determine integer width.
>>
>> Add a test program that prints the integer width, and a make target that
>> aligns the sandbox configuration with the result.
>>
>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> ---
>>
>>   Makefile              |  6 ++++++
>>   doc/arch/sandbox.rst  | 16 +++++++++++-----
>>   test/py/conftest.py   |  1 +
>>   tools/Makefile        |  2 ++
>>   tools/bits-per-long.c | 14 ++++++++++++++
>>   5 files changed, 34 insertions(+), 5 deletions(-)
>>   create mode 100644 tools/bits-per-long.c
>
> This needs to be automatic, so that it builds the 32-bit version on
> 32-bit hosts, 64-bit version on 64-bit hosts.

We should be able to test the following:

* 64bit phys_addr_t on ilp32.
* 32bit phys_addr_t on ilp32.
* 64bit phys_addr_t on lp64.

Gitlab CI currently tests:

* 64bit phys_addr_t on lp64.
* 32bit phys_addr_t on lp64.

Best regards

Heinrich

>
> See here for my attempt. I suspect it just needs your bits_per_long
> thing brought in, but in any case I hope it gives you inspiration.
>
> https://patchwork.ozlabs.org/project/uboot/patch/20220123195514.3152022-4-sjg@chromium.org/
>
> Basically we should be able to build sandbox on any platform and it
> should just work, without manual configuration.
>
> Regards,
> Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Michal Suchánek]

On Sat, Oct 15, 2022 at 06:54:02AM +0200, Heinrich Schuchardt wrote:
> On 10/14/22 22:52, Michal Suchanek wrote:
> > Currently sandbox configuration defautls to 64bit and there is no
> > automation for building 32bit sandbox on 32bit hosts.
> > 
> > Use _LP64 macro as heuristic for detecting 64bit targets.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> 
> Please, explain why you think a Kconfig level patch is preferable to
> what I proposed in
> 
> [PATCH] sandbox: Eliminate CONFIG_HOST_32/64BIT
> https://lists.denx.de/pipermail/u-boot/2022-October/497236.html

The existing dependency canot be described when the option is
eliminated:

>   config SANDBOX64
>       bool "Use 64-bit addresses"
>       select PHYS_64BIT
> -     select HOST_64BIT
> +     depends on HOST_64BIT

If we can run SANDBOX64 on 32bit eliminating the option is fine.

Thanks

Michal

> 
> > ---
> > 
> > Changes in v2:
> > simplify and move detection to kconfig
> > 
> > ---
> >   arch/sandbox/Kconfig    | 18 +++---------------
> >   scripts/Kconfig.include |  4 ++++
> >   2 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
> > index 852a7c8bf2..35508c6b29 100644
> > --- a/arch/sandbox/Kconfig
> > +++ b/arch/sandbox/Kconfig
> > @@ -13,7 +13,7 @@ config SYS_CPU
> >   config SANDBOX64
> >   	bool "Use 64-bit addresses"
> >   	select PHYS_64BIT
> > -	select HOST_64BIT
> > +	depends on HOST_64BIT
> > 
> >   config SANDBOX_RAM_SIZE_MB
> >   	int "RAM size in MiB"
> > @@ -41,23 +41,11 @@ config SYS_CONFIG_NAME
> >   	default "sandbox_spl" if SANDBOX_SPL
> >   	default "sandbox" if !SANDBOX_SPL
> > 
> > -choice
> > -	prompt "Run sandbox on 32/64-bit host"
> > -	default HOST_64BIT
> > -	help
> > -	  Sandbox can be built on 32-bit and 64-bit hosts.
> > -	  The default is to build on a 64-bit host and run
> > -	  on a 64-bit host. If you want to run sandbox on
> > -	  a 32-bit host, change it here.
> > -
> >   config HOST_32BIT
> > -	bool "32-bit host"
> > -	depends on !PHYS_64BIT
> > +	def_bool ! $(cc-define,_LP64)
> > 
> >   config HOST_64BIT
> > -	bool "64-bit host"
> > -
> > -endchoice
> > +	def_bool $(cc-define,_LP64)
> > 
> >   config SANDBOX_CRASH_RESET
> >   	bool "Reset on crash"
> > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> > index dad5583451..b7598ca5d9 100644
> > --- a/scripts/Kconfig.include
> > +++ b/scripts/Kconfig.include
> 
> This include is copied from Linux. From time to time we synchronize the
> Kconfig framework from Linux. So we should avoid U-Boot specific changes
> here.
> 
> Best regards
> 
> Heinrich
> 
> > @@ -22,6 +22,10 @@ success = $(if-success,$(1),y,n)
> >   # Return y if the compiler supports <flag>, n otherwise
> >   cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
> > 
> > +# $(cc-define,<macro>)
> > +# Return y if the compiler defines <macro>, n otherwise
> > +cc-define = $(success,$(CC) -dM -E -x c /dev/null | grep -q '^#define \<$(1)\>')
> > +
> >   # $(ld-option,<flag>)
> >   # Return y if the linker supports <flag>, n otherwise
> >   ld-option = $(success,$(LD) -v $(1))
> 

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi Michal,

On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> Use _LP64 macro as heuristic for detecting 64bit targets.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>
> Changes in v2:
> simplify and move detection to kconfig
>
> ---
>  arch/sandbox/Kconfig    | 18 +++---------------
>  scripts/Kconfig.include |  4 ++++
>  2 files changed, 7 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

My only question is whether we can allow building the 32-bit version
on a 64-bit machine? That would need a separate option I think, to
say:

I don't want you to automatically determine HOST_32/64BIT. Instead,
use 32 (or 64).

This is along the lines of what Heinrich is saying, except that I
strongly feel that we must do the right thing by default, as your
patch does.

>
> diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
> index 852a7c8bf2..35508c6b29 100644
> --- a/arch/sandbox/Kconfig
> +++ b/arch/sandbox/Kconfig
> @@ -13,7 +13,7 @@ config SYS_CPU
>  config SANDBOX64
>         bool "Use 64-bit addresses"
>         select PHYS_64BIT
> -       select HOST_64BIT
> +       depends on HOST_64BIT
>
>  config SANDBOX_RAM_SIZE_MB
>         int "RAM size in MiB"
> @@ -41,23 +41,11 @@ config SYS_CONFIG_NAME
>         default "sandbox_spl" if SANDBOX_SPL
>         default "sandbox" if !SANDBOX_SPL
>
> -choice
> -       prompt "Run sandbox on 32/64-bit host"
> -       default HOST_64BIT
> -       help
> -         Sandbox can be built on 32-bit and 64-bit hosts.
> -         The default is to build on a 64-bit host and run
> -         on a 64-bit host. If you want to run sandbox on
> -         a 32-bit host, change it here.
> -
>  config HOST_32BIT
> -       bool "32-bit host"
> -       depends on !PHYS_64BIT
> +       def_bool ! $(cc-define,_LP64)
>
>  config HOST_64BIT
> -       bool "64-bit host"
> -
> -endchoice
> +       def_bool $(cc-define,_LP64)
>
>  config SANDBOX_CRASH_RESET
>         bool "Reset on crash"
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index dad5583451..b7598ca5d9 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -22,6 +22,10 @@ success = $(if-success,$(1),y,n)
>  # Return y if the compiler supports <flag>, n otherwise
>  cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
>
> +# $(cc-define,<macro>)
> +# Return y if the compiler defines <macro>, n otherwise
> +cc-define = $(success,$(CC) -dM -E -x c /dev/null | grep -q '^#define \<$(1)\>')
> +
>  # $(ld-option,<flag>)
>  # Return y if the linker supports <flag>, n otherwise
>  ld-option = $(success,$(LD) -v $(1))
> --
> 2.37.3
>

Regards,
SImon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/15/22 19:53, Simon Glass wrote:
> Hi Michal,
>
> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>>
>> Currently sandbox configuration defautls to 64bit and there is no
>> automation for building 32bit sandbox on 32bit hosts.
>>
>> Use _LP64 macro as heuristic for detecting 64bit targets.
>>
>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> ---
>>
>> Changes in v2:
>> simplify and move detection to kconfig
>>
>> ---
>>   arch/sandbox/Kconfig    | 18 +++---------------
>>   scripts/Kconfig.include |  4 ++++
>>   2 files changed, 7 insertions(+), 15 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> My only question is whether we can allow building the 32-bit version
> on a 64-bit machine? That would need a separate option I think, to
> say:
>
> I don't want you to automatically determine HOST_32/64BIT. Instead,
> use 32 (or 64).
>
> This is along the lines of what Heinrich is saying, except that I
> strongly feel that we must do the right thing by default, as your
> patch does.

The whole point of phys_addr_t and phys_size_t is that it can be 64bit
or 32bit on ilp32.

With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
that is bad.

32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
what we currently test with sandbox_defconfig on Gitlab CI.

My patch is ending up in the same behavior as Michal's patch except that
it allows to have 64bit phys_addr_t on ilp32.

>
>>
>> diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
>> index 852a7c8bf2..35508c6b29 100644
>> --- a/arch/sandbox/Kconfig
>> +++ b/arch/sandbox/Kconfig
>> @@ -13,7 +13,7 @@ config SYS_CPU
>>   config SANDBOX64
>>          bool "Use 64-bit addresses"
>>          select PHYS_64BIT
>> -       select HOST_64BIT
>> +       depends on HOST_64BIT

This line is utterly wrong.

Best regards

Heinrich

>>
>>   config SANDBOX_RAM_SIZE_MB
>>          int "RAM size in MiB"
>> @@ -41,23 +41,11 @@ config SYS_CONFIG_NAME
>>          default "sandbox_spl" if SANDBOX_SPL
>>          default "sandbox" if !SANDBOX_SPL
>>
>> -choice
>> -       prompt "Run sandbox on 32/64-bit host"
>> -       default HOST_64BIT
>> -       help
>> -         Sandbox can be built on 32-bit and 64-bit hosts.
>> -         The default is to build on a 64-bit host and run
>> -         on a 64-bit host. If you want to run sandbox on
>> -         a 32-bit host, change it here.
>> -
>>   config HOST_32BIT
>> -       bool "32-bit host"
>> -       depends on !PHYS_64BIT
>> +       def_bool ! $(cc-define,_LP64)
>>
>>   config HOST_64BIT
>> -       bool "64-bit host"
>> -
>> -endchoice
>> +       def_bool $(cc-define,_LP64)
>>
>>   config SANDBOX_CRASH_RESET
>>          bool "Reset on crash"
>> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
>> index dad5583451..b7598ca5d9 100644
>> --- a/scripts/Kconfig.include
>> +++ b/scripts/Kconfig.include
>> @@ -22,6 +22,10 @@ success = $(if-success,$(1),y,n)
>>   # Return y if the compiler supports <flag>, n otherwise
>>   cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
>>
>> +# $(cc-define,<macro>)
>> +# Return y if the compiler defines <macro>, n otherwise
>> +cc-define = $(success,$(CC) -dM -E -x c /dev/null | grep -q '^#define \<$(1)\>')
>> +
>>   # $(ld-option,<flag>)
>>   # Return y if the linker supports <flag>, n otherwise
>>   ld-option = $(success,$(LD) -v $(1))
>> --
>> 2.37.3
>>
>
> Regards,
> SImon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi Heinrich,

On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/15/22 19:53, Simon Glass wrote:
> > Hi Michal,
> >
> > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> >>
> >> Currently sandbox configuration defautls to 64bit and there is no
> >> automation for building 32bit sandbox on 32bit hosts.
> >>
> >> Use _LP64 macro as heuristic for detecting 64bit targets.
> >>
> >> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >> ---
> >>
> >> Changes in v2:
> >> simplify and move detection to kconfig
> >>
> >> ---
> >>   arch/sandbox/Kconfig    | 18 +++---------------
> >>   scripts/Kconfig.include |  4 ++++
> >>   2 files changed, 7 insertions(+), 15 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > My only question is whether we can allow building the 32-bit version
> > on a 64-bit machine? That would need a separate option I think, to
> > say:
> >
> > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > use 32 (or 64).
> >
> > This is along the lines of what Heinrich is saying, except that I
> > strongly feel that we must do the right thing by default, as your
> > patch does.
>
> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> or 32bit on ilp32.
>
> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> that is bad.
>
> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> what we currently test with sandbox_defconfig on Gitlab CI.
>
> My patch is ending up in the same behavior as Michal's patch except that
> it allows to have 64bit phys_addr_t on ilp32.

It needs to automatically default to 32 or 64 bit depending on the
host. If the user wants to fiddle with Kconfig to force it to the
other one, that should be possible to.

It looks like your patch requires manual configuration, but perhaps I
just misunderstood it?

[..]

Regards,
Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/15/22 20:39, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/15/22 19:53, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>>>>
>>>> Currently sandbox configuration defautls to 64bit and there is no
>>>> automation for building 32bit sandbox on 32bit hosts.
>>>>
>>>> Use _LP64 macro as heuristic for detecting 64bit targets.
>>>>
>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> simplify and move detection to kconfig
>>>>
>>>> ---
>>>>    arch/sandbox/Kconfig    | 18 +++---------------
>>>>    scripts/Kconfig.include |  4 ++++
>>>>    2 files changed, 7 insertions(+), 15 deletions(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> My only question is whether we can allow building the 32-bit version
>>> on a 64-bit machine? That would need a separate option I think, to
>>> say:
>>>
>>> I don't want you to automatically determine HOST_32/64BIT. Instead,
>>> use 32 (or 64).
>>>
>>> This is along the lines of what Heinrich is saying, except that I
>>> strongly feel that we must do the right thing by default, as your
>>> patch does.
>>
>> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
>> or 32bit on ilp32.
>>
>> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
>> that is bad.
>>
>> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
>> what we currently test with sandbox_defconfig on Gitlab CI.
>>
>> My patch is ending up in the same behavior as Michal's patch except that
>> it allows to have 64bit phys_addr_t on ilp32.
>
> It needs to automatically default to 32 or 64 bit depending on the
> host. If the user wants to fiddle with Kconfig to force it to the
> other one, that should be possible to.
>
> It looks like your patch requires manual configuration, but perhaps I
> just misunderstood it?

__LP64__ is a symbol defined by the compiler when compiling for 64bit
and not defined when compiling for 32bit systems. There is nothing
manual about it.

My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.

Michal's patch compiles a program tools/bits-per-long.c that ends up
returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
The solution is only overly complicated.

What has to be chosen manually with both patches is PHYS_64BIT e.g. by
selecting sandbox64_defconfig instead of sandbox_defconfig.

Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
is a necessary test scenario and introduced an invalid dependency.

With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.

With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
phys_addr_t.

Best regards

Heinrich

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Michal Suchánek]

On Sat, Oct 15, 2022 at 09:05:53PM +0200, Heinrich Schuchardt wrote:
> On 10/15/22 20:39, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > 
> > > On 10/15/22 19:53, Simon Glass wrote:
> > > > Hi Michal,
> > > > 
> > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > 
> > > > > Currently sandbox configuration defautls to 64bit and there is no
> > > > > automation for building 32bit sandbox on 32bit hosts.
> > > > > 
> > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
> > > > > 
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > ---
> > > > > 
> > > > > Changes in v2:
> > > > > simplify and move detection to kconfig
> > > > > 
> > > > > ---
> > > > >    arch/sandbox/Kconfig    | 18 +++---------------
> > > > >    scripts/Kconfig.include |  4 ++++
> > > > >    2 files changed, 7 insertions(+), 15 deletions(-)
> > > > 
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > 
> > > > My only question is whether we can allow building the 32-bit version
> > > > on a 64-bit machine? That would need a separate option I think, to
> > > > say:
> > > > 
> > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > > > use 32 (or 64).
> > > > 
> > > > This is along the lines of what Heinrich is saying, except that I
> > > > strongly feel that we must do the right thing by default, as your
> > > > patch does.
> > > 
> > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> > > or 32bit on ilp32.
> > > 
> > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> > > that is bad.
> > > 
> > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> > > what we currently test with sandbox_defconfig on Gitlab CI.
> > > 
> > > My patch is ending up in the same behavior as Michal's patch except that
> > > it allows to have 64bit phys_addr_t on ilp32.
> > 
> > It needs to automatically default to 32 or 64 bit depending on the
> > host. If the user wants to fiddle with Kconfig to force it to the
> > other one, that should be possible to.
> > 
> > It looks like your patch requires manual configuration, but perhaps I
> > just misunderstood it?
> 
> __LP64__ is a symbol defined by the compiler when compiling for 64bit
> and not defined when compiling for 32bit systems. There is nothing
> manual about it.
> 
> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> 
> Michal's patch compiles a program tools/bits-per-long.c that ends up
> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> The solution is only overly complicated.
> 
> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> selecting sandbox64_defconfig instead of sandbox_defconfig.
> 
> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> is a necessary test scenario and introduced an invalid dependency.

It did not introduce it, it merely did not remove it.

> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> 
> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> phys_addr_t.
> 
> Best regards
> 
> Heinrich

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi Heinrich,

On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/15/22 20:39, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/15/22 19:53, Simon Glass wrote:
> >>> Hi Michal,
> >>>
> >>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> >>>>
> >>>> Currently sandbox configuration defautls to 64bit and there is no
> >>>> automation for building 32bit sandbox on 32bit hosts.
> >>>>
> >>>> Use _LP64 macro as heuristic for detecting 64bit targets.
> >>>>
> >>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> simplify and move detection to kconfig
> >>>>
> >>>> ---
> >>>>    arch/sandbox/Kconfig    | 18 +++---------------
> >>>>    scripts/Kconfig.include |  4 ++++
> >>>>    2 files changed, 7 insertions(+), 15 deletions(-)
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>
> >>> My only question is whether we can allow building the 32-bit version
> >>> on a 64-bit machine? That would need a separate option I think, to
> >>> say:
> >>>
> >>> I don't want you to automatically determine HOST_32/64BIT. Instead,
> >>> use 32 (or 64).
> >>>
> >>> This is along the lines of what Heinrich is saying, except that I
> >>> strongly feel that we must do the right thing by default, as your
> >>> patch does.
> >>
> >> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> >> or 32bit on ilp32.
> >>
> >> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> >> that is bad.
> >>
> >> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> >> what we currently test with sandbox_defconfig on Gitlab CI.
> >>
> >> My patch is ending up in the same behavior as Michal's patch except that
> >> it allows to have 64bit phys_addr_t on ilp32.
> >
> > It needs to automatically default to 32 or 64 bit depending on the
> > host. If the user wants to fiddle with Kconfig to force it to the
> > other one, that should be possible to.
> >
> > It looks like your patch requires manual configuration, but perhaps I
> > just misunderstood it?
>
> __LP64__ is a symbol defined by the compiler when compiling for 64bit
> and not defined when compiling for 32bit systems. There is nothing
> manual about it.
>
> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
>
> Michal's patch compiles a program tools/bits-per-long.c that ends up
> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> The solution is only overly complicated.
>
> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> selecting sandbox64_defconfig instead of sandbox_defconfig.
>
> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> is a necessary test scenario and introduced an invalid dependency.
>
> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
>
> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> phys_addr_t.

That's all great, thank you, but please can you address my actual question?

Regards,
Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/15/22 20:39, Simon Glass wrote:
>> > Hi Heinrich,
>> >
>> > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> >>
>> >> On 10/15/22 19:53, Simon Glass wrote:
>> >>> Hi Michal,
>> >>>
>> >>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>> >>>>
>> >>>> Currently sandbox configuration defautls to 64bit and there is no
>> >>>> automation for building 32bit sandbox on 32bit hosts.
>> >>>>
>> >>>> Use _LP64 macro as heuristic for detecting 64bit targets.
>> >>>>
>> >>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> >>>> ---
>> >>>>
>> >>>> Changes in v2:
>> >>>> simplify and move detection to kconfig
>> >>>>
>> >>>> ---
>> >>>>    arch/sandbox/Kconfig    | 18 +++---------------
>> >>>>    scripts/Kconfig.include |  4 ++++
>> >>>>    2 files changed, 7 insertions(+), 15 deletions(-)
>> >>>
>> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> >>>
>> >>> My only question is whether we can allow building the 32-bit version
>> >>> on a 64-bit machine? That would need a separate option I think, to
>> >>> say:
>> >>>
>> >>> I don't want you to automatically determine HOST_32/64BIT. Instead,
>> >>> use 32 (or 64).
>> >>>
>> >>> This is along the lines of what Heinrich is saying, except that I
>> >>> strongly feel that we must do the right thing by default, as your
>> >>> patch does.
>> >>
>> >> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
>> >> or 32bit on ilp32.
>> >>
>> >> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
>> >> that is bad.
>> >>
>> >> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
>> >> what we currently test with sandbox_defconfig on Gitlab CI.
>> >>
>> >> My patch is ending up in the same behavior as Michal's patch except that
>> >> it allows to have 64bit phys_addr_t on ilp32.
>> >
>> > It needs to automatically default to 32 or 64 bit depending on the
>> > host. If the user wants to fiddle with Kconfig to force it to the
>> > other one, that should be possible to.
>> >
>> > It looks like your patch requires manual configuration, but perhaps I
>> > just misunderstood it?
>>
>> __LP64__ is a symbol defined by the compiler when compiling for 64bit
>> and not defined when compiling for 32bit systems. There is nothing
>> manual about it.
>>
>> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
>>
>> Michal's patch compiles a program tools/bits-per-long.c that ends up
>> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
>> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
>> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
>> The solution is only overly complicated.
>>
>> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
>> selecting sandbox64_defconfig instead of sandbox_defconfig.
>>
>> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
>> is a necessary test scenario and introduced an invalid dependency.
>>
>> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
>>
>> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
>> phys_addr_t.
>
>That's all great, thank you, but please can you address my actual question?

Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.

What other question do you have?

Best regards 

Heinrich 

>
>Regards,
>Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

Am 15. Oktober 2022 21:17:12 MESZ schrieb "Michal Suchánek" <msuchanek@suse.de>:
>On Sat, Oct 15, 2022 at 09:05:53PM +0200, Heinrich Schuchardt wrote:
>> On 10/15/22 20:39, Simon Glass wrote:
>> > Hi Heinrich,
>> > 
>> > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> > > 
>> > > On 10/15/22 19:53, Simon Glass wrote:
>> > > > Hi Michal,
>> > > > 
>> > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>> > > > > 
>> > > > > Currently sandbox configuration defautls to 64bit and there is no
>> > > > > automation for building 32bit sandbox on 32bit hosts.
>> > > > > 
>> > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
>> > > > > 
>> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> > > > > ---
>> > > > > 
>> > > > > Changes in v2:
>> > > > > simplify and move detection to kconfig
>> > > > > 
>> > > > > ---
>> > > > >    arch/sandbox/Kconfig    | 18 +++---------------
>> > > > >    scripts/Kconfig.include |  4 ++++
>> > > > >    2 files changed, 7 insertions(+), 15 deletions(-)
>> > > > 
>> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
>> > > > 
>> > > > My only question is whether we can allow building the 32-bit version
>> > > > on a 64-bit machine? That would need a separate option I think, to
>> > > > say:
>> > > > 
>> > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
>> > > > use 32 (or 64).
>> > > > 
>> > > > This is along the lines of what Heinrich is saying, except that I
>> > > > strongly feel that we must do the right thing by default, as your
>> > > > patch does.
>> > > 
>> > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
>> > > or 32bit on ilp32.
>> > > 
>> > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
>> > > that is bad.
>> > > 
>> > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
>> > > what we currently test with sandbox_defconfig on Gitlab CI.
>> > > 
>> > > My patch is ending up in the same behavior as Michal's patch except that
>> > > it allows to have 64bit phys_addr_t on ilp32.
>> > 
>> > It needs to automatically default to 32 or 64 bit depending on the
>> > host. If the user wants to fiddle with Kconfig to force it to the
>> > other one, that should be possible to.
>> > 
>> > It looks like your patch requires manual configuration, but perhaps I
>> > just misunderstood it?
>> 
>> __LP64__ is a symbol defined by the compiler when compiling for 64bit
>> and not defined when compiling for 32bit systems. There is nothing
>> manual about it.
>> 
>> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
>> 
>> Michal's patch compiles a program tools/bits-per-long.c that ends up
>> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
>> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
>> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
>> The solution is only overly complicated.
>> 
>> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
>> selecting sandbox64_defconfig instead of sandbox_defconfig.
>> 
>> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
>> is a necessary test scenario and introduced an invalid dependency.
>
>It did not introduce it, it merely did not remove it.


Without your patch I could build with PHYS_64BIT=y on ARMv7. With your patch I can not.

This obviously is a new constraint.

Best regards 

Heinrich

>
>> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
>> 
>> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
>> phys_addr_t.
>> 
>> Best regards
>> 
>> Heinrich

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi Heinrich,

On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/15/22 20:39, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >> >>
> >> >> On 10/15/22 19:53, Simon Glass wrote:
> >> >>> Hi Michal,
> >> >>>
> >> >>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> >> >>>>
> >> >>>> Currently sandbox configuration defautls to 64bit and there is no
> >> >>>> automation for building 32bit sandbox on 32bit hosts.
> >> >>>>
> >> >>>> Use _LP64 macro as heuristic for detecting 64bit targets.
> >> >>>>
> >> >>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >> >>>> ---
> >> >>>>
> >> >>>> Changes in v2:
> >> >>>> simplify and move detection to kconfig
> >> >>>>
> >> >>>> ---
> >> >>>>    arch/sandbox/Kconfig    | 18 +++---------------
> >> >>>>    scripts/Kconfig.include |  4 ++++
> >> >>>>    2 files changed, 7 insertions(+), 15 deletions(-)
> >> >>>
> >> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> >>>
> >> >>> My only question is whether we can allow building the 32-bit version
> >> >>> on a 64-bit machine? That would need a separate option I think, to
> >> >>> say:
> >> >>>
> >> >>> I don't want you to automatically determine HOST_32/64BIT. Instead,
> >> >>> use 32 (or 64).
> >> >>>
> >> >>> This is along the lines of what Heinrich is saying, except that I
> >> >>> strongly feel that we must do the right thing by default, as your
> >> >>> patch does.
> >> >>
> >> >> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> >> >> or 32bit on ilp32.
> >> >>
> >> >> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> >> >> that is bad.
> >> >>
> >> >> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> >> >> what we currently test with sandbox_defconfig on Gitlab CI.
> >> >>
> >> >> My patch is ending up in the same behavior as Michal's patch except that
> >> >> it allows to have 64bit phys_addr_t on ilp32.
> >> >
> >> > It needs to automatically default to 32 or 64 bit depending on the
> >> > host. If the user wants to fiddle with Kconfig to force it to the
> >> > other one, that should be possible to.
> >> >
> >> > It looks like your patch requires manual configuration, but perhaps I
> >> > just misunderstood it?
> >>
> >> __LP64__ is a symbol defined by the compiler when compiling for 64bit
> >> and not defined when compiling for 32bit systems. There is nothing
> >> manual about it.
> >>
> >> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> >>
> >> Michal's patch compiles a program tools/bits-per-long.c that ends up
> >> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> >> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> >> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> >> The solution is only overly complicated.
> >>
> >> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> >> selecting sandbox64_defconfig instead of sandbox_defconfig.
> >>
> >> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> >> is a necessary test scenario and introduced an invalid dependency.
> >>
> >> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> >>
> >> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> >> phys_addr_t.
> >
> >That's all great, thank you, but please can you address my actual question?
>
> Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
>
> What other question do you have?

"It needs to automatically default to 32 or 64 bit depending on the
host. If the user wants to fiddle with Kconfig to force it to the
other one, that should be possible to."

I am not talking about anyone's patch, actually, just trying to state
what I think should happen.

Regards,
Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Heinrich Schuchardt]

On 10/15/22 21:46, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 10/15/22 20:39, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 10/15/22 19:53, Simon Glass wrote:
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
>>>>>>>>
>>>>>>>> Currently sandbox configuration defautls to 64bit and there is no
>>>>>>>> automation for building 32bit sandbox on 32bit hosts.
>>>>>>>>
>>>>>>>> Use _LP64 macro as heuristic for detecting 64bit targets.
>>>>>>>>
>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> simplify and move detection to kconfig
>>>>>>>>
>>>>>>>> ---
>>>>>>>>     arch/sandbox/Kconfig    | 18 +++---------------
>>>>>>>>     scripts/Kconfig.include |  4 ++++
>>>>>>>>     2 files changed, 7 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>
>>>>>>> My only question is whether we can allow building the 32-bit version
>>>>>>> on a 64-bit machine? That would need a separate option I think, to
>>>>>>> say:
>>>>>>>
>>>>>>> I don't want you to automatically determine HOST_32/64BIT. Instead,
>>>>>>> use 32 (or 64).
>>>>>>>
>>>>>>> This is along the lines of what Heinrich is saying, except that I
>>>>>>> strongly feel that we must do the right thing by default, as your
>>>>>>> patch does.
>>>>>>
>>>>>> The whole point of phys_addr_t and phys_size_t is that it can be 64bit
>>>>>> or 32bit on ilp32.
>>>>>>
>>>>>> With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
>>>>>> that is bad.
>>>>>>
>>>>>> 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
>>>>>> what we currently test with sandbox_defconfig on Gitlab CI.
>>>>>>
>>>>>> My patch is ending up in the same behavior as Michal's patch except that
>>>>>> it allows to have 64bit phys_addr_t on ilp32.
>>>>>
>>>>> It needs to automatically default to 32 or 64 bit depending on the
>>>>> host. If the user wants to fiddle with Kconfig to force it to the
>>>>> other one, that should be possible to.
>>>>>
>>>>> It looks like your patch requires manual configuration, but perhaps I
>>>>> just misunderstood it?
>>>>
>>>> __LP64__ is a symbol defined by the compiler when compiling for 64bit
>>>> and not defined when compiling for 32bit systems. There is nothing
>>>> manual about it.
>>>>
>>>> My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
>>>>
>>>> Michal's patch compiles a program tools/bits-per-long.c that ends up
>>>> returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
>>>> bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
>>>> and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
>>>> The solution is only overly complicated.
>>>>
>>>> What has to be chosen manually with both patches is PHYS_64BIT e.g. by
>>>> selecting sandbox64_defconfig instead of sandbox_defconfig.
>>>>
>>>> Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
>>>> is a necessary test scenario and introduced an invalid dependency.
>>>>
>>>> With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
>>>>
>>>> With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
>>>> phys_addr_t.
>>>
>>> That's all great, thank you, but please can you address my actual question?
>>
>> Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
>>
>> What other question do you have?
>
> "It needs to automatically default to 32 or 64 bit depending on the
> host. If the user wants to fiddle with Kconfig to force it to the
> other one, that should be possible to."

The user forces 32bit or 64bit by selecting a 32bit or 64bit compiler
not with Kconfig. PHYS_64BIT is the only thing that needs to be selected
via Kconfig.

>
> I am not talking about anyone's patch, actually, just trying to state
> what I think should happen.

The physical systems that U-Boot has to deal with are:

* 32bit without physical address extension (PAE)
   Here phys_addr_t must be 32 bit.
* 32bit with physical address extension.
   Here phys_addr_t must be 64 bit.
* 64bit systems without PAE.
   Here phys_addr_t must be 64 bit.

We want to model these three scenarios with the sandbox.

So we have to build:

* Sandbox with PHYS_64BIT=n using a 32bit compiler.
* Sandbox with PHYS_64BIT=y using a 32bit compiler.
* Sandbox with PHYS_64BIT=y using a 64bit compiler.

Sandbox with PHYS_64BIT=n and using a 64bit compiler is irrelevant as it
does not match any physical system.

PHYS_64BIT selecting HOST_64BIT=y was always wrong.

Best regards

Heinrich

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Michal Suchánek]

On Sat, Oct 15, 2022 at 10:27:43PM +0200, Heinrich Schuchardt wrote:
> On 10/15/22 21:46, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > 
> > > 
> > > 
> > > Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
> > > > Hi Heinrich,
> > > > 
> > > > On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > 
> > > > > On 10/15/22 20:39, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > > 
> > > > > > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > 
> > > > > > > On 10/15/22 19:53, Simon Glass wrote:
> > > > > > > > Hi Michal,
> > > > > > > > 
> > > > > > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > > > > 
> > > > > > > > > Currently sandbox configuration defautls to 64bit and there is no
> > > > > > > > > automation for building 32bit sandbox on 32bit hosts.
> > > > > > > > > 
> > > > > > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Changes in v2:
> > > > > > > > > simplify and move detection to kconfig
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >     arch/sandbox/Kconfig    | 18 +++---------------
> > > > > > > > >     scripts/Kconfig.include |  4 ++++
> > > > > > > > >     2 files changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > 
> > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > > 
> > > > > > > > My only question is whether we can allow building the 32-bit version
> > > > > > > > on a 64-bit machine? That would need a separate option I think, to
> > > > > > > > say:
> > > > > > > > 
> > > > > > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > > > > > > > use 32 (or 64).
> > > > > > > > 
> > > > > > > > This is along the lines of what Heinrich is saying, except that I
> > > > > > > > strongly feel that we must do the right thing by default, as your
> > > > > > > > patch does.
> > > > > > > 
> > > > > > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> > > > > > > or 32bit on ilp32.
> > > > > > > 
> > > > > > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> > > > > > > that is bad.
> > > > > > > 
> > > > > > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> > > > > > > what we currently test with sandbox_defconfig on Gitlab CI.
> > > > > > > 
> > > > > > > My patch is ending up in the same behavior as Michal's patch except that
> > > > > > > it allows to have 64bit phys_addr_t on ilp32.
> > > > > > 
> > > > > > It needs to automatically default to 32 or 64 bit depending on the
> > > > > > host. If the user wants to fiddle with Kconfig to force it to the
> > > > > > other one, that should be possible to.
> > > > > > 
> > > > > > It looks like your patch requires manual configuration, but perhaps I
> > > > > > just misunderstood it?
> > > > > 
> > > > > __LP64__ is a symbol defined by the compiler when compiling for 64bit
> > > > > and not defined when compiling for 32bit systems. There is nothing
> > > > > manual about it.
> > > > > 
> > > > > My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> > > > > 
> > > > > Michal's patch compiles a program tools/bits-per-long.c that ends up
> > > > > returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> > > > > bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> > > > > and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> > > > > The solution is only overly complicated.
> > > > > 
> > > > > What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> > > > > selecting sandbox64_defconfig instead of sandbox_defconfig.
> > > > > 
> > > > > Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> > > > > is a necessary test scenario and introduced an invalid dependency.
> > > > > 
> > > > > With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> > > > > 
> > > > > With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> > > > > phys_addr_t.
> > > > 
> > > > That's all great, thank you, but please can you address my actual question?
> > > 
> > > Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
> > > 
> > > What other question do you have?
> > 
> > "It needs to automatically default to 32 or 64 bit depending on the
> > host. If the user wants to fiddle with Kconfig to force it to the
> > other one, that should be possible to."
> 
> The user forces 32bit or 64bit by selecting a 32bit or 64bit compiler
> not with Kconfig. PHYS_64BIT is the only thing that needs to be selected
> via Kconfig.
> 
> > 
> > I am not talking about anyone's patch, actually, just trying to state
> > what I think should happen.
> 
> The physical systems that U-Boot has to deal with are:
> 
> * 32bit without physical address extension (PAE)
>   Here phys_addr_t must be 32 bit.
> * 32bit with physical address extension.
>   Here phys_addr_t must be 64 bit.
> * 64bit systems without PAE.
>   Here phys_addr_t must be 64 bit.
> 
> We want to model these three scenarios with the sandbox.
> 
> So we have to build:
> 
> * Sandbox with PHYS_64BIT=n using a 32bit compiler.
> * Sandbox with PHYS_64BIT=y using a 32bit compiler.
> * Sandbox with PHYS_64BIT=y using a 64bit compiler.

To get these three and not the fourth a kconfig option would still be
needed, right?

Thanks

Michal

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi,

On Mon, 17 Oct 2022 at 01:28, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Sat, Oct 15, 2022 at 10:27:43PM +0200, Heinrich Schuchardt wrote:
> > On 10/15/22 21:46, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 10/15/22 20:39, Simon Glass wrote:
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 10/15/22 19:53, Simon Glass wrote:
> > > > > > > > > Hi Michal,
> > > > > > > > >
> > > > > > > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > > > > >
> > > > > > > > > > Currently sandbox configuration defautls to 64bit and there is no
> > > > > > > > > > automation for building 32bit sandbox on 32bit hosts.
> > > > > > > > > >
> > > > > > > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > simplify and move detection to kconfig
> > > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >     arch/sandbox/Kconfig    | 18 +++---------------
> > > > > > > > > >     scripts/Kconfig.include |  4 ++++
> > > > > > > > > >     2 files changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > >
> > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >
> > > > > > > > > My only question is whether we can allow building the 32-bit version
> > > > > > > > > on a 64-bit machine? That would need a separate option I think, to
> > > > > > > > > say:
> > > > > > > > >
> > > > > > > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > > > > > > > > use 32 (or 64).
> > > > > > > > >
> > > > > > > > > This is along the lines of what Heinrich is saying, except that I
> > > > > > > > > strongly feel that we must do the right thing by default, as your
> > > > > > > > > patch does.
> > > > > > > >
> > > > > > > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> > > > > > > > or 32bit on ilp32.
> > > > > > > >
> > > > > > > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> > > > > > > > that is bad.
> > > > > > > >
> > > > > > > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> > > > > > > > what we currently test with sandbox_defconfig on Gitlab CI.
> > > > > > > >
> > > > > > > > My patch is ending up in the same behavior as Michal's patch except that
> > > > > > > > it allows to have 64bit phys_addr_t on ilp32.
> > > > > > >
> > > > > > > It needs to automatically default to 32 or 64 bit depending on the
> > > > > > > host. If the user wants to fiddle with Kconfig to force it to the
> > > > > > > other one, that should be possible to.
> > > > > > >
> > > > > > > It looks like your patch requires manual configuration, but perhaps I
> > > > > > > just misunderstood it?
> > > > > >
> > > > > > __LP64__ is a symbol defined by the compiler when compiling for 64bit
> > > > > > and not defined when compiling for 32bit systems. There is nothing
> > > > > > manual about it.
> > > > > >
> > > > > > My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> > > > > >
> > > > > > Michal's patch compiles a program tools/bits-per-long.c that ends up
> > > > > > returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> > > > > > bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> > > > > > and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> > > > > > The solution is only overly complicated.
> > > > > >
> > > > > > What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> > > > > > selecting sandbox64_defconfig instead of sandbox_defconfig.
> > > > > >
> > > > > > Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> > > > > > is a necessary test scenario and introduced an invalid dependency.
> > > > > >
> > > > > > With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> > > > > >
> > > > > > With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> > > > > > phys_addr_t.
> > > > >
> > > > > That's all great, thank you, but please can you address my actual question?
> > > >
> > > > Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
> > > >
> > > > What other question do you have?
> > >
> > > "It needs to automatically default to 32 or 64 bit depending on the
> > > host. If the user wants to fiddle with Kconfig to force it to the
> > > other one, that should be possible to."
> >
> > The user forces 32bit or 64bit by selecting a 32bit or 64bit compiler
> > not with Kconfig. PHYS_64BIT is the only thing that needs to be selected
> > via Kconfig.
> >
> > >
> > > I am not talking about anyone's patch, actually, just trying to state
> > > what I think should happen.
> >
> > The physical systems that U-Boot has to deal with are:
> >
> > * 32bit without physical address extension (PAE)
> >   Here phys_addr_t must be 32 bit.
> > * 32bit with physical address extension.
> >   Here phys_addr_t must be 64 bit.
> > * 64bit systems without PAE.
> >   Here phys_addr_t must be 64 bit.
> >
> > We want to model these three scenarios with the sandbox.
> >
> > So we have to build:
> >
> > * Sandbox with PHYS_64BIT=n using a 32bit compiler.
> > * Sandbox with PHYS_64BIT=y using a 32bit compiler.
> > * Sandbox with PHYS_64BIT=y using a 64bit compiler.
>
> To get these three and not the fourth a kconfig option would still be
> needed, right?

My concern is that 1 and 3 are built automatically by default,
depending on what bitness you are using (or compiler, that's fine too
as it might be equivalent).

So NO Kconfig change to get that behaviour. My request for a Kconfig
to *manually* select which to use is not as important as the above, so
let's ignore it.

For #2 that would need to do a config change as it isn't worth
creating a new sandbox build just for that. We could do it in CI with
'buildman -a PHYS_64BIT=y'

Regards,
Simon

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Simon Glass]

Hi,

On Mon, 17 Oct 2022 at 01:28, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Sat, Oct 15, 2022 at 10:27:43PM +0200, Heinrich Schuchardt wrote:
> > On 10/15/22 21:46, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 10/15/22 20:39, Simon Glass wrote:
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 10/15/22 19:53, Simon Glass wrote:
> > > > > > > > > Hi Michal,
> > > > > > > > >
> > > > > > > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > > > > >
> > > > > > > > > > Currently sandbox configuration defautls to 64bit and there is no
> > > > > > > > > > automation for building 32bit sandbox on 32bit hosts.
> > > > > > > > > >
> > > > > > > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > simplify and move detection to kconfig
> > > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >     arch/sandbox/Kconfig    | 18 +++---------------
> > > > > > > > > >     scripts/Kconfig.include |  4 ++++
> > > > > > > > > >     2 files changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > >
> > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >
> > > > > > > > > My only question is whether we can allow building the 32-bit version
> > > > > > > > > on a 64-bit machine? That would need a separate option I think, to
> > > > > > > > > say:
> > > > > > > > >
> > > > > > > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > > > > > > > > use 32 (or 64).
> > > > > > > > >
> > > > > > > > > This is along the lines of what Heinrich is saying, except that I
> > > > > > > > > strongly feel that we must do the right thing by default, as your
> > > > > > > > > patch does.
> > > > > > > >
> > > > > > > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> > > > > > > > or 32bit on ilp32.
> > > > > > > >
> > > > > > > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> > > > > > > > that is bad.
> > > > > > > >
> > > > > > > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> > > > > > > > what we currently test with sandbox_defconfig on Gitlab CI.
> > > > > > > >
> > > > > > > > My patch is ending up in the same behavior as Michal's patch except that
> > > > > > > > it allows to have 64bit phys_addr_t on ilp32.
> > > > > > >
> > > > > > > It needs to automatically default to 32 or 64 bit depending on the
> > > > > > > host. If the user wants to fiddle with Kconfig to force it to the
> > > > > > > other one, that should be possible to.
> > > > > > >
> > > > > > > It looks like your patch requires manual configuration, but perhaps I
> > > > > > > just misunderstood it?
> > > > > >
> > > > > > __LP64__ is a symbol defined by the compiler when compiling for 64bit
> > > > > > and not defined when compiling for 32bit systems. There is nothing
> > > > > > manual about it.
> > > > > >
> > > > > > My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> > > > > >
> > > > > > Michal's patch compiles a program tools/bits-per-long.c that ends up
> > > > > > returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> > > > > > bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> > > > > > and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> > > > > > The solution is only overly complicated.
> > > > > >
> > > > > > What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> > > > > > selecting sandbox64_defconfig instead of sandbox_defconfig.
> > > > > >
> > > > > > Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> > > > > > is a necessary test scenario and introduced an invalid dependency.
> > > > > >
> > > > > > With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> > > > > >
> > > > > > With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> > > > > > phys_addr_t.
> > > > >
> > > > > That's all great, thank you, but please can you address my actual question?
> > > >
> > > > Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
> > > >
> > > > What other question do you have?
> > >
> > > "It needs to automatically default to 32 or 64 bit depending on the
> > > host. If the user wants to fiddle with Kconfig to force it to the
> > > other one, that should be possible to."
> >
> > The user forces 32bit or 64bit by selecting a 32bit or 64bit compiler
> > not with Kconfig. PHYS_64BIT is the only thing that needs to be selected
> > via Kconfig.
> >
> > >
> > > I am not talking about anyone's patch, actually, just trying to state
> > > what I think should happen.
> >
> > The physical systems that U-Boot has to deal with are:
> >
> > * 32bit without physical address extension (PAE)
> >   Here phys_addr_t must be 32 bit.
> > * 32bit with physical address extension.
> >   Here phys_addr_t must be 64 bit.
> > * 64bit systems without PAE.
> >   Here phys_addr_t must be 64 bit.
> >
> > We want to model these three scenarios with the sandbox.
> >
> > So we have to build:
> >
> > * Sandbox with PHYS_64BIT=n using a 32bit compiler.
> > * Sandbox with PHYS_64BIT=y using a 32bit compiler.
> > * Sandbox with PHYS_64BIT=y using a 64bit compiler.
>
> To get these three and not the fourth a kconfig option would still be
> needed, right?

My concern is that 1 and 3 are built automatically by default,
depending on what bitness you are using (or compiler, that's fine too
as it might be equivalent).

So NO Kconfig change to get that behaviour. My request for a Kconfig
to *manually* select which to use is not as important as the above, so
let's ignore it.

For #2 that would need to do a config change as it isn't worth
creating a new sandbox build just for that. We could do it in CI with
'buildman -a PHYS_64BIT=y'

Regards,
Simon

Applied to u-boot-dm, thanks!

Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit

[Michal Suchánek]

On Fri, Oct 21, 2022 at 06:05:51PM -0700, Simon Glass wrote:
> Hi,
> 
> On Mon, 17 Oct 2022 at 01:28, Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Sat, Oct 15, 2022 at 10:27:43PM +0200, Heinrich Schuchardt wrote:
> > > On 10/15/22 21:46, Simon Glass wrote:
> > > > Hi Heinrich,
> > > >
> > > > On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg@chromium.org>:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 10/15/22 20:39, Simon Glass wrote:
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Sat, 15 Oct 2022 at 12:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 10/15/22 19:53, Simon Glass wrote:
> > > > > > > > > > Hi Michal,
> > > > > > > > > >
> > > > > > > > > > On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Currently sandbox configuration defautls to 64bit and there is no
> > > > > > > > > > > automation for building 32bit sandbox on 32bit hosts.
> > > > > > > > > > >
> > > > > > > > > > > Use _LP64 macro as heuristic for detecting 64bit targets.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > Changes in v2:
> > > > > > > > > > > simplify and move detection to kconfig
> > > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > >     arch/sandbox/Kconfig    | 18 +++---------------
> > > > > > > > > > >     scripts/Kconfig.include |  4 ++++
> > > > > > > > > > >     2 files changed, 7 insertions(+), 15 deletions(-)
> > > > > > > > > >
> > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > >
> > > > > > > > > > My only question is whether we can allow building the 32-bit version
> > > > > > > > > > on a 64-bit machine? That would need a separate option I think, to
> > > > > > > > > > say:
> > > > > > > > > >
> > > > > > > > > > I don't want you to automatically determine HOST_32/64BIT. Instead,
> > > > > > > > > > use 32 (or 64).
> > > > > > > > > >
> > > > > > > > > > This is along the lines of what Heinrich is saying, except that I
> > > > > > > > > > strongly feel that we must do the right thing by default, as your
> > > > > > > > > > patch does.
> > > > > > > > >
> > > > > > > > > The whole point of phys_addr_t and phys_size_t is that it can be 64bit
> > > > > > > > > or 32bit on ilp32.
> > > > > > > > >
> > > > > > > > > With this patch we cannot build with CONFIG_PHYS_64BIT=y on ilp32 and
> > > > > > > > > that is bad.
> > > > > > > > >
> > > > > > > > > 32 bit phys_addr_t on lp64 is irrelevant for actual hardware but this is
> > > > > > > > > what we currently test with sandbox_defconfig on Gitlab CI.
> > > > > > > > >
> > > > > > > > > My patch is ending up in the same behavior as Michal's patch except that
> > > > > > > > > it allows to have 64bit phys_addr_t on ilp32.
> > > > > > > >
> > > > > > > > It needs to automatically default to 32 or 64 bit depending on the
> > > > > > > > host. If the user wants to fiddle with Kconfig to force it to the
> > > > > > > > other one, that should be possible to.
> > > > > > > >
> > > > > > > > It looks like your patch requires manual configuration, but perhaps I
> > > > > > > > just misunderstood it?
> > > > > > >
> > > > > > > __LP64__ is a symbol defined by the compiler when compiling for 64bit
> > > > > > > and not defined when compiling for 32bit systems. There is nothing
> > > > > > > manual about it.
> > > > > > >
> > > > > > > My patch uses this symbol to replace HOST_32BIT and HOST_64BIT.
> > > > > > >
> > > > > > > Michal's patch compiles a program tools/bits-per-long.c that ends up
> > > > > > > returning 64 on 64 bit systems (where __LP64__ is defined) and 32 on 32
> > > > > > > bit systems (where __LP64__ is not defined) and then chooses HOST_32BIT
> > > > > > > and HOST_64BIT accordingly. This part of Michal's patch is not wrong.
> > > > > > > The solution is only overly complicated.
> > > > > > >
> > > > > > > What has to be chosen manually with both patches is PHYS_64BIT e.g. by
> > > > > > > selecting sandbox64_defconfig instead of sandbox_defconfig.
> > > > > > >
> > > > > > > Unfortunately Michal did not understand that PHYS_64BIT=y, HOST_32BIT=y
> > > > > > > is a necessary test scenario and introduced an invalid dependency.
> > > > > > >
> > > > > > > With my patch sandbox64_defconfig on a 32bit system uses 64bit phys_addr_t.
> > > > > > >
> > > > > > > With Michal's patch sandbox64_defconfig on a 32bit system uses 32bit
> > > > > > > phys_addr_t.
> > > > > >
> > > > > > That's all great, thank you, but please can you address my actual question?
> > > > >
> > > > > Your question in this thread was if my patch requires extra manual configuration compared to Michal's patch and the answer was no.
> > > > >
> > > > > What other question do you have?
> > > >
> > > > "It needs to automatically default to 32 or 64 bit depending on the
> > > > host. If the user wants to fiddle with Kconfig to force it to the
> > > > other one, that should be possible to."
> > >
> > > The user forces 32bit or 64bit by selecting a 32bit or 64bit compiler
> > > not with Kconfig. PHYS_64BIT is the only thing that needs to be selected
> > > via Kconfig.
> > >
> > > >
> > > > I am not talking about anyone's patch, actually, just trying to state
> > > > what I think should happen.
> > >
> > > The physical systems that U-Boot has to deal with are:
> > >
> > > * 32bit without physical address extension (PAE)
> > >   Here phys_addr_t must be 32 bit.
> > > * 32bit with physical address extension.
> > >   Here phys_addr_t must be 64 bit.
> > > * 64bit systems without PAE.
> > >   Here phys_addr_t must be 64 bit.
> > >
> > > We want to model these three scenarios with the sandbox.
> > >
> > > So we have to build:
> > >
> > > * Sandbox with PHYS_64BIT=n using a 32bit compiler.
> > > * Sandbox with PHYS_64BIT=y using a 32bit compiler.
> > > * Sandbox with PHYS_64BIT=y using a 64bit compiler.
> >
> > To get these three and not the fourth a kconfig option would still be
> > needed, right?
> 
> My concern is that 1 and 3 are built automatically by default,
> depending on what bitness you are using (or compiler, that's fine too
> as it might be equivalent).
> 
> So NO Kconfig change to get that behaviour. My request for a Kconfig
> to *manually* select which to use is not as important as the above, so
> let's ignore it.
> 
> For #2 that would need to do a config change as it isn't worth
> creating a new sandbox build just for that. We could do it in CI with
> 'buildman -a PHYS_64BIT=y'

Then you may want the alternative patch instead:

[PATCH] sandbox: Eliminate CONFIG_HOST_32/64BIT
https://lists.denx.de/pipermail/u-boot/2022-October/497236.html

hm, on a second thought you probably don't unless it's cleaned up to use
__LP64__ only once to define the sandbox bits.

Thanks

Michal

> 
> Regards,
> Simon
> 
> Applied to u-boot-dm, thanks!

[PATCH] sandbox: Correctly define BITS_PER_LONG

[Michal Suchanek]

SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
platform.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---

 arch/sandbox/include/asm/types.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
index c1a5d2af82..5f4b649ee3 100644
--- a/arch/sandbox/include/asm/types.h
+++ b/arch/sandbox/include/asm/types.h
@@ -18,11 +18,7 @@ typedef unsigned short umode_t;
 /*
  * Number of bits in a C 'long' on this architecture.
  */
-#ifdef	CONFIG_PHYS_64BIT
-#define BITS_PER_LONG 64
-#else	/* CONFIG_PHYS_64BIT */
-#define BITS_PER_LONG 32
-#endif	/* CONFIG_PHYS_64BIT */
+#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
 
 #ifdef	CONFIG_PHYS_64BIT
 typedef unsigned long long dma_addr_t;
-- 
2.38.0

Re: [PATCH] sandbox: Correctly define BITS_PER_LONG

[Heinrich Schuchardt]

Am 22. Oktober 2022 23:22:01 MESZ schrieb Michal Suchanek <msuchanek@suse.de>:
>SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
>platform.

Please, explain in the commit message what this patch is good for.

Aren't further patches needed to make use of it?

Best regards

Heinrich 

>
>Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>---
>
> arch/sandbox/include/asm/types.h | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
>diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
>index c1a5d2af82..5f4b649ee3 100644
>--- a/arch/sandbox/include/asm/types.h
>+++ b/arch/sandbox/include/asm/types.h
>@@ -18,11 +18,7 @@ typedef unsigned short umode_t;
> /*
>  * Number of bits in a C 'long' on this architecture.
>  */
>-#ifdef	CONFIG_PHYS_64BIT
>-#define BITS_PER_LONG 64
>-#else	/* CONFIG_PHYS_64BIT */
>-#define BITS_PER_LONG 32
>-#endif	/* CONFIG_PHYS_64BIT */
>+#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
> 
> #ifdef	CONFIG_PHYS_64BIT
> typedef unsigned long long dma_addr_t;

Re: [PATCH] sandbox: Correctly define BITS_PER_LONG

[Michal Suchánek]

On Sat, Oct 22, 2022 at 11:52:29PM +0200, Heinrich Schuchardt wrote:
> 
> 
> Am 22. Oktober 2022 23:22:01 MESZ schrieb Michal Suchanek <msuchanek@suse.de>:
> >SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
> >platform.
> 
> Please, explain in the commit message what this patch is good for.

For setting BITS_PER_LONG correctly.

> Aren't further patches needed to make use of it?

'make ue of it' would likely by running 32bit sandbox with 64bit
phys_addr_t, and that indeed won't be fixed by this patch alone.

Nonetheless, since nobody noticed that this is broken so far I figured I
will send the patch anyway.

Thanks

Michal

> Best regards
> 
> Heinrich 
> 
> >
> >Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >---
> >
> > arch/sandbox/include/asm/types.h | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> >diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
> >index c1a5d2af82..5f4b649ee3 100644
> >--- a/arch/sandbox/include/asm/types.h
> >+++ b/arch/sandbox/include/asm/types.h
> >@@ -18,11 +18,7 @@ typedef unsigned short umode_t;
> > /*
> >  * Number of bits in a C 'long' on this architecture.
> >  */
> >-#ifdef	CONFIG_PHYS_64BIT
> >-#define BITS_PER_LONG 64
> >-#else	/* CONFIG_PHYS_64BIT */
> >-#define BITS_PER_LONG 32
> >-#endif	/* CONFIG_PHYS_64BIT */
> >+#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
> > 
> > #ifdef	CONFIG_PHYS_64BIT
> > typedef unsigned long long dma_addr_t;

Re: [PATCH] sandbox: Correctly define BITS_PER_LONG

[Heinrich Schuchardt]

On 10/23/22 09:50, Michal Suchánek wrote:
> On Sat, Oct 22, 2022 at 11:52:29PM +0200, Heinrich Schuchardt wrote:
>>
>>
>> Am 22. Oktober 2022 23:22:01 MESZ schrieb Michal Suchanek <msuchanek@suse.de>:
>>> SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
>>> platform.
>>
>> Please, explain in the commit message what this patch is good for.
>
> For setting BITS_PER_LONG correctly.
>
>> Aren't further patches needed to make use of it?
>
> 'make ue of it' would likely by running 32bit sandbox with 64bit
> phys_addr_t, and that indeed won't be fixed by this patch alone.
>
> Nonetheless, since nobody noticed that this is broken so far I figured I
> will send the patch anyway.
>
> Thanks
>
> Michal
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>>
>>> arch/sandbox/include/asm/types.h | 6 +-----
>>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
>>> index c1a5d2af82..5f4b649ee3 100644
>>> --- a/arch/sandbox/include/asm/types.h
>>> +++ b/arch/sandbox/include/asm/types.h
>>> @@ -18,11 +18,7 @@ typedef unsigned short umode_t;
>>> /*
>>>   * Number of bits in a C 'long' on this architecture.
>>>   */
>>> -#ifdef	CONFIG_PHYS_64BIT
>>> -#define BITS_PER_LONG 64
CONFIG_PHYS_64BIT defines the length of phys_addr_t.

BITS_PER_LONG is about the length of long.

The length of long is defined by the compiler. phys_addr_t exists for
having a type that is independent of the length of long.

This patch is obviously wrong.

Best regards

Heinrich

>>> -#else	/* CONFIG_PHYS_64BIT */
>>> -#define BITS_PER_LONG 32
>>> -#endif	/* CONFIG_PHYS_64BIT */
>>> +#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
>>>
>>> #ifdef	CONFIG_PHYS_64BIT
>>> typedef unsigned long long dma_addr_t;

Re: [PATCH] sandbox: Correctly define BITS_PER_LONG

[Michal Suchánek]

On Sun, Oct 23, 2022 at 09:56:29AM +0200, Heinrich Schuchardt wrote:
> On 10/23/22 09:50, Michal Suchánek wrote:
> > On Sat, Oct 22, 2022 at 11:52:29PM +0200, Heinrich Schuchardt wrote:
> > > 
> > > 
> > > Am 22. Oktober 2022 23:22:01 MESZ schrieb Michal Suchanek <msuchanek@suse.de>:
> > > > SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
> > > > platform.
> > > 
> > > Please, explain in the commit message what this patch is good for.
> > 
> > For setting BITS_PER_LONG correctly.
> > 
> > > Aren't further patches needed to make use of it?
> > 
> > 'make ue of it' would likely by running 32bit sandbox with 64bit
> > phys_addr_t, and that indeed won't be fixed by this patch alone.
> > 
> > Nonetheless, since nobody noticed that this is broken so far I figured I
> > will send the patch anyway.
> > 
> > Thanks
> > 
> > Michal
> > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > 
> > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > ---
> > > > 
> > > > arch/sandbox/include/asm/types.h | 6 +-----
> > > > 1 file changed, 1 insertion(+), 5 deletions(-)
> > > > 
> > > > diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
> > > > index c1a5d2af82..5f4b649ee3 100644
> > > > --- a/arch/sandbox/include/asm/types.h
> > > > +++ b/arch/sandbox/include/asm/types.h
> > > > @@ -18,11 +18,7 @@ typedef unsigned short umode_t;
> > > > /*
> > > >   * Number of bits in a C 'long' on this architecture.
> > > >   */
> > > > -#ifdef	CONFIG_PHYS_64BIT
> > > > -#define BITS_PER_LONG 64
> CONFIG_PHYS_64BIT defines the length of phys_addr_t.
> 
> BITS_PER_LONG is about the length of long.
Sure
> 
> The length of long is defined by the compiler.
Sure
> phys_addr_t exists for
> having a type that is independent of the length of long.
sure
> 
> This patch is obviously wrong.

That's completely contradictory to what you say above.

According to that the patch is obviously right because it changes the
definition of BITS_PER_LONG from depending on phys_addr_t to depending
on the  length of long is defined by the compiler.

Thanks

Michal

> 
> Best regards
> 
> Heinrich
> 
> > > > -#else	/* CONFIG_PHYS_64BIT */
> > > > -#define BITS_PER_LONG 32
> > > > -#endif	/* CONFIG_PHYS_64BIT */
> > > > +#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
> > > > 
> > > > #ifdef	CONFIG_PHYS_64BIT
> > > > typedef unsigned long long dma_addr_t;
> 

Re: [PATCH] sandbox: Correctly define BITS_PER_LONG

[Simon Glass]

On Sun, Oct 23, 2022 at 09:56:29AM +0200, Heinrich Schuchardt wrote:
> On 10/23/22 09:50, Michal Suchánek wrote:
> > On Sat, Oct 22, 2022 at 11:52:29PM +0200, Heinrich Schuchardt wrote:
> > >
> > >
> > > Am 22. Oktober 2022 23:22:01 MESZ schrieb Michal Suchanek <msuchanek@suse.de>:
> > > > SANDBOX_BITS_PER_LONG is the number of bits in long on the sandbox
> > > > platform.
> > >
> > > Please, explain in the commit message what this patch is good for.
> >
> > For setting BITS_PER_LONG correctly.
> >
> > > Aren't further patches needed to make use of it?
> >
> > 'make ue of it' would likely by running 32bit sandbox with 64bit
> > phys_addr_t, and that indeed won't be fixed by this patch alone.
> >
> > Nonetheless, since nobody noticed that this is broken so far I figured I
> > will send the patch anyway.
> >
> > Thanks
> >
> > Michal
> >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >
> > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > ---
> > > >
> > > > arch/sandbox/include/asm/types.h | 6 +-----
> > > > 1 file changed, 1 insertion(+), 5 deletions(-)
> > > >
Applied to u-boot-dm, thanks!