[PATCH] tests: Build correct sandbox configuration on 32bit
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); +} +
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-bootNote: 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
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
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
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-sj...
Basically we should be able to build sandbox on any platform and it should just work, without manual configuration.
Regards, Simon
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))
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 runon a 64-bit host. If you want to run sandbox ona 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))
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 runon a 64-bit host. If you want to run sandbox ona 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))
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_64BITconfig 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_64BIThelpSandbox can be built on 32-bit and 64-bit hosts.The default is to build on a 64-bit host and runon a 64-bit host. If you want to run sandbox ona 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
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_64BIThelpSandbox can be built on 32-bit and 64-bit hosts.The default is to build on a 64-bit host and runon a 64-bit host. If you want to run sandbox ona 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
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
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
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
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
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
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
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
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
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
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
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!
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!
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;
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;
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;
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;
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;
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!
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-sj...
Basically we should be able to build sandbox on any platform and it should just work, without manual configuration.
Regards, Simon
participants (4)
-
Heinrich Schuchardt -
Michal Suchanek -
Michal Suchánek
-
Simon Glass