Re: [PATCH v3 06/15] Introduce structured tag value definition

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Sep 10, 2026 at 09:41:26AM +0200, Herve Codina wrote:
> Hi David,
> 
> On Thu, 10 Sep 2026 14:51:08 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Aug 26, 2026 at 10:31:37AM +0200, Herve Codina wrote:
> > > The goal of structured tag values is to ease the introduction of new
> > > tags in future releases with the capability for an already existing
> > > release to ignore those structured tags. In order to do that data length
> > > related to the unknown tag needs to be identified.
> > > 
> > > Also add a flag to tell an old release if this tag can be simply skipped
> > > or must lead to an error.  
> > 
> > I suggested/insisted on this in the past, but I've since realised this
> > isn't actually useful.  The "structured tag" format is only useful
> > because it allows you to skip over unknown tags, which means there's
> > no point using it for ags that can't be skipped.  If we need new tags
> > that can't be safely skipped, they can be added as old-style tags
> > instead.
> >
> > In which case "old style" versus "new style" is no longer a good
> > characterization: they would exist side by side, so the distinction is
> > more "skippable" versus "non skippable" tags.  Which suggests that
> > "skippable tags" or "metadata tags" might be a better term than
> > "structured tags" - that would focus more on the why than the how.
> 
> Do you mean that the SKIP_SAFE bit should be removed ?

Yes.

> I like the defined structure with the DATA_LEN_ENCODING part. Even for tags
> which are not "skippable".
> 
> This ensures a kind of standardized format for all future tags instead of a 
> specific definition (related to the length of the data) for each new tag.

If we were designing the dtb format from scratch, I'd agree.  But
given we already have what we have I think the drawbacks of
introducing a second way of doing tags outweighs the benefits.

> For this kind of information related to the length of data, I prefer a global
> rule instead of tag-specific rules.

Right, but it can't truly be global, because we have the existing
tags.

> > > Structured tag value is defined on 32bit and is defined as follow:
> > > 
> > >   Bits  | 31 | 30        | 29             28 | 27    0|
> > >   ------+----+-----------+-------------------+--------+
> > >   Fields| 1  | SKIP_SAFE | DATA_LEN_ENCODING | TAG_ID |
> > >   ------+----+-----------+-------------------+--------+
> > > 
> > > Bit 31 is always set to 1 to identify a structured tag value.
> > > 
> > > Bit 30 (SKIP_SAFE) is set to 1 if the tag can be safely ignored when its
> > > TAG_ID value is not a known value (unknown tag). If the SKIP_SAFE bit is
> > > set to 0 this tag must not be ignored and an error should be reported
> > > when its TAG_ID value is not a known value (unknown tag).
> > > 
> > > Bits 29..28 (DATA_LEN_ENCODING) indicates the length of the data related
> > > to the tag. Following values are possible:
> > >   - 0b00: No data.
> > >           The tag is followed by the next tag value.  
> > 
> > I think "next tag." would be clearer than "next tag value." - "next
> > tag value" could be confused as meaning the data that comes after the
> > tag.
> 
> Ok, will be updated.
> 
> > 
> > >   - 0b01: 1 cell data
> > >           The tag is followed by a 1 cell (u32) data. The next tag is
> > >           available after this cell.  
> > 
> > Remove "available", it doesn't add any information.
> 
> Ok, will be updated.
> 
> > 
> > >   - 0b10: 2 cells data
> > >           The tag is followed by a 2 cells (2 * u32) data. The next tag
> > >           is available after those two cells.  
> > 
> > Ditto.
> 
> Will be update as well.
> 
> > 
> > >   - 0b11: Data length encoding
> > >           The tag is followed by a cell (u32) indicating the size of the
> > >           data. This size is given in bytes. Data are available right
> > >           after this cell.
> > > 
> > >           The next tag is available after the data. Padding is present
> > >           after the data in order to have the next tag aligned on 32bits.
> > >           This padding is not included in the size of the data.  
> > 
> > I'm guessing the 1 & 2 cell cases are pretty common so it saves a
> > moderate amount of dtb size to have this encoding.  However, it does
> > come at the cost of greater code complexity.  Is it worth it?  I'm
> > willing to believe it is, but I think the case needs to be made.
> 
> In term of code complexity, FDT_PROPDATA_PHANDLE introduced in the addons
> series, patch 7/74 [0], is a 1-cell (or 1-fdt32) "structured" tag.
> 
> It can be compared with FDT_PROPDATA_PHANDLE_REF, introduced in patch 11/74 [1].
> FDT_PROPDATA_PHANDLE_REF is a "Data length encoding" tag.
> 
> Modifications in libfdt/fdt.c are pretty similar for both tags.

Right, it won't be in the case of specific tag handling.  It's the
general case skipping that's more complex, because it has to consider
both cases.

> [0] https://lore.kernel.org/all/20260826094950.1088288-8-herve.codina@bootlin.com/
> [1] https://lore.kernel.org/all/20260826094950.1088288-12-herve.codina@bootlin.com/
> 
> > 
> > I'd also avoid the term "cell" here.  To me a "cell" is specifically a
> > term applying to a 32-bit value _within a device tree property_.
> > These are 32-bits, but not part of a property.  Plus if Segher
> > reappears, he'll complain that in the old days of OF, a cell wasn't
> > necessarily 32-bits :).
> 
> ok, I will avoid the term "cell" in the next iteration.
> 
> > 
> > > Bits 27..0 (TAG_ID) is the tag identifier defining a specific tag.
> > > 
> > > Introduce the structured tag values definition and some specific tags
> > > reserved for tests based on this structure definition.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  libfdt/fdt.h | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/libfdt/fdt.h b/libfdt/fdt.h
> > > index a07abfcc..f41a355f 100644
> > > --- a/libfdt/fdt.h
> > > +++ b/libfdt/fdt.h
> > > @@ -49,6 +49,7 @@ struct fdt_property {
> > >  
> > >  #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
> > >  #define FDT_TAGSIZE	sizeof(fdt32_t)
> > > +#define FDT_CELLSIZE	sizeof(fdt32_t)  
> > 
> > I'd avoid creating this constant for similar reasons to avoid the term
> > "cell" above.
> 
> Ok, I will avoid the constant and use sizeof(fdt32_t) directly when needed
> in the code.
> 
> Also, I will avoid the CELL in all other part of this file.
> 
> Best regards,
> Hervé
> 

-- 
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 --]