[PATCH 0/6] hw: Centralize handling, improve error messages for -machine dumpdtb

[PATCH 0/6] hw: Centralize handling, improve error messages for -machine dumpdtb

[Peter Maydell]

We originally implemented '-machine dumpdtb' in a fairly ad-hoc way:
every machine using FDT is supposed to call qemu_fdt_dumpdtb() once
it has finished creating and modifying the DTB; if the user passed in
the machine option then qemu_fdt_dumpdtb() will write the FDT to a
file and then exit QEMU.

Somewhat later we implemented the QMP and HMP dumpdtb commands; for
these to work we had to make all the FDT-using machines set
MachineState::fdt to point to the FDT blob.

This means we can clean up the handling of the -machine option, so we
can implement it in one place in machine.c.  The benefit of this is:
 * boards only need to do one thing, not two
 * we can have better error messages for the "user asked us to
   dump the DTB but this board doesn't have one" case

(In particular the bug report
https://gitlab.com/qemu-project/qemu/-/issues/2733
is essentially because we silently ignore the option if there
is no DTB to dump.)

The openrisc machines and the MIPS boston machine both were not
setting MachineState::fdt, so the HMP/QMP dumpdtb don't work for
those machines; the series starts by fixing those bugs.  Then we can
implement the centralized handling of the machine option.  Finally we
get to fix the "no error message" problem:

 $ qemu-system-aarch64 -M raspi4b,dumpdtb=/tmp/d.dtb
 qemu-system-aarch64: This machine doesn't have an FDT
 (Perhaps it doesn't support FDT at all, or perhaps you need to
 provide an FDT with the -fdt option?)

The fact that there are three places that report "this machine
doesn't have an FDT" is a bit of a wart, stemming largely from the
fact that the QMP dumpdtb command is only conditionally present if
CONFIG_FDT.  In theory we could make it unconditional, but I opted to
leave that can of worms for another day...

thanks
-- PMM

Peter Maydell (6):
  monitor/hmp-cmds.c: Clean up hmp_dumpdtb printf
  hw/openrisc: Support monitor dumpdtb command
  hw/mips/boston: Check for error return from boston_fdt_filter()
  hw/mips/boston: Support dumpdtb monitor commands
  hw: Centralize handling of -machine dumpdtb option
  hw/core/machine.c: Make -machine dumpdtb=file.dtb with no DTB an error

 include/hw/loader-fit.h      | 21 +++++++++++++++++---
 include/hw/openrisc/boot.h   |  3 ++-
 include/system/device_tree.h |  2 --
 hw/arm/boot.c                |  2 --
 hw/core/loader-fit.c         | 38 ++++++++++++++++++++----------------
 hw/core/machine.c            | 23 ++++++++++++++++++++++
 hw/loongarch/virt.c          |  1 -
 hw/mips/boston.c             | 16 ++++++++++-----
 hw/openrisc/boot.c           |  8 +++++---
 hw/openrisc/openrisc_sim.c   |  2 +-
 hw/openrisc/virt.c           |  2 +-
 hw/ppc/e500.c                |  1 -
 hw/ppc/pegasos2.c            |  1 -
 hw/ppc/pnv.c                 |  1 -
 hw/ppc/spapr.c               |  1 -
 hw/riscv/boot.c              |  2 --
 monitor/hmp-cmds.c           |  2 +-
 system/device_tree-stub.c    |  5 ++++-
 system/device_tree.c         | 22 ++++++---------------
 19 files changed, 93 insertions(+), 60 deletions(-)

-- 
2.34.1

[PATCH 1/6] monitor/hmp-cmds.c: Clean up hmp_dumpdtb printf

[Peter Maydell]

In hmp_dumpdtb(), we print a message when the command succeeds.  This
message is missing the trailing \n, so the HMP command prompt is
printed immediately after it.  We also weren't capitalizing 'DTB', or
quoting the filename in the message.  Fix these nits.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 monitor/hmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 0aa22e1ae27..ff87fd89e4d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -431,6 +431,6 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    monitor_printf(mon, "dtb dumped to %s", filename);
+    monitor_printf(mon, "DTB dumped to '%s'\n", filename);
 }
 #endif
-- 
2.34.1

[PATCH 2/6] hw/openrisc: Support monitor dumpdtb command

[Peter Maydell]

The openrisc machines don't set MachineState::fdt to point to their
DTB blob.  This means that although the command line '-machine
dumpdtb=file.dtb' option works, the equivalent QMP and HMP monitor
commands do not, but instead produce the error "This machine doesn't
have a FDT".

Set MachineState::fdt in openrisc_load_fdt(), when we write it to
guest memory.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/openrisc/boot.h | 3 ++-
 hw/openrisc/boot.c         | 7 +++++--
 hw/openrisc/openrisc_sim.c | 2 +-
 hw/openrisc/virt.c         | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/hw/openrisc/boot.h b/include/hw/openrisc/boot.h
index 25a313d63a1..9b4d88072c4 100644
--- a/include/hw/openrisc/boot.h
+++ b/include/hw/openrisc/boot.h
@@ -20,6 +20,7 @@
 #define OPENRISC_BOOT_H
 
 #include "exec/cpu-defs.h"
