Hi Heinrich, On Sat, 15 Oct 2022 at 13:05, Heinrich Schuchardt <xypron.g...@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.g...@gmx.de> > > wrote: > >> > >> On 10/15/22 19:53, Simon Glass wrote: > >>> Hi Michal, > >>> > >>> On Fri, 14 Oct 2022 at 14:53, Michal Suchanek <msucha...@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 <msucha...@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 <s...@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