[PATCH 0/2] xen/device-tree: FDT dump helper and common discard_initial_modules()

[PATCH 0/2] xen/device-tree: FDT dump helper and common discard_initial_modules()

[Oleksii Kurochko]

Two small, independent device-tree changes. Arm is the only user of both
at the moment; RISC-V is going to use them as well.

Patch 1 adds device_tree_dump(), which prints an FDT in a DTS-like format
under CONFIG_DEVICE_TREE_DEBUG, to make it possible to inspect the device
tree Xen generates for a dom0less domain.

Patch 2 moves discard_initial_modules() from Arm to common device-tree
code, as everything it uses is already common, so that other device-tree
based architectures can reuse it. No functional change.

Build-tested on arm64 and arm32.

CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2900841778


Oleksii Kurochko (2):
  xen/device-tree: add a helper to dump an FDT in DTS-like format
  xen/device-tree: move discard_initial_modules() to common code

 xen/Kconfig.debug                       |   3 +-
 xen/arch/arm/include/asm/setup.h        |   2 -
 xen/arch/arm/setup.c                    |  35 +----
 xen/common/device-tree/Makefile         |   1 +
 xen/common/device-tree/bootinfo.c       |  30 ++++
 xen/common/device-tree/dom0less-build.c |   4 +
 xen/common/device-tree/fdt-dump.c       | 180 ++++++++++++++++++++++++
 xen/include/xen/bootfdt.h               |  13 ++
 xen/include/xen/bootinfo.h              |   8 ++
 9 files changed, 240 insertions(+), 36 deletions(-)
 create mode 100644 xen/common/device-tree/fdt-dump.c


base-commit: f9b72572c8d5b6d312fd0b5014f65bed20efac40
-- 
2.55.0

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

[Oleksii Kurochko]

The device tree Xen builds 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, hence CONFIG_DEVICE_TREE_DEBUG. Mention
the dump in the help text of the option.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/Kconfig.debug                       |   3 +-
 xen/common/device-tree/Makefile         |   1 +
 xen/common/device-tree/dom0less-build.c |   4 +
 xen/common/device-tree/fdt-dump.c       | 180 ++++++++++++++++++++++++
 xen/include/xen/bootfdt.h               |  13 ++
 5 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/device-tree/fdt-dump.c

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index d900d926c555..664f50bf8a96 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -95,7 +95,8 @@ config DEVICE_TREE_DEBUG
 	depends on DEVICE_TREE_PARSE
 	help
 	  Device tree parsing and DOM0 device tree building messages are
-	  logged in the Xen ring buffer.
+	  logged in the Xen ring buffer. The device tree generated for each
+	  dom0less domU is dumped there as well, in a DTS-like format.
 	  If unsure, say N here.
 
 config SCRUB_DEBUG
diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
index 9036e455d66a..959dd2a9fd30 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_DEBUG) += 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..182201384faf 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,9 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret < 0 )
         goto err;
 