+#include "hw/boards.h"
 
 hwaddr openrisc_load_kernel(ram_addr_t ram_size,
                             const char *kernel_filename,
@@ -28,7 +29,7 @@ hwaddr openrisc_load_kernel(ram_addr_t ram_size,
 hwaddr openrisc_load_initrd(void *fdt, const char *filename,
                             hwaddr load_start, uint64_t mem_size);
 
-uint32_t openrisc_load_fdt(void *fdt, hwaddr load_start,
+uint32_t openrisc_load_fdt(MachineState *ms, void *fdt, hwaddr load_start,
                            uint64_t mem_size);
 
 #endif /* OPENRISC_BOOT_H */
diff --git a/hw/openrisc/boot.c b/hw/openrisc/boot.c
index 0f08df812dc..72e2756af05 100644
--- a/hw/openrisc/boot.c
+++ b/hw/openrisc/boot.c
@@ -90,8 +90,8 @@ hwaddr openrisc_load_initrd(void *fdt, const char *filename,
     return start + size;
 }
 
-uint32_t openrisc_load_fdt(void *fdt, hwaddr load_start,
-                           uint64_t mem_size)
+uint32_t openrisc_load_fdt(MachineState *ms, void *fdt,
+                           hwaddr load_start, uint64_t mem_size)
 {
     uint32_t fdt_addr;
     int ret;
@@ -111,6 +111,9 @@ uint32_t openrisc_load_fdt(void *fdt, hwaddr load_start,
     /* copy in the device tree */
     qemu_fdt_dumpdtb(fdt, fdtsize);
 
+    /* Save FDT for dumpdtb monitor command */
+    ms->fdt = fdt;
+
     rom_add_blob_fixed_as("fdt", fdt, fdtsize, fdt_addr,
                           &address_space_memory);
     qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index e0da4067ba3..d9e0744922a 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -354,7 +354,7 @@ static void openrisc_sim_init(MachineState *machine)
                                              machine->initrd_filename,
                                              load_addr, machine->ram_size);
         }
-        boot_info.fdt_addr = openrisc_load_fdt(state->fdt, load_addr,
+        boot_info.fdt_addr = openrisc_load_fdt(machine, state->fdt, load_addr,
                                                machine->ram_size);
     }
 }
diff --git a/hw/openrisc/virt.c b/hw/openrisc/virt.c
index 7b60bf85094..9afe407b00a 100644
--- a/hw/openrisc/virt.c
+++ b/hw/openrisc/virt.c
@@ -540,7 +540,7 @@ static void openrisc_virt_init(MachineState *machine)
                                              machine->initrd_filename,
                                              load_addr, machine->ram_size);
         }
-        boot_info.fdt_addr = openrisc_load_fdt(state->fdt, load_addr,
+        boot_info.fdt_addr = openrisc_load_fdt(machine, state->fdt, load_addr,
                                                machine->ram_size);
     }
 }
-- 
2.34.1

[PATCH 3/6] hw/mips/boston: Check for error return from boston_fdt_filter()

[Peter Maydell]

The function boston_fdt_filter() can return NULL on errors (in which
case it will print an error message).  When we call this from the
non-FIT-image codepath, we aren't checking the return value, so we
will plough on with a NULL pointer, and segfault in fdt_totalsize().
Check for errors here.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/mips/boston.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 364c328032a..f0e9a2461a0 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -810,6 +810,10 @@ static void boston_mach_init(MachineState *machine)
 
             dtb_load_data = boston_fdt_filter(s, dtb_file_data,
                                               NULL, &dtb_vaddr);
+            if (!dtb_load_data) {
+                /* boston_fdt_filter() already printed the error for us */
+                exit(1);
+            }
 
             /* Calculate real fdt size after filter */
             dt_size = fdt_totalsize(dtb_load_data);
-- 
2.34.1

[PATCH 4/6] hw/mips/boston: Support dumpdtb monitor commands

[Peter Maydell]

The boston machine doesn't set MachineState::fdt to the DTB blob that
it has loaded or created, which means that the QMP/HMP dumpdtb
monitor commands don't work.

Setting MachineState::fdt is easy in the non-FIT codepath: we can
simply do so immediately before loading the DTB into guest memory.
The FIT codepath is a bit more awkward as currently the FIT loader
throws away the memory that the FDT was in after it loads it into
guest memory.  So we add a void *pfdt argument to load_fit() for it
to store the FDT pointer into.

There is some readjustment required of the pointer handling in
loader-fit.c, so that it applies 'const' only where it should (e.g.
the data pointer we get back from fdt_getprop() is const, because
it's into the middle of the input FDT data, but the pointer that
fit_load_image_alloc() should not be const, because it's freshly
allocated memory that the caller can change if it likes).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/loader-fit.h | 21 ++++++++++++++++++---
 hw/core/loader-fit.c    | 38 +++++++++++++++++++++-----------------
 hw/mips/boston.c        | 11 +++++++----
 3 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/include/hw/loader-fit.h b/include/hw/loader-fit.h
index 0832e379dc9..9a43490ed63 100644
--- a/include/hw/loader-fit.h
+++ b/include/hw/loader-fit.h
@@ -30,12 +30,27 @@ struct fit_loader_match {
 struct fit_loader {
     const struct fit_loader_match *matches;
     hwaddr (*addr_to_phys)(void *opaque, uint64_t addr);
-    const void *(*fdt_filter)(void *opaque, const void *fdt,
-                              const void *match_data, hwaddr *load_addr);
+    void *(*fdt_filter)(void *opaque, const void *fdt,
+                        const void *match_data, hwaddr *load_addr);
     const void *(*kernel_filter)(void *opaque, const void *kernel,
                                  hwaddr *load_addr, hwaddr *entry_addr);
 };
 
-int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque);
+/**
+ * load_fit: load a FIT format image
+ * @ldr: structure defining board specific properties and hooks
+ * @filename: image to load
+ * @pfdt: pointer to update with address of FDT blob
+ * @opaque: opaque value passed back to the hook functions in @ldr
+ * Returns: 0 on success, or a negative errno on failure
+ *
+ * @pfdt is used to tell the caller about the FDT blob. On return, it
+ * has been set to point to the FDT blob, and it is now the caller's
+ * responsibility to free that memory with g_free(). Usually the caller
+ * will want to pass in &machine->fdt here, to record the FDT blob for
+ * the dumpdtb option and QMP/HMP commands.
+ */
+int load_fit(const struct fit_loader *ldr, const char *filename, void **pfdt,
+             void *opaque);
 
 #endif /* HW_LOADER_FIT_H */
diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 9bdd4fa17c6..6eb66406b07 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -32,8 +32,8 @@
 
 #define FIT_LOADER_MAX_PATH (128)
 
-static const void *fit_load_image_alloc(const void *itb, const char *name,
-                                        int *poff, size_t *psz, Error **errp)
+static void *fit_load_image_alloc(const void *itb, const char *name,
+                                  int *poff, size_t *psz, Error **errp)
 {
     const void *data;
     const char *comp;
@@ -80,11 +80,11 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
             return NULL;
         }
 
-        data = g_realloc(uncomp_data, uncomp_len);
+        uncomp_data = g_realloc(uncomp_data, uncomp_len);
         if (psz) {
             *psz = uncomp_len;
         }
-        return data;
+        return uncomp_data;
     }
 
     error_setg(errp, "unknown compression '%s'", comp);
@@ -177,13 +177,12 @@ out:
 
 static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
                         int cfg, void *opaque, const void *match_data,
-                        hwaddr kernel_end, Error **errp)
+                        hwaddr kernel_end, void **pfdt, Error **errp)
 {
     ERRP_GUARD();
     Error *err = NULL;
     const char *name;
-    const void *data;
-    const void *load_data;
+    void *data;
     hwaddr load_addr;
     int img_off;
     size_t sz;
@@ -194,7 +193,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
         return 0;
     }
 
-    load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
+    data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
     if (!data) {
         error_prepend(errp, "unable to load FDT image from FIT: ");
         return -EINVAL;
@@ -211,19 +210,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
     }
 
     if (ldr->fdt_filter) {
-        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
+        void *filtered_data;
+
+        filtered_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
+        if (filtered_data != data) {
+            g_free(data);
+            data = filtered_data;
+        }
     }
 
     load_addr = ldr->addr_to_phys(opaque, load_addr);
-    sz = fdt_totalsize(load_data);
-    rom_add_blob_fixed(name, load_data, sz, load_addr);
+    sz = fdt_totalsize(data);
+    rom_add_blob_fixed(name, data, sz, load_addr);
 
-    ret = 0;
+    *pfdt = data;
+    return 0;
 out:
     g_free((void *) data);
-    if (data != load_data) {
-        g_free((void *) load_data);
-    }
     return ret;
 }
 
@@ -259,7 +262,8 @@ out:
     return ret;
 }
 
-int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
+int load_fit(const struct fit_loader *ldr, const char *filename,
+             void **pfdt, void *opaque)
 {
     Error *err = NULL;
     const struct fit_loader_match *match;
@@ -323,7 +327,7 @@ int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
         goto out;
     }
 
-    ret = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end,
+    ret = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end, pfdt,
                        &err);
     if (ret) {
         error_report_err(err);
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index f0e9a2461a0..99e65f9fafb 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -358,8 +358,8 @@ static void gen_firmware(void *p, hwaddr kernel_entry, hwaddr fdt_addr)
                        kernel_entry);
 }
 
-static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
-                                     const void *match_data, hwaddr *load_addr)
+static void *boston_fdt_filter(void *opaque, const void *fdt_orig,
+                               const void *match_data, hwaddr *load_addr)
 {
     BostonState *s = BOSTON(opaque);
     MachineState *machine = s->mach;
@@ -797,7 +797,7 @@ static void boston_mach_init(MachineState *machine)
         if (kernel_size > 0) {
             int dt_size;
             g_autofree const void *dtb_file_data = NULL;
-            g_autofree const void *dtb_load_data = NULL;
+            void *dtb_load_data = NULL;
             hwaddr dtb_paddr = QEMU_ALIGN_UP(kernel_high, 64 * KiB);
             hwaddr dtb_vaddr = cpu_mips_phys_to_kseg0(NULL, dtb_paddr);
 
@@ -815,6 +815,8 @@ static void boston_mach_init(MachineState *machine)
                 exit(1);
             }
 
+            machine->fdt = dtb_load_data;
+
             /* Calculate real fdt size after filter */
             dt_size = fdt_totalsize(dtb_load_data);
             rom_add_blob_fixed("dtb", dtb_load_data, dt_size, dtb_paddr);
@@ -822,7 +824,8 @@ static void boston_mach_init(MachineState *machine)
                                 rom_ptr(dtb_paddr, dt_size));
         } else {
             /* Try to load file as FIT */
-            fit_err = load_fit(&boston_fit_loader, machine->kernel_filename, s);
+            fit_err = load_fit(&boston_fit_loader, machine->kernel_filename,
+                               &machine->fdt, s);
             if (fit_err) {
                 error_report("unable to load kernel image");
                 exit(1);
-- 
2.34.1

[PATCH 5/6] hw: Centralize handling of -machine dumpdtb option

[Peter Maydell]

Currently we handle the 'dumpdtb' machine sub-option ad-hoc in every
board model that has an FDT.  It's up to the board code to make sure
it calls qemu_fdt_dumpdtb() in the right place.

This means we're inconsistent and often just ignore the user's
command line argument:
 * if the board doesn't have an FDT at all
 * if the board supports FDT, but there happens not to be one
   present (usually because of a missing -fdt option)

This isn't very helpful because it gives the user no clue why their
option was ignored.

However, in order to support the QMP/HMP dumpdtb commands we require
now that every FDT machine stores a pointer to the FDT in
MachineState::fdt.  This means we can handle -machine dumpdtb
centrally by calling the qmp_dumpdtb() function, unifying its
handling with the QMP/HMP commands.  All the board code calls to
qemu_fdt_dumpdtb() can then be removed.

For this commit we retain the existing behaviour that if there
is no FDT we silently ignore the -machine dumpdtb option.
---
 include/system/device_tree.h |  2 --
 hw/arm/boot.c                |  2 --
 hw/core/machine.c            | 25 +++++++++++++++++++++++++
 hw/loongarch/virt.c          |  1 -
 hw/mips/boston.c             |  1 -
 hw/openrisc/boot.c           |  1 -
 hw/ppc/e500.c                |  1 -
 hw/ppc/pegasos2.c            |  1 -
 hw/ppc/pnv.c                 |  1 -
 hw/ppc/spapr.c               |  1 -
 hw/riscv/boot.c              |  2 --
 system/device_tree.c         | 15 ---------------
 12 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/include/system/device_tree.h b/include/system/device_tree.h
index eb601522f88..49d8482ed4e 100644
--- a/include/system/device_tree.h
+++ b/include/system/device_tree.h
@@ -133,8 +133,6 @@ int qemu_fdt_add_path(void *fdt, const char *path);
                          sizeof(qdt_tmp));                                    \
     } while (0)
 
