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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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