Re: [PATCH v2 2/2] xen/device-tree: add a helper to dump an FDT in DTS-like format

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/14/26 1:13 PM, Orzel, Michal wrote:
> On 12-Oct-26 09:34, Oleksii Kurochko wrote:
> > The device tree Xen builds for dom0/hwdom or for a dom0less domain is
> > only ever consumed by the guest, so a mistake in it surfaces as an
> > obscure failure inside the domain, far away from the code which
> > produced the tree.
> >
> > Add device_tree_dump(). The output follows dtc's format so that a dump
> > can be diffed against the source DTS or fed back to dtc; the value
> > formatting heuristics are ported from dtc's utilfdt_print_data() for
> > that reason. The walk itself goes through libfdt rather than decoding
> > the tag stream by hand as dtc's fdtdump does.
> >
> > Dumping a whole tree is only useful while debugging and costs both code
> > size and a lot of console output, so it is put under its own option,
> > CONFIG_DEVICE_TREE_DUMP. It is separate from CONFIG_DEVICE_TREE_DEBUG
> > because the generated tree is often all that is needed, without the rest
> > of the device tree debug messages.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Dump the device tree generated for dom0/hwdom as well.
> >   - Put the dump under its own CONFIG_DEVICE_TREE_DUMP instead of
> >     CONFIG_DEVICE_TREE_DEBUG.
> >   - Pass the domain to device_tree_dump() and print the "Device tree for"
> >     header from it.
> >   - Combine isprint() with isascii(), as Xen's isprint() also accepts
> >     the characters 0xa0-0xff, unlike the one dtc uses.
> >   - Update the commit message.
> > ---
> > ---
> >   xen/Kconfig.debug                       |  10 ++
> >   xen/arch/arm/domain_build.c             |   2 +
> >   xen/common/device-tree/Makefile         |   1 +
> >   xen/common/device-tree/dom0less-build.c |   3 +
> >   xen/common/device-tree/fdt-dump.c       | 185 ++++++++++++++++++++++++
> >   xen/include/xen/bootfdt.h               |  16 ++
> >   6 files changed, 217 insertions(+)
> >   create mode 100644 xen/common/device-tree/fdt-dump.c
> >
> > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> > index d900d926c555..45cff9dbc467 100644
> > --- a/xen/Kconfig.debug
> > +++ b/xen/Kconfig.debug
> > @@ -98,6 +98,16 @@ config DEVICE_TREE_DEBUG
> >           logged in the Xen ring buffer.
> >           If unsure, say N here.
> >   
> > +config DEVICE_TREE_DUMP
> > +       bool "Dump generated device trees"
> > +       depends on DEVICE_TREE_PARSE
> > +       help
> > +         The device tree Xen generates for dom0/hwdom and for each dom0less
> > +         domain is printed on the Xen console in a DTS-like format.
> > +         The dump is logged at debug level, so on a non-debug build it is
> > +         only shown with "loglvl=all".
> That's not correct. loglvl="debug" is enough. That said, I don't find it
> particularly necessary for the second part of the sentence to be part of the
> help message. If you want to keep it, fix the loglvl.
I am okay to drop the second part of the sentence to be part of the
help message.