-void qemu_fdt_dumpdtb(void *fdt, int size);
-
 /**
  * qemu_fdt_setprop_sized_cells_from_array:
  * @fdt: device tree blob
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index cbc24356fc1..533424cf2e1 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -661,8 +661,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         binfo->modify_dtb(binfo, fdt);
     }
 
-    qemu_fdt_dumpdtb(fdt, size);
-
     /* Put the DTB into the memory map as a ROM image: this will ensure
      * the DTB is copied again upon reset, even if addr points into RAM.
      */
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 254cc20c4cb..1b740071ac7 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -19,6 +19,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qemu/madvise.h"
 #include "qom/object_interfaces.h"
 #include "system/cpus.h"
@@ -1695,6 +1696,24 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify)
     notifier_remove(notify);
 }
 
+static void handle_machine_dumpdtb(MachineState *ms)
+{
+    if (!ms->dumpdtb) {
+        return;
+    }
+    if (!ms->fdt) {
+        /* Silently ignore dumpdtb option if there is nothing to dump */
+        return;
+    }
+#ifdef CONFIG_FDT
+    qmp_dumpdtb(ms->dumpdtb, &error_fatal);
+    exit(0);
+#else
+    error_report("This machine doesn't have an FDT");
+    exit(1);
+#endif
+}
+
 void qdev_machine_creation_done(void)
 {
     cpu_synchronize_all_post_init();
@@ -1711,6 +1730,12 @@ void qdev_machine_creation_done(void)
     phase_advance(PHASE_MACHINE_READY);
     qdev_assert_realized_properly();
 
+    /*
+     * If the user used -machine dumpdtb=file.dtb to request that we
+     * dump the DTB to a file,  do it now, and exit.
+     */
+    handle_machine_dumpdtb(current_machine);
+
     /* TODO: once all bus devices are qdevified, this should be done
      * when bus is created by qdev.c */
     /*
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 63fa0f4e32a..8ef965dea0e 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -674,7 +674,6 @@ static void virt_fdt_setup(LoongArchVirtMachineState *lvms)
      * Put the FDT into the memory map as a ROM image: this will ensure
      * the FDT is copied again upon reset, even if addr points into RAM.
      */
-    qemu_fdt_dumpdtb(machine->fdt, lvms->fdt_size);
     rom_add_blob_fixed_as("fdt", machine->fdt, lvms->fdt_size, FDT_BASE,
                           &address_space_memory);
     qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 99e65f9fafb..73cbc11b33d 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -395,7 +395,6 @@ static void *boston_fdt_filter(void *opaque, const void *fdt_orig,
                         1, ram_high_sz);
 
     fdt = g_realloc(fdt, fdt_totalsize(fdt));
-    qemu_fdt_dumpdtb(fdt, fdt_sz);
 
     s->fdt_base = *load_addr;
 
diff --git a/hw/openrisc/boot.c b/hw/openrisc/boot.c
index 72e2756af05..0a5881be314 100644
--- a/hw/openrisc/boot.c
+++ b/hw/openrisc/boot.c
@@ -109,7 +109,6 @@ uint32_t openrisc_load_fdt(MachineState *ms, void *fdt,
     /* Should only fail if we've built a corrupted tree */
     g_assert(ret == 0);
     /* copy in the device tree */
-    qemu_fdt_dumpdtb(fdt, fdtsize);
 
     /* Save FDT for dumpdtb monitor command */
     ms->fdt = fdt;
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 26933e0457e..fe8b9f79621 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -658,7 +658,6 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
 
 done:
     if (!dry_run) {
-        qemu_fdt_dumpdtb(fdt, fdt_size);
         cpu_physical_memory_write(addr, fdt, fdt_size);
 
         /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 0364243f4fe..eebb359abb0 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -417,7 +417,6 @@ static void pegasos2_machine_reset(MachineState *machine, ResetType type)
     d[1] = cpu_to_be64(pm->kernel_size - (pm->kernel_entry - pm->kernel_addr));
     qemu_fdt_setprop(fdt, "/chosen", "qemu,boot-kernel", d, sizeof(d));
 
-    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     g_free(pm->fdt_blob);
     pm->fdt_blob = fdt;
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 11fd477b71b..87607508c76 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -744,7 +744,6 @@ static void pnv_reset(MachineState *machine, ResetType type)
         _FDT((fdt_pack(fdt)));
     }
 
-    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
 
     /* Update machine->fdt with latest fdt */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f3a4b4235d4..c15340a58d8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1760,7 +1760,6 @@ static void spapr_machine_reset(MachineState *machine, ResetType type)
                                   0, fdt_addr, 0);
         cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
     }
