[PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[Tom Rini]

In this function from fdt_check.c we have (reasonably and as the name
implies) a number of checks on the DTB. However, there are cases where
we may wish to assume that we have been given a perfect DTB already and
do nothing here. Add a test for can_assume(PERFECT) as the first check
in this function and if true, perform no checks.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Along the lines of the patches I posted back in December, in U-Boot SPL
we just don't have the space for this check much of the time and so have
always omitted it (going back to at least when Simon posted the initial
patch to make libfdt/fdt_check.c here). This is another case where it's
a noticeable size win for us. I had missed this change in particular
because we had in turn missed catching up on fdt_check_full being moved
out of fdt_ro.c and in to fdt_check.c.
---
 libfdt/fdt_check.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
index cca052353213..2fd5b61d016a 100644
--- a/libfdt/fdt_check.c
+++ b/libfdt/fdt_check.c
@@ -21,6 +21,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 	const char *propname;
 	bool expect_end = false;
 
+	if (can_assume(PERFECT))
+		return 0;
 	if (bufsize < FDT_V1_SIZE)
 		return -FDT_ERR_TRUNCATED;
 	if (bufsize < fdt_header_size(fdt))
-- 
2.43.0

Re: [PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[David Gibson]

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

On Tue, May 26, 2026 at 02:30:22PM -0600, Tom Rini wrote:
> In this function from fdt_check.c we have (reasonably and as the name
> implies) a number of checks on the DTB. However, there are cases where
> we may wish to assume that we have been given a perfect DTB already and
> do nothing here. Add a test for can_assume(PERFECT) as the first check
> in this function and if true, perform no checks.
> 
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Along the lines of the patches I posted back in December, in U-Boot SPL
> we just don't have the space for this check much of the time and so have
> always omitted it (going back to at least when Simon posted the initial
> patch to make libfdt/fdt_check.c here). This is another case where it's
> a noticeable size win for us. I had missed this change in particular
> because we had in turn missed catching up on fdt_check_full being moved
> out of fdt_ro.c and in to fdt_check.c.

I'm not necessarily against this, but I have some misgivings.

fdt_check_full() is (deliberately) not called from anywhere else in
libfdt - it's intended to allow the user to explicitly do a full
validity check on the tree.  Given that meaning, I'm not sure it's
wise to turn it into a no-op based on the assume flags.

Your comment seems to imply that the issue here is size - simply
having this function compiled - rather than being too expensive when
(explicitly) called.  That's a little surprising to me - it's in its
own compilation unit, specifically so that the linker can omit it if
it's not used.  Is there something unusual about your build
environment that's not letting that happen?

> ---
>  libfdt/fdt_check.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
> index cca052353213..2fd5b61d016a 100644
> --- a/libfdt/fdt_check.c
> +++ b/libfdt/fdt_check.c
> @@ -21,6 +21,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
>  	const char *propname;
>  	bool expect_end = false;
>  
> +	if (can_assume(PERFECT))
> +		return 0;
>  	if (bufsize < FDT_V1_SIZE)
>  		return -FDT_ERR_TRUNCATED;
>  	if (bufsize < fdt_header_size(fdt))
> -- 
> 2.43.0
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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

Re: [PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[Tom Rini]

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

On Wed, May 27, 2026 at 02:23:20PM +1000, David Gibson wrote:
> On Tue, May 26, 2026 at 02:30:22PM -0600, Tom Rini wrote:
> > In this function from fdt_check.c we have (reasonably and as the name
> > implies) a number of checks on the DTB. However, there are cases where
> > we may wish to assume that we have been given a perfect DTB already and
> > do nothing here. Add a test for can_assume(PERFECT) as the first check
> > in this function and if true, perform no checks.
> > 
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Along the lines of the patches I posted back in December, in U-Boot SPL
> > we just don't have the space for this check much of the time and so have
> > always omitted it (going back to at least when Simon posted the initial
> > patch to make libfdt/fdt_check.c here). This is another case where it's
> > a noticeable size win for us. I had missed this change in particular
> > because we had in turn missed catching up on fdt_check_full being moved
> > out of fdt_ro.c and in to fdt_check.c.
> 
> I'm not necessarily against this, but I have some misgivings.
> 
> fdt_check_full() is (deliberately) not called from anywhere else in
> libfdt - it's intended to allow the user to explicitly do a full
> validity check on the tree.  Given that meaning, I'm not sure it's
> wise to turn it into a no-op based on the assume flags.
> 
> Your comment seems to imply that the issue here is size - simply
> having this function compiled - rather than being too expensive when
> (explicitly) called.  That's a little surprising to me - it's in its
> own compilation unit, specifically so that the linker can omit it if
> it's not used.  Is there something unusual about your build
> environment that's not letting that happen?

So, we have code like this:
        /* Get the total space reserved for FDT in blob */
        live_fdt = bloblist_get_blob(BLOBLISTT_CONTROL_FDT, &blob_size);
        if (live_fdt != gd->fdt_blob)
                return -ENOENT;

        ret = fdt_check_full(live_fdt, blob_size);
        if (ret)
                return fdtdec_ret_to_errno(ret);

And this is compiled on TPL, SPL and full U-Boot builds. On the first
two, we're just too space constrained to do this check. So it's not the
linker doing the right thing or not, it's avoiding having to #if the
code directly (or rather, CONFIG_VAL(...)). My line of thinking was that
since ASSUME_PERFECT is that everything is really perfect, this is the
way to go. Yes, it's a little odd to have both "call the validation
function" and "the validation function does not validate" but that's
just because in the second case, we explicitly configured ourself to not
validate anything.

And FWIW, that's really how we use the assume mask in U-Boot, either
0xff or 0x0. It's a case where we're either passing along the tree we
bundled with ourself (and so we can assume it's fine) or it's passed
along (and we verify).

-- 
Tom

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

Re: [PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[Simon Glass]

On Tue, 26 May 2026 at 14:30, Tom Rini <trini@konsulko.com> wrote:
>
> In this function from fdt_check.c we have (reasonably and as the name
> implies) a number of checks on the DTB. However, there are cases where
> we may wish to assume that we have been given a perfect DTB already and
> do nothing here. Add a test for can_assume(PERFECT) as the first check
> in this function and if true, perform no checks.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Along the lines of the patches I posted back in December, in U-Boot SPL
> we just don't have the space for this check much of the time and so have
> always omitted it (going back to at least when Simon posted the initial
> patch to make libfdt/fdt_check.c here). This is another case where it's
> a noticeable size win for us. I had missed this change in particular
> because we had in turn missed catching up on fdt_check_full being moved
> out of fdt_ro.c and in to fdt_check.c.
> ---
>  libfdt/fdt_check.c | 2 ++
>  1 file changed, 2 insertions(+)

This does honour the flags so...

Reviewed-by: Simon Glass <sjg@chromium.org>


>
> diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
> index cca052353213..2fd5b61d016a 100644
> --- a/libfdt/fdt_check.c
> +++ b/libfdt/fdt_check.c
> @@ -21,6 +21,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
>         const char *propname;
>         bool expect_end = false;
>
> +       if (can_assume(PERFECT))
> +               return 0;
>         if (bufsize < FDT_V1_SIZE)
>                 return -FDT_ERR_TRUNCATED;
>         if (bufsize < fdt_header_size(fdt))
> --
> 2.43.0
>
>

Re: [PATCH] libfdt: fdt_check_full: Add can_assume(PERFECT) check

[Tom Rini]

On Tue, May 26, 2026 at 10:38:59PM -0600, Tom Rini wrote:
> On Wed, May 27, 2026 at 02:23:20PM +1000, David Gibson wrote:
> > On Tue, May 26, 2026 at 02:30:22PM -0600, Tom Rini wrote:
> > > In this function from fdt_check.c we have (reasonably and as the name
> > > implies) a number of checks on the DTB. However, there are cases where
> > > we may wish to assume that we have been given a perfect DTB already and
> > > do nothing here. Add a test for can_assume(PERFECT) as the first check
> > > in this function and if true, perform no checks.
> > > 
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Along the lines of the patches I posted back in December, in U-Boot SPL
> > > we just don't have the space for this check much of the time and so have
> > > always omitted it (going back to at least when Simon posted the initial
> > > patch to make libfdt/fdt_check.c here). This is another case where it's
> > > a noticeable size win for us. I had missed this change in particular
> > > because we had in turn missed catching up on fdt_check_full being moved
> > > out of fdt_ro.c and in to fdt_check.c.
> > 
> > I'm not necessarily against this, but I have some misgivings.
> > 
> > fdt_check_full() is (deliberately) not called from anywhere else in
> > libfdt - it's intended to allow the user to explicitly do a full
> > validity check on the tree.  Given that meaning, I'm not sure it's
> > wise to turn it into a no-op based on the assume flags.
> > 
> > Your comment seems to imply that the issue here is size - simply
> > having this function compiled - rather than being too expensive when
> > (explicitly) called.  That's a little surprising to me - it's in its
> > own compilation unit, specifically so that the linker can omit it if
> > it's not used.  Is there something unusual about your build
> > environment that's not letting that happen?
> 
> So, we have code like this:
>         /* Get the total space reserved for FDT in blob */
>         live_fdt = bloblist_get_blob(BLOBLISTT_CONTROL_FDT, &blob_size);
>         if (live_fdt != gd->fdt_blob)
>                 return -ENOENT;
> 
>         ret = fdt_check_full(live_fdt, blob_size);
>         if (ret)
>                 return fdtdec_ret_to_errno(ret);
> 
> And this is compiled on TPL, SPL and full U-Boot builds. On the first
> two, we're just too space constrained to do this check. So it's not the
> linker doing the right thing or not, it's avoiding having to #if the
> code directly (or rather, CONFIG_VAL(...)). My line of thinking was that
> since ASSUME_PERFECT is that everything is really perfect, this is the
> way to go. Yes, it's a little odd to have both "call the validation
> function" and "the validation function does not validate" but that's
> just because in the second case, we explicitly configured ourself to not
> validate anything.
> 
> And FWIW, that's really how we use the assume mask in U-Boot, either
> 0xff or 0x0. It's a case where we're either passing along the tree we
> bundled with ourself (and so we can assume it's fine) or it's passed
> along (and we verify).

Hey, just checking if there's any further discussion here? I want to try
and get the delta between U-Boot and upstream down to zero if we can.
Thanks!

-- 
Tom