+    dt_dprintk("Device tree for %pd:\n", d);
+    device_tree_dump(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..fc3d9eca5891
--- /dev/null
+++ b/xen/common/device-tree/fdt-dump.c
@@ -0,0 +1,180 @@
+/* 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);
+}
+
+static bool __init is_printable_string(const void *data, unsigned int len)
+{
+    const char *s = data;
+    const char *se = s + len;
+
+    /* A zero length property, or one which isn't NUL terminated, isn't one. */
+    if ( (len == 0) || (s[len - 1] != '\0') )
+        return false;
+
+    while ( s < se )
+    {
+        const char *ss = s;
+
+        while ( (s < se) && *s && isprint(*s) )
+            s++;
+
+        /* Stopped on a non printable character, or the string is empty. */
+        if ( (*s != '\0') || (s == ss) )
+            return false;
+
+        s++;
+    }
+
+    return true;
+}
+
+/*
+ * Print the value of a property, using the same heuristics as dtc: a list of
+ * NUL terminated printable strings is printed as such, a value which is a
+ * multiple of 4 bytes as a list of cells, anything else as a byte string.
+ *
+ * The caller has already started the line (and with it selected the log
+ * level), so no XENLOG_* prefix may be used here.
+ */
+static void __init print_value(const void *data, unsigned int len)
+{
+    unsigned int i;
+
+    /* Empty property, nothing to print. */
+    if ( len == 0 )
+        return;
+
+    if ( is_printable_string(data, len) )
+    {
+        const char *s = data;
+        const char *end = data + len;
+
+        printk(" = ");
+        while ( s < end )
+        {
+            printk("\"%s\"", s);
+            s += strlen(s) + 1;
+            if ( s < end )
+                printk(", ");
+        }
+    }
+    else if ( IS_ALIGNED(len, sizeof(fdt32_t)) )
+    {
+        const fdt32_t *cell = data;
+
+        printk(" = <");
+        for ( i = 0; i < len / sizeof(*cell); i++ )
+            printk("%s0x%08x", i ? " " : "", fdt32_ld(&cell[i]));
+        printk(">");
+    }
+    else
+    {
+        const uint8_t *byte = data;
+
+        printk(" = [");
+        for ( i = 0; i < len; i++ )
+            printk("%s%02x", i ? " " : "", byte[i]);
+        printk("]");
+    }
+}
+
+static void __init print_properties(const void *fdt, int node, int indent)
+{
+    int prop;
+
+    fdt_for_each_property_offset ( prop, fdt, node )
+    {
+        const char *name;
+        const void *data;
+        int len;
+
+        data = fdt_getprop_by_offset(fdt, prop, &name, &len);
+        if ( !data )
+        {
+            printk(XENLOG_DEBUG "%*scouldn't read property at offset %d: %s\n",
+                   indent, "", prop, fdt_strerror(len));
+            return;
+        }
+
+        printk(XENLOG_DEBUG "%*s%s", indent, "", name);
+        print_value(data, len);
+        printk(";\n");
+    }
+
+    if ( (prop < 0) && (prop != -FDT_ERR_NOTFOUND) )
+        printk(XENLOG_DEBUG "%*scouldn't walk the properties: %s\n",
+               indent, "", fdt_strerror(prop));
+}
+
+void __init device_tree_dump(const void *fdt)
+{
+    int node, depth = 0;
+    /* Depth up to which nodes are open and still have to be closed. */
+    int open_depth = 0;
+
+    printk(XENLOG_DEBUG "/dts-v1/;\n");
+
+    /*
+     * fdt_next_node() signals the end of the subtree it was started on by
+     * making the depth negative, and only returns an error when the blob
+     * itself is malformed.
+     */
+    for ( node = 0; (node >= 0) && (depth >= 0);
+          node = fdt_next_node(fdt, node, &depth) )
+    {
+        const char *name = fdt_get_name(fdt, node, NULL);
+
+        /* Close the nodes which aren't a parent of the current one. */
+        while ( open_depth > depth )
+            printk(XENLOG_DEBUG "%*s};\n", indent_of(--open_depth), "");
+
+        printk(XENLOG_DEBUG "%*s%s {\n", indent_of(depth), "",
+               (depth == 0) ? "/" : (name ?: "<unknown>"));
+        print_properties(fdt, node, indent_of(depth) + 2);
+
+        open_depth = depth + 1;
+    }
+
+    while ( open_depth > 0 )
+        printk(XENLOG_DEBUG "%*s};\n", indent_of(--open_depth), "");
+
+    if ( node < 0 )
+        printk(XENLOG_DEBUG "The device tree dump is incomplete: %s\n",
+               fdt_strerror(node));
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 0e82ccea2f84..08983375209b 100644
--- a/xen/include/xen/bootfdt.h
+++ b/xen/include/xen/bootfdt.h
@@ -99,6 +99,19 @@ int device_tree_for_each_node(const void *fdt, int node,
                               device_tree_node_func func,
                               void *data);
 
+/*
+ * device_tree_dump - print a device tree blob in a DTS-like format
+ * @fdt: flat device tree.
+ *
+ * The tree is printed at debug log level and is only meant to be used for
+ * debugging, so it is a no-op unless CONFIG_DEVICE_TREE_DEBUG is enabled.
+ */
+#ifdef CONFIG_DEVICE_TREE_DEBUG
+void device_tree_dump(const void *fdt);
+#else
+static inline void device_tree_dump(const void *fdt) {}
+#endif
+
 typedef enum {
     BOOTMOD_XEN,
     BOOTMOD_FDT,
-- 
2.55.0

[PATCH 2/2] xen/device-tree: move discard_initial_modules() to common code

[Oleksii Kurochko]

Everything discard_initial_modules() touches is already common code:
bootinfo.modules, using_static_heap and fw_unreserved_regions(). Move it
out of Arm into common/device-tree/bootinfo.c, next to the boot allocator
handling of the same memory, and declare it in xen/bootinfo.h, so that
other device-tree based architectures can reuse it instead of growing a
copy of their own.

The arch-specific remove_early_mappings() is not moved along with it:
how the early FDT mapping is torn down is up to the architecture. As the
DTB is one of the boot modules being released, the mapping has to go once
they are discarded, so call it from start_xen() right after
discard_initial_modules(), which keeps the common function free of any
arch dependency.

While moving it, use unsigned int for the loop counter and replace the
"goto out" by a plain return, as there is nothing left to do at the label.

No functional change.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/arm/include/asm/setup.h  |  2 --
 xen/arch/arm/setup.c              | 35 ++-----------------------------
 xen/common/device-tree/bootinfo.c | 30 ++++++++++++++++++++++++++
 xen/include/xen/bootinfo.h        |  8 +++++++
 4 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index c491c567298f..02fdf2b56c9c 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -40,8 +40,6 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
 
 void create_dom0(void);
 
-void discard_initial_modules(void);
-
 void init_pdx(void);
 void setup_mm(void);
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 16f899dff57b..a65c3d2a655e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -210,39 +210,6 @@ static void __init processor_id(void)
     processor_setup();
 }
 
-void __init discard_initial_modules(void)
-{
-    struct boot_modules *mi = &bootinfo.modules;
-    int i;
-
-    /*
-     * When using static heap feature, don't give boot_modules memory back to
-     * the heap allocator
-     */
-    if ( using_static_heap )
-        goto out;
-
-    for ( i = 0; i < mi->nr_mods; i++ )
-    {
-        paddr_t s = mi->module[i].start;
-        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-        if ( mi->module[i].kind == BOOTMOD_XEN )
-            continue;
-
-        if ( !mfn_valid(maddr_to_mfn(s)) ||
-             !mfn_valid(maddr_to_mfn(e)) )
-            continue;
-
-        fw_unreserved_regions(s, e, init_domheap_pages, 0);
-    }
-
-    mi->nr_mods = 0;
-
- out:
-    remove_early_mappings();
-}
-
 /* Relocate the FDT in Xen heap */
 static void __init relocate_fdt(const void **dtb_vaddr, size_t dtb_size)
 {
@@ -509,6 +476,8 @@ void asmlinkage __init noreturn start_xen(unsigned long fdt_paddr)
      */
     discard_initial_modules();
 
+    remove_early_mappings();
+
     heap_init_late();
 
     init_constructors();
diff --git a/xen/common/device-tree/bootinfo.c b/xen/common/device-tree/bootinfo.c
index c637c45ad594..6772bf2df002 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -471,6 +471,36 @@ void __init populate_boot_allocator(void)
     }
 }
 