-    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
 
     g_free(spapr->fdt_blob);
     spapr->fdt_size = fdt_totalsize(fdt);
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index c309441b7d8..765b9e2b1ab 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -374,8 +374,6 @@ void riscv_load_fdt(hwaddr fdt_addr, void *fdt)
     uint32_t fdtsize = fdt_totalsize(fdt);
 
     /* copy in the device tree */
-    qemu_fdt_dumpdtb(fdt, fdtsize);
-
     rom_add_blob_fixed_as("fdt", fdt, fdtsize, fdt_addr,
                           &address_space_memory);
     qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
diff --git a/system/device_tree.c b/system/device_tree.c
index 11f3178095c..56d4ac5650a 100644
--- a/system/device_tree.c
+++ b/system/device_tree.c
@@ -594,21 +594,6 @@ int qemu_fdt_add_path(void *fdt, const char *path)
     return retval;
 }
 
-void qemu_fdt_dumpdtb(void *fdt, int size)
-{
-    const char *dumpdtb = current_machine->dumpdtb;
-
-    if (dumpdtb) {
-        /* Dump the dtb to a file and quit */
-        if (g_file_set_contents(dumpdtb, fdt, size, NULL)) {
-            info_report("dtb dumped to %s. Exiting.", dumpdtb);
-            exit(0);
-        }
-        error_report("%s: Failed dumping dtb to %s", __func__, dumpdtb);
-        exit(1);
-    }
-}
-
 int qemu_fdt_setprop_sized_cells_from_array(void *fdt,
                                             const char *node_path,
                                             const char *property,
-- 
2.34.1

[PATCH 6/6] hw/core/machine.c: Make -machine dumpdtb=file.dtb with no DTB an error

[Peter Maydell]

Currently if the user requests via -machine dumpdtb=file.dtb that we
dump the DTB, but the machine doesn't have a DTB, we silently ignore
the option.  This is confusing to users, and is a legacy of the old
board-specific implementation of the option, where if the execution
codepath didn't go via a call to qemu_fdt_dumpdtb() we would never
handle the option.

Now we handle the option in one place in machine.c, we can provide
the user with a useful message if they asked us to dump a DTB when
none exists.  qmp_dumpdtb() already produces this error; remove the
logic in handle_machine_dumpdtb() that was there specifically to
avoid hitting it.

While we're here, beef up the error message a bit with a hint, and
make it consistent about "an FDT" rather than "a FDT".  (In the
qmp_dumpdtb() case this needs an ERRP_GUARD to make
error_append_hint() work when the caller passes error_fatal.)

Note that the three places where we might report "doesn't have an
FDT" are hit in different situations:

(1) in handle_machine_dumpdtb(), if CONFIG_FDT is not set: this is
because the QEMU binary was built without libfdt at all. The
build system will not let you build with a machine type that
needs an FDT but no libfdt, so here we know both that the machine
doesn't use FDT and that QEMU doesn't have the support:

(2) in the device_tree-stub.c qmp_dumpdtb(): this is used when
we had libfdt at build time but the target architecture didn't
enable any machines which did "select DEVICE_TREE", so here we
know that the machine doesn't use FDT.

(3) in qmp_dumpdtb(), if current_machine->fdt is NULL all we know
is that this machine never set it. That might be because it doesn't
use FDT, or it might be because the user didn't pass an FDT
on the command line and the machine doesn't autogenerate an FDT.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2733
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/machine.c         | 6 ++----
 system/device_tree-stub.c | 5 ++++-
 system/device_tree.c      | 7 ++++++-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1b740071ac7..f0e45fbad9d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1701,15 +1701,13 @@ static void handle_machine_dumpdtb(MachineState *ms)
     if (!ms->dumpdtb) {
         return;
     }
-    if (!ms->fdt) {
-        /* Silently ignore dumpdtb option if there is nothing to dump */
-        return;
-    }
 #ifdef CONFIG_FDT
     qmp_dumpdtb(ms->dumpdtb, &error_fatal);
     exit(0);
 #else
     error_report("This machine doesn't have an FDT");
+    error_printf("(this machine type definitely doesn't use FDT, and "
+                 "this QEMU doesn't have FDT support compiled in)\n");
     exit(1);
 #endif
 }
diff --git a/system/device_tree-stub.c b/system/device_tree-stub.c
index bddda6fa37a..428330b0fec 100644
--- a/system/device_tree-stub.c
+++ b/system/device_tree-stub.c
@@ -5,6 +5,9 @@
 #ifdef CONFIG_FDT
 void qmp_dumpdtb(const char *filename, Error **errp)
 {
-    error_setg(errp, "This machine doesn't have a FDT");
+    ERRP_GUARD();
+
+    error_setg(errp, "This machine doesn't have an FDT");
+    error_append_hint(errp, "(this machine type definitely doesn't use FDT)\n");
 }
 #endif
