* [PATCH] tests: Build correct sandbox configuration on 32bit
@ 2022-10-13 20:28 Michal Suchanek
2022-10-14 3:05 ` Heinrich Schuchardt
2022-10-14 15:56 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Michal Suchanek @ 2022-10-13 20:28 UTC (permalink / raw)
To: u-boot
Cc: Michal Suchanek, AKASHI Takahiro, Heiko Thiery,
Heinrich Schuchardt, Marek Behún, Pali Rohár,
Quentin Schulz, Samuel Holland, Simon Glass, Stefan Roese,
Weijie Gao
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] tests: Build correct sandbox configuration on 32bit
2022-10-13 20:28 [PATCH] tests: Build correct sandbox configuration on 32bit Michal Suchanek
@ 2022-10-14 3:05 ` Heinrich Schuchardt
2022-10-14 8:43 ` Michal Suchánek
2022-10-14 15:56 ` Simon Glass
1 sibling, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-14 3:05 UTC (permalink / raw)
To: Michal Suchanek
Cc: AKASHI Takahiro, Heiko Thiery, Marek Behún, Pali Rohár,
Quentin Schulz, Samuel Holland, Simon Glass, Stefan Roese,
Weijie Gao, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] tests: Build correct sandbox configuration on 32bit
2022-10-14 3:05 ` Heinrich Schuchardt
@ 2022-10-14 8:43 ` Michal Suchánek
2022-10-15 5:00 ` Heinrich Schuchardt
0 siblings, 1 reply; 29+ messages in thread
From: Michal Suchánek @ 2022-10-14 8:43 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: AKASHI Takahiro, Heiko Thiery, Marek Behún, Pali Rohár,
Quentin Schulz, Samuel Holland, Simon Glass, Stefan Roese,
Weijie Gao, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] tests: Build correct sandbox configuration on 32bit
2022-10-13 20:28 [PATCH] tests: Build correct sandbox configuration on 32bit Michal Suchanek
2022-10-14 3:05 ` Heinrich Schuchardt
@ 2022-10-14 15:56 ` Simon Glass
2022-10-14 20:52 ` [PATCH v2] " Michal Suchanek
2022-10-15 5:05 ` [PATCH] tests: Build correct sandbox configuration on 32bit Heinrich Schuchardt
1 sibling, 2 replies; 29+ messages in thread
From: Simon Glass @ 2022-10-14 15:56 UTC (permalink / raw)
To: Michal Suchanek
Cc: u-boot, AKASHI Takahiro, Heiko Thiery, Heinrich Schuchardt,
Marek Behún, Pali Rohár, Quentin Schulz,
Samuel Holland, Stefan Roese, Weijie Gao
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-14 15:56 ` Simon Glass
@ 2022-10-14 20:52 ` Michal Suchanek
2022-10-15 4:54 ` Heinrich Schuchardt
2022-10-15 17:53 ` Simon Glass
2022-10-15 5:05 ` [PATCH] tests: Build correct sandbox configuration on 32bit Heinrich Schuchardt
1 sibling, 2 replies; 29+ messages in thread
From: Michal Suchanek @ 2022-10-14 20:52 UTC (permalink / raw)
To: u-boot; +Cc: Michal Suchanek, Simon Glass, Heinrich Schuchardt
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-14 20:52 ` [PATCH v2] " Michal Suchanek
@ 2022-10-15 4:54 ` Heinrich Schuchardt
2022-10-15 7:19 ` Michal Suchánek
2022-10-15 17:53 ` Simon Glass
1 sibling, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 4:54 UTC (permalink / raw)
To: Michal Suchanek; +Cc: Simon Glass, u-boot
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))
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] tests: Build correct sandbox configuration on 32bit
2022-10-14 8:43 ` Michal Suchánek
@ 2022-10-15 5:00 ` Heinrich Schuchardt
0 siblings, 0 replies; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 5:00 UTC (permalink / raw)
To: Michal Suchánek
Cc: AKASHI Takahiro, Heiko Thiery, Marek Behún, Pali Rohár,
Quentin Schulz, Samuel Holland, Simon Glass, Stefan Roese,
Weijie Gao, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] tests: Build correct sandbox configuration on 32bit
2022-10-14 15:56 ` Simon Glass
2022-10-14 20:52 ` [PATCH v2] " Michal Suchanek
@ 2022-10-15 5:05 ` Heinrich Schuchardt
1 sibling, 0 replies; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 5:05 UTC (permalink / raw)
To: Simon Glass
Cc: u-boot, AKASHI Takahiro, Heiko Thiery, Marek Behún,
Pali Rohár, Quentin Schulz, Samuel Holland, Stefan Roese,
Weijie Gao, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 4:54 ` Heinrich Schuchardt
@ 2022-10-15 7:19 ` Michal Suchánek
0 siblings, 0 replies; 29+ messages in thread
From: Michal Suchánek @ 2022-10-15 7:19 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Simon Glass, u-boot
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))
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-14 20:52 ` [PATCH v2] " Michal Suchanek
2022-10-15 4:54 ` Heinrich Schuchardt
@ 2022-10-15 17:53 ` Simon Glass
2022-10-15 18:31 ` Heinrich Schuchardt
1 sibling, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-10-15 17:53 UTC (permalink / raw)
To: Michal Suchanek; +Cc: u-boot, Heinrich Schuchardt
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 17:53 ` Simon Glass
@ 2022-10-15 18:31 ` Heinrich Schuchardt
2022-10-15 18:39 ` Simon Glass
0 siblings, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 18:31 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 18:31 ` Heinrich Schuchardt
@ 2022-10-15 18:39 ` Simon Glass
2022-10-15 19:05 ` Heinrich Schuchardt
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-10-15 18:39 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 18:39 ` Simon Glass
@ 2022-10-15 19:05 ` Heinrich Schuchardt
2022-10-15 19:17 ` Michal Suchánek
2022-10-15 19:24 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 19:05 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:05 ` Heinrich Schuchardt
@ 2022-10-15 19:17 ` Michal Suchánek
2022-10-15 19:35 ` Heinrich Schuchardt
2022-10-15 19:24 ` Simon Glass
1 sibling, 1 reply; 29+ messages in thread
From: Michal Suchánek @ 2022-10-15 19:17 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Simon Glass, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:05 ` Heinrich Schuchardt
2022-10-15 19:17 ` Michal Suchánek
@ 2022-10-15 19:24 ` Simon Glass
2022-10-15 19:29 ` Heinrich Schuchardt
1 sibling, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-10-15 19:24 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:24 ` Simon Glass
@ 2022-10-15 19:29 ` Heinrich Schuchardt
2022-10-15 19:46 ` Simon Glass
0 siblings, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 19:29 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:17 ` Michal Suchánek
@ 2022-10-15 19:35 ` Heinrich Schuchardt
0 siblings, 0 replies; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 19:35 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Simon Glass, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:29 ` Heinrich Schuchardt
@ 2022-10-15 19:46 ` Simon Glass
2022-10-15 20:27 ` Heinrich Schuchardt
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-10-15 19:46 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 19:46 ` Simon Glass
@ 2022-10-15 20:27 ` Heinrich Schuchardt
2022-10-17 7:28 ` Michal Suchánek
0 siblings, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-15 20:27 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Michal Suchanek
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-15 20:27 ` Heinrich Schuchardt
@ 2022-10-17 7:28 ` Michal Suchánek
2022-10-19 13:18 ` Simon Glass
2022-10-22 1:05 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Michal Suchánek @ 2022-10-17 7:28 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Simon Glass, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-17 7:28 ` Michal Suchánek
@ 2022-10-19 13:18 ` Simon Glass
2022-10-22 1:05 ` Simon Glass
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-10-19 13:18 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Heinrich Schuchardt, u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-17 7:28 ` Michal Suchánek
2022-10-19 13:18 ` Simon Glass
@ 2022-10-22 1:05 ` Simon Glass
2022-10-22 19:38 ` Michal Suchánek
1 sibling, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-10-22 1:05 UTC (permalink / raw)
To: Simon Glass; +Cc: Heinrich Schuchardt, u-boot, Michal Suchánek
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!
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] tests: Build correct sandbox configuration on 32bit
2022-10-22 1:05 ` Simon Glass
@ 2022-10-22 19:38 ` Michal Suchánek
2022-10-22 21:22 ` [PATCH] sandbox: Correctly define BITS_PER_LONG Michal Suchanek
0 siblings, 1 reply; 29+ messages in thread
From: Michal Suchánek @ 2022-10-22 19:38 UTC (permalink / raw)
To: Simon Glass; +Cc: Heinrich Schuchardt, u-boot
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!
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-22 19:38 ` Michal Suchánek
@ 2022-10-22 21:22 ` Michal Suchanek
2022-10-22 21:52 ` Heinrich Schuchardt
0 siblings, 1 reply; 29+ messages in thread
From: Michal Suchanek @ 2022-10-22 21:22 UTC (permalink / raw)
To: u-boot; +Cc: Michal Suchanek, Simon Glass, Heinrich Schuchardt
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-22 21:22 ` [PATCH] sandbox: Correctly define BITS_PER_LONG Michal Suchanek
@ 2022-10-22 21:52 ` Heinrich Schuchardt
2022-10-23 7:50 ` Michal Suchánek
0 siblings, 1 reply; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-22 21:52 UTC (permalink / raw)
To: Michal Suchanek, u-boot; +Cc: Simon Glass
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;
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-22 21:52 ` Heinrich Schuchardt
@ 2022-10-23 7:50 ` Michal Suchánek
2022-10-23 7:56 ` Heinrich Schuchardt
0 siblings, 1 reply; 29+ messages in thread
From: Michal Suchánek @ 2022-10-23 7:50 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, Simon Glass
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;
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-23 7:50 ` Michal Suchánek
@ 2022-10-23 7:56 ` Heinrich Schuchardt
2022-10-23 11:30 ` Michal Suchánek
2023-03-01 20:14 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Heinrich Schuchardt @ 2022-10-23 7:56 UTC (permalink / raw)
To: Michal Suchánek; +Cc: u-boot, Simon Glass, 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;
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-23 7:56 ` Heinrich Schuchardt
@ 2022-10-23 11:30 ` Michal Suchánek
2023-03-01 20:14 ` Simon Glass
1 sibling, 0 replies; 29+ messages in thread
From: Michal Suchánek @ 2022-10-23 11:30 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, 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(-)
> > > >
> > > > 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;
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sandbox: Correctly define BITS_PER_LONG
2022-10-23 7:56 ` Heinrich Schuchardt
2022-10-23 11:30 ` Michal Suchánek
@ 2023-03-01 20:14 ` Simon Glass
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2023-03-01 20:14 UTC (permalink / raw)
To: Michal Suchánek; +Cc: u-boot, Simon Glass, Heinrich Schuchardt
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!
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2023-03-01 20:15 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-13 20:28 [PATCH] tests: Build correct sandbox configuration on 32bit Michal Suchanek
2022-10-14 3:05 ` Heinrich Schuchardt
2022-10-14 8:43 ` Michal Suchánek
2022-10-15 5:00 ` Heinrich Schuchardt
2022-10-14 15:56 ` Simon Glass
2022-10-14 20:52 ` [PATCH v2] " Michal Suchanek
2022-10-15 4:54 ` Heinrich Schuchardt
2022-10-15 7:19 ` Michal Suchánek
2022-10-15 17:53 ` Simon Glass
2022-10-15 18:31 ` Heinrich Schuchardt
2022-10-15 18:39 ` Simon Glass
2022-10-15 19:05 ` Heinrich Schuchardt
2022-10-15 19:17 ` Michal Suchánek
2022-10-15 19:35 ` Heinrich Schuchardt
2022-10-15 19:24 ` Simon Glass
2022-10-15 19:29 ` Heinrich Schuchardt
2022-10-15 19:46 ` Simon Glass
2022-10-15 20:27 ` Heinrich Schuchardt
2022-10-17 7:28 ` Michal Suchánek
2022-10-19 13:18 ` Simon Glass
2022-10-22 1:05 ` Simon Glass
2022-10-22 19:38 ` Michal Suchánek
2022-10-22 21:22 ` [PATCH] sandbox: Correctly define BITS_PER_LONG Michal Suchanek
2022-10-22 21:52 ` Heinrich Schuchardt
2022-10-23 7:50 ` Michal Suchánek
2022-10-23 7:56 ` Heinrich Schuchardt
2022-10-23 11:30 ` Michal Suchánek
2023-03-01 20:14 ` Simon Glass
2022-10-15 5:05 ` [PATCH] tests: Build correct sandbox configuration on 32bit Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.