Hi Randolph,
On Wed, 13 May 2026 at 14:22, Randolph Sapp <[email protected]> wrote:
>
> On Wed May 13, 2026 at 11:34 AM CDT, Simon Glass wrote:
> > Hi Randolph,
> >
> > On 2026-05-13T01:56:06, Randolph Sapp <[email protected]> wrote:
> >> test: boot: add a fdt reserved region check
> >>
> >> Add a image_fdt suite and a check for boot_fdt_add_mem_rsv_regions. This
> >> will ensure the user is properly informed of any reservation failures.
> >> It will also validate that reservations are cleaned up correctly when
> >> switching FDTs.
> >>
> >> Signed-off-by: Randolph Sapp <[email protected]>
> >>
> >> test/boot/Makefile | 1 +
> >> test/boot/image_fdt.c | 53
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> test/cmd_ut.c | 2 ++
> >> test/py/tests/test_suite.py | 2 +-
> >> 4 files changed, 57 insertions(+), 1 deletion(-)
> >
> > Change log?
>
> No change log for a patch that didn't exist in the series previously.
>
> >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c
> >> @@ -0,0 +1,53 @@
> >> +#include <fdt_support.h>
> >> +#include <asm/global_data.h>
> >> +#include <config.h>
> >> +#include <image.h>
> >> +#include <lmb.h>
> >> +#include <malloc.h>
> >> +#include <test/test.h>
> >> +#include <test/ut.h>
> >
> > Please sort these: config.h (if needed) first, then the plain
> > top-level headers alphabetically (fdt_support.h, image.h, lmb.h,
> > malloc.h), then asm/global_data.h, then test/*.
> >
> >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c
> >> @@ -0,0 +1,53 @@
> >> +#define FDTDEC_MAX_SIZE (2 * 1024 * 1024)
> >> +
> >> +#define IMAGE_FDT_TEST(_name, _flags) UNIT_TEST(_name, _flags, image_fdt)
> >
> > FDTDEC_MAX_SIZE is never referenced - looks copied from
> > test/dm/fdtdec.c - please drop it.
> >
> >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c
> >> @@ -0,0 +1,53 @@
> >> + /* Loading a new device tree should be allowed */
> >> + blob = malloc(fdt_sz);
> >> + ut_assertok_ptr(blob);
> >> + memcpy(blob, gd->fdt_blob, fdt_sz);
> >> +
> >> + nodeoffset = fdt_path_offset(blob, '/reserved-memory');
> >> + ut_assertok(fdt_del_node(blob, nodeoffset));
> >
> > If /reserved-memory is missing, fdt_path_offset() returns a negative
> > libfdt error and fdt_del_node() then fails with a confusing message.
> > Please ut_assert(nodeoffset >= 0) first.
> >
> > Also, blob is malloc()'d but never freed.
> >
> >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c
> >> @@ -0,0 +1,53 @@
> >> + boot_fdt_add_mem_rsv_regions(blob);
> >> + ut_assert_console_end();
> >> + boot_relocate_fdt((char **)&blob, &fdt_sz);
> >> +
> >> + /* Reservation should not exist now */
> >> + ut_asserteq(0, lmb_is_reserved_flags(start, LMB_NOMAP));
> >
> > What is the boot_relocate_fdt() call testing - could you add a
> > comment? The reservation at start is already gone after
> > boot_fdt_add_mem_rsv_regions(blob), so the final ut_asserteq() does
> > not depend on it. It also allocates from LMB for the relocated copy,
> > which then leaks. I'd drop it, or split it into a separate test with
> > its own assertions. The (char **)&blob cast is a hint that the call
> > doesn't belong here.
> >
> >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c
> >> @@ -0,0 +1,53 @@
> >> +IMAGE_FDT_TEST(test_boot_fdt_add_mem_rsv_regions, UTF_LIVE_TREE |
> >> UTF_CONSOLE);
> >
> > Just to check: the test only touches gd->fdt_blob (the flat tree), so
> > why UTF_LIVE_TREE? If you want it to run regardless of OF_LIVE, drop
> > the flag (or use UTF_FLAT_TREE). If the intent was to avoid running
> > twice, please mention that in a comment.
> >
> > Regards,
> > Simon
>
> As far as I'm aware, UTF_LIVE_TREE is required for this test. I've tried using
> UTF_FLAT_TREE and no UTF.*TREE options and for some reason this restricts part
> of the parsing that's required for boot_fdt_add_mem_rsv_regions. Operations
> that
> should report errors simply do not without UTF_LIVE_TREE.
I dug into this. The flag isn't affecting parsing -
boot_fdt_add_mem_rsv_regions() uses libfdt directly on gd->fdt_blob,
so uts->of_live is invisible to it. What you're actually hitting is in
the test runner, ut_run_test_live_flat() in test/test-main.c:
/* live branch */
if (CONFIG_IS_ENABLED(OF_LIVE)) {
if (!(test->flags & UTF_FLAT_TREE)) {
uts->of_live = true;
ret = ut_run_test(...);
...
/* flat branch */
if ((!CONFIG_IS_ENABLED(OF_LIVE) ||
(test->flags & UTF_SCAN_FDT)) &&
!(test->flags & UTF_LIVE_TREE) &&
...
Sandbox has OF_LIVE=y. With UTF_FLAT_TREE alone:
- live branch is skipped (UTF_FLAT_TREE set)
- flat branch is skipped (OF_LIVE on, UTF_SCAN_FDT not set)
So the test body never runs, and the assertions never fire. That looks
like 'operations that should report errors simply do not' but really
the test just isn't being called.
I reproduced this on your series: with UTF_FLAT_TREE | UTF_CONSOLE the
runner prints 'Running 1 image_fdt tests' followed by 'Tests run: 1,
failures: 0' and never emits the per-test 'Test: ...' line.
The cleanest fix is to drop the tree flag entirely - just UTF_CONSOLE.
On sandbox (OF_LIVE=y) the runner will execute it once with
of_live=true and skip the second flat pass on its own, same observable
behaviour as UTF_LIVE_TREE. As a bonus, on a hypothetical !OF_LIVE
build it would still run via the flat branch, where UTF_LIVE_TREE
would silently skip it.
I also tried UTF_FLAT_TREE | UTF_SCAN_FDT for completeness - that
makes dm_extended_scan() fail with -EEXIST because devices are already
bound, so it's not a viable alternative:
scmi_bind_protocols() sandbox-scmi_agent scmi: Cannot have more than
one SCMI agent
This may be a bug in SCMI (e.g. it should use devicetree).
Regards,
Simon
