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

Sat Oct 22 21:38:32 CEST 2022

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 at 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 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.
> >
> > 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!