diff --git a/system/device_tree.c b/system/device_tree.c
index 56d4ac5650a..0d554240f93 100644
--- a/system/device_tree.c
+++ b/system/device_tree.c
@@ -635,11 +635,16 @@ out:
 
 void qmp_dumpdtb(const char *filename, Error **errp)
 {
+    ERRP_GUARD();
+
     g_autoptr(GError) err = NULL;
     uint32_t size;
 
     if (!current_machine->fdt) {
-        error_setg(errp, "This machine doesn't have a FDT");
+        error_setg(errp, "This machine doesn't have an FDT");
+        error_append_hint(errp,
+                          "(Perhaps it doesn't support FDT at all, or perhaps "
+                          "you need to provide an FDT with the -fdt option?)\n");
         return;
     }
 
-- 
2.34.1

Re: [PATCH 1/6] monitor/hmp-cmds.c: Clean up hmp_dumpdtb printf

[Richard Henderson]

On 2/6/25 07:12, Peter Maydell wrote:
> In hmp_dumpdtb(), we print a message when the command succeeds.  This
> message is missing the trailing \n, so the HMP command prompt is
> printed immediately after it.  We also weren't capitalizing 'DTB', or
> quoting the filename in the message.  Fix these nits.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   monitor/hmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 0aa22e1ae27..ff87fd89e4d 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -431,6 +431,6 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict)
>           return;
>       }
>   
> -    monitor_printf(mon, "dtb dumped to %s", filename);
> +    monitor_printf(mon, "DTB dumped to '%s'\n", filename);
>   }
>   #endif

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/6] hw/openrisc: Support monitor dumpdtb command

[Richard Henderson]

On 2/6/25 07:12, Peter Maydell wrote:
> The openrisc machines don't setMachineState::fdt to point to their
> DTB blob.  This means that although the command line '-machine
> dumpdtb=file.dtb' option works, the equivalent QMP and HMP monitor
> commands do not, but instead produce the error "This machine doesn't
> have a FDT".
> 
> SetMachineState::fdt in openrisc_load_fdt(), when we write it to
> guest memory.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/openrisc/boot.h | 3 ++-
>   hw/openrisc/boot.c         | 7 +++++--
>   hw/openrisc/openrisc_sim.c | 2 +-
>   hw/openrisc/virt.c         | 2 +-
>   4 files changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 5/6] hw: Centralize handling of -machine dumpdtb option

[Richard Henderson]

On 2/6/25 07:12, Peter Maydell wrote:
> Currently we handle the 'dumpdtb' machine sub-option ad-hoc in every
> board model that has an FDT.  It's up to the board code to make sure
> it calls qemu_fdt_dumpdtb() in the right place.
> 
> This means we're inconsistent and often just ignore the user's
> command line argument:
>   * if the board doesn't have an FDT at all
>   * if the board supports FDT, but there happens not to be one
>     present (usually because of a missing -fdt option)
> 
> This isn't very helpful because it gives the user no clue why their
> option was ignored.
> 
> However, in order to support the QMP/HMP dumpdtb commands we require
> now that every FDT machine stores a pointer to the FDT in
> MachineState::fdt.  This means we can handle -machine dumpdtb
> centrally by calling the qmp_dumpdtb() function, unifying its
> handling with the QMP/HMP commands.  All the board code calls to
> qemu_fdt_dumpdtb() can then be removed.
> 
> For this commit we retain the existing behaviour that if there
> is no FDT we silently ignore the -machine dumpdtb option.
> ---
>   include/system/device_tree.h |  2 --
>   hw/arm/boot.c                |  2 --
>   hw/core/machine.c            | 25 +++++++++++++++++++++++++
>   hw/loongarch/virt.c          |  1 -
>   hw/mips/boston.c             |  1 -
>   hw/openrisc/boot.c           |  1 -
>   hw/ppc/e500.c                |  1 -
>   hw/ppc/pegasos2.c            |  1 -
>   hw/ppc/pnv.c                 |  1 -
>   hw/ppc/spapr.c               |  1 -
>   hw/riscv/boot.c              |  2 --
>   system/device_tree.c         | 15 ---------------
>   12 files changed, 25 insertions(+), 28 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 6/6] hw/core/machine.c: Make -machine dumpdtb=file.dtb with no DTB an error

[Richard Henderson]

On 2/6/25 07:12, Peter Maydell wrote:
> Currently if the user requests via -machine dumpdtb=file.dtb that we
> dump the DTB, but the machine doesn't have a DTB, we silently ignore
> the option.  This is confusing to users, and is a legacy of the old
> board-specific implementation of the option, where if the execution
> codepath didn't go via a call to qemu_fdt_dumpdtb() we would never
> handle the option.
> 
> Now we handle the option in one place in machine.c, we can provide
> the user with a useful message if they asked us to dump a DTB when
> none exists.  qmp_dumpdtb() already produces this error; remove the
> logic in handle_machine_dumpdtb() that was there specifically to
> avoid hitting it.
> 
> While we're here, beef up the error message a bit with a hint, and
> make it consistent about "an FDT" rather than "a FDT".  (In the
> qmp_dumpdtb() case this needs an ERRP_GUARD to make
> error_append_hint() work when the caller passes error_fatal.)
> 
> Note that the three places where we might report "doesn't have an
> FDT" are hit in different situations:
> 
> (1) in handle_machine_dumpdtb(), if CONFIG_FDT is not set: this is
> because the QEMU binary was built without libfdt at all. The
> build system will not let you build with a machine type that
> needs an FDT but no libfdt, so here we know both that the machine
> doesn't use FDT and that QEMU doesn't have the support:
> 
> (2) in the device_tree-stub.c qmp_dumpdtb(): this is used when
> we had libfdt at build time but the target architecture didn't
> enable any machines which did "select DEVICE_TREE", so here we
> know that the machine doesn't use FDT.
> 
> (3) in qmp_dumpdtb(), if current_machine->fdt is NULL all we know
> is that this machine never set it. That might be because it doesn't
> use FDT, or it might be because the user didn't pass an FDT
> on the command line and the machine doesn't autogenerate an FDT.
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2733
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/core/machine.c         | 6 ++----
>   system/device_tree-stub.c | 5 ++++-
>   system/device_tree.c      | 7 ++++++-
>   3 files changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/6] hw/mips/boston: Check for error return from boston_fdt_filter()