+void __init discard_initial_modules(void)
+{
+    struct boot_modules *mi = &bootinfo.modules;
+    unsigned int i;
+
+    /*
+     * When using static heap feature, don't give boot_modules memory back to
+     * the heap allocator
+     */
+    if ( using_static_heap )
+        return;
+
+    for ( i = 0; i < mi->nr_mods; i++ )
+    {
+        paddr_t s = mi->module[i].start;
+        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
+
+        if ( mi->module[i].kind == BOOTMOD_XEN )
+            continue;
+
+        if ( !mfn_valid(maddr_to_mfn(s)) ||
+             !mfn_valid(maddr_to_mfn(e)) )
+            continue;
+
+        fw_unreserved_regions(s, e, init_domheap_pages, 0);
+    }
+
+    mi->nr_mods = 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
index dbf492c2e36e..60ef25f3cf83 100644
--- a/xen/include/xen/bootinfo.h
+++ b/xen/include/xen/bootinfo.h
@@ -214,4 +214,12 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
                            void (*cb)(paddr_t ps, paddr_t pe),
                            unsigned int first);
 
+/*
+ * Return the memory occupied by the boot modules to the heap allocator.
+ *
+ * It needs to be called **before** heap_init_late() so that the released
+ * modules are scrubbed (unless suppressed).
+ */
+void discard_initial_modules(void);
+
 #endif /* XEN_BOOTINFO_H */
-- 
2.55.0

Re: [PATCH 2/2] xen/device-tree: move discard_initial_modules() to common code

[Orzel, Michal]

