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

[Heinrich Schuchardt]

On 10/15/22 21:46, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, 15 Oct 2022 at 13:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> Am 15. Oktober 2022 21:24:36 MESZ schrieb Simon Glass <sjg at chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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 at 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 at 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