[Philippe Mathieu-Daudé]

On 6/2/25 16:12, Peter Maydell wrote:
> The function boston_fdt_filter() can return NULL on errors (in which
> case it will print an error message).  When we call this from the
> non-FIT-image codepath, we aren't checking the return value, so we
> will plough on with a NULL pointer, and segfault in fdt_totalsize().
> Check for errors here.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/mips/boston.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/6] hw/mips/boston: Support dumpdtb monitor commands

[Philippe Mathieu-Daudé]

Hi Peter,

On 6/2/25 16:12, Peter Maydell wrote:
> The boston machine doesn't set MachineState::fdt to the DTB blob that
> it has loaded or created, which means that the QMP/HMP dumpdtb
> monitor commands don't work.
> 
> Setting MachineState::fdt is easy in the non-FIT codepath: we can
> simply do so immediately before loading the DTB into guest memory.
> The FIT codepath is a bit more awkward as currently the FIT loader
> throws away the memory that the FDT was in after it loads it into
> guest memory.  So we add a void *pfdt argument to load_fit() for it
> to store the FDT pointer into.
> 
> There is some readjustment required of the pointer handling in
> loader-fit.c, so that it applies 'const' only where it should (e.g.
> the data pointer we get back from fdt_getprop() is const, because
> it's into the middle of the input FDT data, but the pointer that
> fit_load_image_alloc() should not be const, because it's freshly
> allocated memory that the caller can change if it likes).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/loader-fit.h | 21 ++++++++++++++++++---
>   hw/core/loader-fit.c    | 38 +++++++++++++++++++++-----------------
>   hw/mips/boston.c        | 11 +++++++----
>   3 files changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/include/hw/loader-fit.h b/include/hw/loader-fit.h
> index 0832e379dc9..9a43490ed63 100644
> --- a/include/hw/loader-fit.h
> +++ b/include/hw/loader-fit.h
> @@ -30,12 +30,27 @@ struct fit_loader_match {
>   struct fit_loader {
>       const struct fit_loader_match *matches;
>       hwaddr (*addr_to_phys)(void *opaque, uint64_t addr);
> -    const void *(*fdt_filter)(void *opaque, const void *fdt,
> -                              const void *match_data, hwaddr *load_addr);
> +    void *(*fdt_filter)(void *opaque, const void *fdt,
> +                        const void *match_data, hwaddr *load_addr);
>       const void *(*kernel_filter)(void *opaque, const void *kernel,
>                                    hwaddr *load_addr, hwaddr *entry_addr);
>   };
>   
> -int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque);
> +/**
> + * load_fit: load a FIT format image
> + * @ldr: structure defining board specific properties and hooks
> + * @filename: image to load
> + * @pfdt: pointer to update with address of FDT blob

It is not clear this field is optional or mandatory.

Looking at other docstrings, optional is not always precised,
and code often consider optional if not precised. Mandatory is
mentioned explicitly.

> + * @opaque: opaque value passed back to the hook functions in @ldr
> + * Returns: 0 on success, or a negative errno on failure
> + *
> + * @pfdt is used to tell the caller about the FDT blob. On return, it
> + * has been set to point to the FDT blob, and it is now the caller's
> + * responsibility to free that memory with g_free(). Usually the caller
> + * will want to pass in &machine->fdt here, to record the FDT blob for
> + * the dumpdtb option and QMP/HMP commands.
> + */
> +int load_fit(const struct fit_loader *ldr, const char *filename, void **pfdt,
> +             void *opaque);


>   static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>                           int cfg, void *opaque, const void *match_data,
> -                        hwaddr kernel_end, Error **errp)
> +                        hwaddr kernel_end, void **pfdt, Error **errp)
>   {
>       ERRP_GUARD();
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> -    const void *load_data;
> +    void *data;
>       hwaddr load_addr;
>       int img_off;
>       size_t sz;
> @@ -194,7 +193,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>           return 0;
>       }
>   
> -    load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
> +    data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
>       if (!data) {
>           error_prepend(errp, "unable to load FDT image from FIT: ");
>           return -EINVAL;
> @@ -211,19 +210,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>       }
>   
>       if (ldr->fdt_filter) {
> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        void *filtered_data;
> +
> +        filtered_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        if (filtered_data != data) {
> +            g_free(data);
> +            data = filtered_data;
> +        }
>       }
>   
>       load_addr = ldr->addr_to_phys(opaque, load_addr);
> -    sz = fdt_totalsize(load_data);
> -    rom_add_blob_fixed(name, load_data, sz, load_addr);
> +    sz = fdt_totalsize(data);
> +    rom_add_blob_fixed(name, data, sz, load_addr);
>   
> -    ret = 0;
> +    *pfdt = data;

Here pfdt is assumed to be non-NULL, so a mandatory field.

Could you update the documentation? Otherwise consider adding
a non-NULL check.

Either ways:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    return 0;
>   out:
>       g_free((void *) data);
> -    if (data != load_data) {
> -        g_free((void *) load_data);
> -    }
>       return ret;
>   }
>   
> @@ -259,7 +262,8 @@ out:
>       return ret;
>   }

Re: [PATCH 1/6] monitor/hmp-cmds.c: Clean up hmp_dumpdtb printf

[Philippe Mathieu-Daudé]

