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

[Heinrich Schuchardt]

Am 15. Oktober 2022 21:17:12 MESZ schrieb "Michal Suchánek" <msuchanek at 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 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.
>
>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