> > +         If unsure, say N here.
> > +
> >   config SCRUB_DEBUG
> >         bool "Page scrubbing test"
> >         default DEBUG
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 72d531618045..6a20af23e1bb 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1705,6 +1705,8 @@ static int __init prepare_dtb_hwdom(struct domain *d, 
> > struct kernel_info *kinfo)
> >       if ( ret < 0 )
> >           goto err;
> >   
> > +    device_tree_dump(d, kinfo->fdt);
> > +
> >       return 0;
> >   
> >     err:
> > diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> > index 9036e455d66a..730d202349eb 100644
> > --- a/xen/common/device-tree/Makefile
> > +++ b/xen/common/device-tree/Makefile
> > @@ -6,6 +6,7 @@ obj-$(CONFIG_DOMAIN_BUILD_HELPERS) += domain-build.init.o
> >   obj-$(filter $(CONFIG_DOM0LESS_BOOT),$(CONFIG_HAS_DEVICE_TREE_DISCOVERY)) += 
> > dom0less-build.init.o
> >   obj-$(CONFIG_DOM0LESS_BOOT) += dom0less-bindings.init.o
> >   obj-$(CONFIG_OVERLAY_DTB) += dt-overlay.o
> > +obj-$(CONFIG_DEVICE_TREE_DUMP) += fdt-dump.init.o
> >   obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += intc.o
> >   obj-$(CONFIG_DOMAIN_BUILD_HELPERS) += kernel.o
> >   obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
> > diff --git a/xen/common/device-tree/dom0less-build.c 
> > b/xen/common/device-tree/dom0less-build.c
> > index fcbeb8adbd73..24a9c6b1e3e1 100644
> > --- a/xen/common/device-tree/dom0less-build.c
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -1,5 +1,6 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> >   
> > +#include <xen/bootfdt.h>
> >   #include <xen/bootinfo.h>
> >   #include <xen/device_tree.h>
> >   #include <xen/dom0less-build.h>
> > @@ -587,6 +588,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct 
> > kernel_info *kinfo)
> >       if ( ret < 0 )
> >           goto err;
> >   
> > +    device_tree_dump(d, kinfo->fdt);
> > +
> >       return 0;
> >   
> >     err:
> > diff --git a/xen/common/device-tree/fdt-dump.c 
> > b/xen/common/device-tree/fdt-dump.c
> > new file mode 100644
> > index 000000000000..296ae0d90d12
> > --- /dev/null
> > +++ b/xen/common/device-tree/fdt-dump.c
> > @@ -0,0 +1,185 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Dump a flattened device tree blob in a DTS-like format.
> > + *
> > + * is_printable_string() and print_value() are derived from
> > + * util_is_printable_string() and utilfdt_print_data() in the device tree
> > + * compiler (dtc).
> > + *
> > + * Copyright 2011 The Chromium Authors, All Rights Reserved.
> > + * Copyright 2008 Jon Loeliger, Freescale Semiconductor, Inc.
> > + * util_is_printable_string contributed by
> > + *      Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> > + */
> > +
> > +#include <xen/bootfdt.h>
> > +#include <xen/ctype.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/types.h>
> > +
> > +/* Deeper nodes are still printed, but not indented any further. */
> > +#define DUMP_MAX_INDENT_DEPTH 32
> > +
> > +/* Indentation, in characters, of a node at depth @depth. */
> > +static int __init indent_of(int depth)
> > +{
> > +    return 2 * min(depth, DUMP_MAX_INDENT_DEPTH);
> Can we use 4 spaces instead of 2 here and in print_properties() call. It does
> look more readable IMO and matches the indentation (1 tab = 4 spaces) of dtc
> when converting dtb to dts.
Fair enough.

I will introduce then DUMP_IDENT_WIDTH and use it in the correspondent 
places:
--- a/xen/common/device-tree/fdt-dump.c
+++ b/xen/common/device-tree/fdt-dump.c
@@ -22,10 +22,13 @@
 /* Deeper nodes are still printed, but not indented any further. */
 #define DUMP_MAX_INDENT_DEPTH 32

+/* Spaces per nesting level. */
+#define DUMP_INDENT_WIDTH 4
+
 /* Indentation, in characters, of a node at depth @depth. */
 static int __init indent_of(int depth)
 {
-    return 2 * min(depth, DUMP_MAX_INDENT_DEPTH);
+    return DUMP_INDENT_WIDTH * min(depth, DUMP_MAX_INDENT_DEPTH);
 }
@@ -161,7 +164,7 @@ void __init device_tree_dump(const struct domain *d, 
const void *fdt)
         printk(XENLOG_DEBUG "%*s%s {\n", indent_of(depth), "",
                (depth == 0) ? "/" : (name ?: "<unknown>"));
-        print_properties(fdt, node, indent_of(depth) + 2);
+        print_properties(fdt, node, indent_of(depth) + DUMP_INDENT_WIDTH);

> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
Thanks!

~ Oleksii