On 6/2/25 16:12, Peter Maydell wrote:
> In hmp_dumpdtb(), we print a message when the command succeeds.  This
> message is missing the trailing \n, so the HMP command prompt is
> printed immediately after it.  We also weren't capitalizing 'DTB', or
> quoting the filename in the message.  Fix these nits.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   monitor/hmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/6] hw/openrisc: Support monitor dumpdtb command

[Philippe Mathieu-Daudé]

On 6/2/25 16:12, Peter Maydell wrote:
> The openrisc machines don't set MachineState::fdt to point to their
> DTB blob.  This means that although the command line '-machine
> dumpdtb=file.dtb' option works, the equivalent QMP and HMP monitor
> commands do not, but instead produce the error "This machine doesn't
> have a FDT".
> 
> Set MachineState::fdt in openrisc_load_fdt(), when we write it to
> guest memory.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/openrisc/boot.h | 3 ++-
>   hw/openrisc/boot.c         | 7 +++++--
>   hw/openrisc/openrisc_sim.c | 2 +-
>   hw/openrisc/virt.c         | 2 +-
>   4 files changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/6] hw/mips/boston: Support dumpdtb monitor commands

[Peter Maydell]

On Mon, 10 Feb 2025 at 10:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 6/2/25 16:12, Peter Maydell wrote:
> > -int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque);
> > +/**
> > + * load_fit: load a FIT format image
> > + * @ldr: structure defining board specific properties and hooks
> > + * @filename: image to load
> > + * @pfdt: pointer to update with address of FDT blob
>
> It is not clear this field is optional or mandatory.
>
> Looking at other docstrings, optional is not always precised,
> and code often consider optional if not precised. Mandatory is
> mentioned explicitly.

I did go back and forth while I was writing this on whether to
make it optional or not (and the versions where it is optional,
I had a note in the documentation about that). But there is exactly
one caller of this function, and that callsite wants to pass a
non-NULL pointer, so I ended up deciding that it was pointless
to make the argument optional and include the code to handle
"pfdt is NULL".

If you get it wrong you'll find out very quickly because your
code will segfault :-)

Generally we (should) say arguments are optional when they're
optional; in those cases we also should document what the behaviour
when they're omitted is. So I think "mandatory" is the default.
In this function, ldr and filename also are mandatory. If
we mark every mandatory argument as "mandatory" then we will
end up with a lot of boilerplate markup for most arguments,
I think.

> Could you update the documentation? Otherwise consider adding
> a non-NULL check.
>
> Either ways:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

thanks
-- PMM

Re: [PATCH 4/6] hw/mips/boston: Support dumpdtb monitor commands

[Philippe Mathieu-Daudé]

On 10/2/25 12:09, Peter Maydell wrote:
> On Mon, 10 Feb 2025 at 10:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 6/2/25 16:12, Peter Maydell wrote:
>>> -int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque);
>>> +/**
>>> + * load_fit: load a FIT format image
>>> + * @ldr: structure defining board specific properties and hooks
>>> + * @filename: image to load
>>> + * @pfdt: pointer to update with address of FDT blob
>>
>> It is not clear this field is optional or mandatory.
>>
>> Looking at other docstrings, optional is not always precised,
>> and code often consider optional if not precised. Mandatory is
>> mentioned explicitly.
> 
> I did go back and forth while I was writing this on whether to
> make it optional or not (and the versions where it is optional,
> I had a note in the documentation about that). But there is exactly
> one caller of this function, and that callsite wants to pass a
> non-NULL pointer, so I ended up deciding that it was pointless
> to make the argument optional and include the code to handle
> "pfdt is NULL".
> 
> If you get it wrong you'll find out very quickly because your
> code will segfault :-)
> 
> Generally we (should) say arguments are optional when they're
> optional; in those cases we also should document what the behaviour
> when they're omitted is. So I think "mandatory" is the default.
> In this function, ldr and filename also are mandatory. If
> we mark every mandatory argument as "mandatory" then we will
> end up with a lot of boilerplate markup for most arguments,
> I think.

OK, fine as is then!

> 
>> Could you update the documentation? Otherwise consider adding
>> a non-NULL check.
>>
>> Either ways:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> thanks
> -- PMM

Re: [PATCH 0/6] hw: Centralize handling, improve error messages for -machine dumpdtb

[Peter Maydell]

On Thu, 6 Feb 2025 at 15:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> We originally implemented '-machine dumpdtb' in a fairly ad-hoc way:
> every machine using FDT is supposed to call qemu_fdt_dumpdtb() once
> it has finished creating and modifying the DTB; if the user passed in
> the machine option then qemu_fdt_dumpdtb() will write the FDT to a
> file and then exit QEMU.
>
> Somewhat later we implemented the QMP and HMP dumpdtb commands; for
> these to work we had to make all the FDT-using machines set
> MachineState::fdt to point to the FDT blob.
>
> This means we can clean up the handling of the -machine option, so we
> can implement it in one place in machine.c.  The benefit of this is:
>  * boards only need to do one thing, not two
>  * we can have better error messages for the "user asked us to
>    dump the DTB but this board doesn't have one" case

> Peter Maydell (6):
>   monitor/hmp-cmds.c: Clean up hmp_dumpdtb printf
>   hw/openrisc: Support monitor dumpdtb command
>   hw/mips/boston: Check for error return from boston_fdt_filter()
>   hw/mips/boston: Support dumpdtb monitor commands
>   hw: Centralize handling of -machine dumpdtb option
>   hw/core/machine.c: Make -machine dumpdtb=file.dtb with no DTB an error

Since these have all been reviewed, I'm going to take them
via target-arm.next, unless anybody wants to propose taking
them via a different route.

thanks
-- PMM