On 01-Oct-26 13:16, Oleksii Kurochko wrote:
> Everything discard_initial_modules() touches is already common code:
> bootinfo.modules, using_static_heap and fw_unreserved_regions(). Move it
> out of Arm into common/device-tree/bootinfo.c, next to the boot allocator
> handling of the same memory, and declare it in xen/bootinfo.h, so that
> other device-tree based architectures can reuse it instead of growing a
> copy of their own.
> 
> The arch-specific remove_early_mappings() is not moved along with it:
> how the early FDT mapping is torn down is up to the architecture. As the
> DTB is one of the boot modules being released, the mapping has to go once
> they are discarded, so call it from start_xen() right after
> discard_initial_modules(), which keeps the common function free of any
> arch dependency.
> 
> While moving it, use unsigned int for the loop counter and replace the
> "goto out" by a plain return, as there is nothing left to do at the label.
> 
> No functional change.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/arm/include/asm/setup.h  |  2 --
>  xen/arch/arm/setup.c              | 35 ++-----------------------------
>  xen/common/device-tree/bootinfo.c | 30 ++++++++++++++++++++++++++
>  xen/include/xen/bootinfo.h        |  8 +++++++
>  4 files changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index c491c567298f..02fdf2b56c9c 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -40,8 +40,6 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>  
>  void create_dom0(void);
>  
> -void discard_initial_modules(void);
> -
>  void init_pdx(void);
>  void setup_mm(void);
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 16f899dff57b..a65c3d2a655e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -210,39 +210,6 @@ static void __init processor_id(void)
>      processor_setup();
>  }
>  
> -void __init discard_initial_modules(void)
> -{
> -    struct boot_modules *mi = &bootinfo.modules;
> -    int i;
> -
> -    /*
> -     * When using static heap feature, don't give boot_modules memory back to
> -     * the heap allocator
> -     */
> -    if ( using_static_heap )
> -        goto out;
> -
> -    for ( i = 0; i < mi->nr_mods; i++ )
> -    {
> -        paddr_t s = mi->module[i].start;
> -        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
> -
> -        if ( mi->module[i].kind == BOOTMOD_XEN )
> -            continue;
> -
> -        if ( !mfn_valid(maddr_to_mfn(s)) ||
> -             !mfn_valid(maddr_to_mfn(e)) )
> -            continue;
> -
> -        fw_unreserved_regions(s, e, init_domheap_pages, 0);
> -    }
> -
> -    mi->nr_mods = 0;
> -
> - out:
> -    remove_early_mappings();
> -}
> -
>  /* Relocate the FDT in Xen heap */
>  static void __init relocate_fdt(const void **dtb_vaddr, size_t dtb_size)
>  {
> @@ -509,6 +476,8 @@ void asmlinkage __init noreturn start_xen(unsigned long fdt_paddr)
>       */
>      discard_initial_modules();
The comment above is now duplicated with the one you added in xen/bootinfo.h.
I'd drop it from here (can be done on commit).

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

NIT for the future: if you send a series of patches, please move the fixes/code
movement changes at the beginning of the series.

~Michal

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

[Orzel, Michal]

On 01-Oct-26 13:16, Oleksii Kurochko wrote:
> The device tree Xen builds 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, hence CONFIG_DEVICE_TREE_DEBUG. Mention
> the dump in the help text of the option.
We should also dump the DTB for dom0/hwdom in prepare_dtb_hwdom(). Please add
that to this patch.

> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/Kconfig.debug                       |   3 +-
>  xen/common/device-tree/Makefile         |   1 +
>  xen/common/device-tree/dom0less-build.c |   4 +
>  xen/common/device-tree/fdt-dump.c       | 180 ++++++++++++++++++++++++
>  xen/include/xen/bootfdt.h               |  13 ++
>  5 files changed, 200 insertions(+), 1 deletion(-)
>  create mode 100644 xen/common/device-tree/fdt-dump.c
> 
> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> index d900d926c555..664f50bf8a96 100644
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -95,7 +95,8 @@ config DEVICE_TREE_DEBUG
>  	depends on DEVICE_TREE_PARSE
>  	help
>  	  Device tree parsing and DOM0 device tree building messages are
> -	  logged in the Xen ring buffer.
> +	  logged in the Xen ring buffer. The device tree generated for each
> +	  dom0less domU is dumped there as well, in a DTS-like format.
I think we could have a separate option DEVICE_TREE_DUMP that depends on
DEVICE_TREE_PARSE. Sometimes all we need is just to see how the generated DTB
look like without seeing thousands of other DT debug messages. Especially that
nothing in fdt-dump.c depends on DEVICE_TREE_DEBUG.

>  	  If unsure, say N here.
>  
>  config SCRUB_DEBUG
> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> index 9036e455d66a..959dd2a9fd30 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_DEBUG) += 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..182201384faf 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,9 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>      if ( ret < 0 )
>          goto err;
>  
> +    dt_dprintk("Device tree for %pd:\n", d);
Move this message to device_tree_dump.

