Re: [PATCH 6/9] test: cmd/fdt: do not use fixed buffer addresses

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3942 bytes --]

On Fri, Nov 14, 2025 at 11:10:59AM -0700, Simon Glass wrote:
> On Fri, 14 Nov 2025 at 07:43, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Nov 14, 2025 at 07:24:53AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 14 Nov 2025 at 07:19, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Nov 14, 2025 at 05:31:40AM -0700, Simon Glass wrote:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Sun, 9 Nov 2025 at 03:10, Heinrich Schuchardt
> > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > >
> > > > > > The location of memory depends on the board. Do not assume memory at fixed
> > > > > > memory locations. Use memalign() instead to allocate a buffer.
> > > > > >
> > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > [snip]
> > > > > >         /* bytes */
> > > > > >         print_hex_dump_bytes("", DUMP_PREFIX_ADDRESS, buf, 0x12);
> > > > > >         ut_assert_nextline("%0*lx: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff  ..\"3DUfw........",
> > > > > > -                          IS_ENABLED(CONFIG_PHYS_64BIT) ? 16 : 8,
> > > > > > -                          (uintptr_t)buf);
> > > > > > +                          IS_ENABLED(CONFIG_PHYS_64BIT) ? 16 : 8, addr);
> > > > [snip]
> > > > > This is adding memory allocations to a test for hex dumping.
> > > > >
> > > > > It would be better and simpler to use a fixed address and make this a
> > > > > sandbox-only test. I struggle to see the value of running these sorts
> > > > > of tests under QEMU?
> > > >
> > > > Removing context to highlight value of running tests on multiple
> > > > platforms.
> > >
> > > $ ./tools/qconfig.py -f ~PHYS_64BIT -l |grep sandbox
> > > sandbox
> > > sandbox_flattree
> > > sandbox_nocmdline
> > > sandbox_noinst
> > > sandbox_spl
> > > sandbox_vpl
> > > $ ./tools/qconfig.py -f PHYS_64BIT -l |grep sandbox
> > > sandbox64
> > > sandbox64_lwip
> 
> If at any point you have made up your mind, please say so, rather than
> continuing what I intended to be a discussion of the pros and cons of
> this patch.

Yes, both Heinrich and I agree we should be running these tests on
hardware. Don't run them on hardware as a position has been rejected.
You can stop reading here if you don't want an explanation.

> > Good, and as soon as you restrict to "sandbox" we stop running
> > "sandbox64", it's why sandbox64 runs so much quicker in CI.
> 
> This is a C test - test/cmd/fdt.c so it is built for all sandbox
> boards. The restriction you are referring to here is for pytests, I
> believe. The core of my argument is that running sandbox tests on
> other architectures is mostly a waste of time, assuming the compiler
> is functioning correctly. This patch is also devaluing sandbox, the
> major advantages of which is its fast, native code execution and fixed
> execution environment (memory map, etc.).
> 
> At the very least, it would help to be clear what bugs we are hoping
> to find with this change.

The position of "don't run tests on hardware, only on sandbox" does not
sound sensible in general. In practice we need to do more, not less, on
device testing and saving milliseconds by skipping tests is noise lost
in the time it takes to acquire a runner and clone the source code or
even which runner we test things on. Spending time on "should this run
on hardware or only sandbox" is time not well spent. Finally, "do a
bunch of stuff on hardware" is still the best overall method for finding
unexpected platform bugs. It's how we catch "some clocks are wrong" or
"we configured thermals wrong" or "we configured memory wrong" and so
forth.

You mention QEMU, and yes, we will be executing these on QEMU in CI, on
some platforms, which are our fastest pytest pipelines. But we will also
being running these on hardware, which is my point and I believe
Heinrich's as well.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]