> +    device_tree_dump(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..fc3d9eca5891
> --- /dev/null
> +++ b/xen/common/device-tree/fdt-dump.c
> @@ -0,0 +1,180 @@
> +/* 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);
> +}
> +
> +static bool __init is_printable_string(const void *data, unsigned int len)
> +{
> +    const char *s = data;
> +    const char *se = s + len;
> +
> +    /* A zero length property, or one which isn't NUL terminated, isn't one. */
> +    if ( (len == 0) || (s[len - 1] != '\0') )
> +        return false;
> +
> +    while ( s < se )
> +    {
> +        const char *ss = s;
> +
> +        while ( (s < se) && *s && isprint(*s) )
Xen's is_print() behaves differently than what dtc uses. You need to combine it
with is_ascii. Otherwise 0xff would be marked as printable.

~Michal

Re: [PATCH 2/2] xen/device-tree: move discard_initial_modules() to common code

[Oleksii Kurochko]

On 10/8/26 1:19 PM, Orzel, Michal wrote:
> 
> 
> On 01-Oct-26 13:16, Oleksii Kurochko wrote:
>> Everything discard_initial_modules() touches is already common code:
>> bootinfo.modules, using_static_heap and fw_unreserved_regions(). Move it
>> out of Arm into common/device-tree/bootinfo.c, next to the boot allocator
>> handling of the same memory, and declare it in xen/bootinfo.h, so that
>> other device-tree based architectures can reuse it instead of growing a
>> copy of their own.
>>
>> The arch-specific remove_early_mappings() is not moved along with it:
>> how the early FDT mapping is torn down is up to the architecture. As the
>> DTB is one of the boot modules being released, the mapping has to go once
>> they are discarded, so call it from start_xen() right after
>> discard_initial_modules(), which keeps the common function free of any
>> arch dependency.
>>
>> While moving it, use unsigned int for the loop counter and replace the
>> "goto out" by a plain return, as there is nothing left to do at the label.
>>
>> No functional change.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>   xen/arch/arm/include/asm/setup.h  |  2 --
>>   xen/arch/arm/setup.c              | 35 ++-----------------------------
>>   xen/common/device-tree/bootinfo.c | 30 ++++++++++++++++++++++++++
>>   xen/include/xen/bootinfo.h        |  8 +++++++
>>   4 files changed, 40 insertions(+), 35 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>> index c491c567298f..02fdf2b56c9c 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -40,8 +40,6 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>>   
>>   void create_dom0(void);
>>   
>> -void discard_initial_modules(void);
>> -
>>   void init_pdx(void);
>>   void setup_mm(void);
>>   
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 16f899dff57b..a65c3d2a655e 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -210,39 +210,6 @@ static void __init processor_id(void)
>>       processor_setup();
>>   }
>>   
>> -void __init discard_initial_modules(void)
>> -{
>> -    struct boot_modules *mi = &bootinfo.modules;
>> -    int i;
>> -
>> -    /*
>> -     * When using static heap feature, don't give boot_modules memory back to
>> -     * the heap allocator
>> -     */
>> -    if ( using_static_heap )
>> -        goto out;
>> -
>> -    for ( i = 0; i < mi->nr_mods; i++ )
>> -    {
>> -        paddr_t s = mi->module[i].start;
>> -        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
>> -
>> -        if ( mi->module[i].kind == BOOTMOD_XEN )
>> -            continue;
>> -
>> -        if ( !mfn_valid(maddr_to_mfn(s)) ||
>> -             !mfn_valid(maddr_to_mfn(e)) )
>> -            continue;
>> -
>> -        fw_unreserved_regions(s, e, init_domheap_pages, 0);
>> -    }
>> -
>> -    mi->nr_mods = 0;
>> -
>> - out:
>> -    remove_early_mappings();
>> -}
>> -
>>   /* Relocate the FDT in Xen heap */
>>   static void __init relocate_fdt(const void **dtb_vaddr, size_t dtb_size)
>>   {
>> @@ -509,6 +476,8 @@ void asmlinkage __init noreturn start_xen(unsigned long fdt_paddr)
>>        */
>>       discard_initial_modules();
> The comment above is now duplicated with the one you added in xen/bootinfo.h.
> I'd drop it from here (can be done on commit).

I will drop it in v2 (if it won't merged at the moment when I will start 
address the comments for this patch series).

> 
> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks!


> 
> NIT for the future: if you send a series of patches, please move the fixes/code
> movement changes at the beginning of the series.
> 

I will take into account that in the future.

~ Oleksii

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

[Oleksii Kurochko]

On 10/8/26 1:57 PM, Orzel, Michal wrote:
> 
> 
> On 01-Oct-26 13:16, Oleksii Kurochko wrote:
>> The device tree Xen builds 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, hence CONFIG_DEVICE_TREE_DEBUG. Mention
>> the dump in the help text of the option.
> We should also dump the DTB for dom0/hwdom in prepare_dtb_hwdom(). Please add
> that to this patch.

It makes sense to me. I will add it.
> 
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>   xen/Kconfig.debug                       |   3 +-
>>   xen/common/device-tree/Makefile         |   1 +
>>   xen/common/device-tree/dom0less-build.c |   4 +
>>   xen/common/device-tree/fdt-dump.c       | 180 ++++++++++++++++++++++++
>>   xen/include/xen/bootfdt.h               |  13 ++
>>   5 files changed, 200 insertions(+), 1 deletion(-)
>>   create mode 100644 xen/common/device-tree/fdt-dump.c
>>
>> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
>> index d900d926c555..664f50bf8a96 100644
>> --- a/xen/Kconfig.debug
>> +++ b/xen/Kconfig.debug
>> @@ -95,7 +95,8 @@ config DEVICE_TREE_DEBUG
>>   	depends on DEVICE_TREE_PARSE
>>   	help
>>   	  Device tree parsing and DOM0 device tree building messages are
>> -	  logged in the Xen ring buffer.
>> +	  logged in the Xen ring buffer. The device tree generated for each
>> +	  dom0less domU is dumped there as well, in a DTS-like format.
> I think we could have a separate option DEVICE_TREE_DUMP that depends on
> DEVICE_TREE_PARSE. Sometimes all we need is just to see how the generated DTB
> look like without seeing thousands of other DT debug messages. Especially that
> nothing in fdt-dump.c depends on DEVICE_TREE_DEBUG.

Agree, it will be better to have DEVICE_TREE_DUMP. I will introduce the 
following config:

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 om0less
     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".
     If unsure, say N here.


> 
>>   	  If unsure, say N here.
>>   
>>   config SCRUB_DEBUG
>> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
>> index 9036e455d66a..959dd2a9fd30 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_DEBUG) += 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..182201384faf 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,9 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>>       if ( ret < 0 )
>>           goto err;
>>   
>> +    dt_dprintk("Device tree for %pd:\n", d);
> Move this message to device_tree_dump.
> 

I'll move.

>> +    device_tree_dump(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..fc3d9eca5891
>> --- /dev/null
>> +++ b/xen/common/device-tree/fdt-dump.c
>> @@ -0,0 +1,180 @@
>> +/* 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);
>> +}
>> +
>> +static bool __init is_printable_string(const void *data, unsigned int len)
>> +{
>> +    const char *s = data;
>> +    const char *se = s + len;
>> +
>> +    /* A zero length property, or one which isn't NUL terminated, isn't one. */
>> +    if ( (len == 0) || (s[len - 1] != '\0') )
>> +        return false;
>> +
>> +    while ( s < se )
>> +    {
>> +        const char *ss = s;
>> +
>> +        while ( (s < se) && *s && isprint(*s) )
> Xen's is_print() behaves differently than what dtc uses. You need to combine it
> with is_ascii. Otherwise 0xff would be marked as printable.

I'll add the is_ascii() before isprint():
      /*
       * Xen's isprint() also accepts the Latin-1 characters 0xa0-0xff,
       * while the one dtc uses only accepts ASCII ones.
       */
      while ( (s < se) && *s && isascii(*s) && isprint(*s) )
         ...

Thanks for review!

~ Oleksii