[PATCH 0/4] arm_load_dtb cleanups

[PATCH 0/4] arm_load_dtb cleanups

[Alex Bennée]

This was prompted by a slop PR that came in via github and while
looking at it I thought what arm_load_dtb really needed was to be
modernised to:

  - use autofree to avoid goto fail
  - use error_setg to report errors
  - use error_fatal rather than open coding exit()

Alex.

Alex Bennée (4):
  hw/arm: use g_autofree for filename in arm_load_dtb
  hw/arm: use g_autofree for fdt in arm_load_dtb
  hw/arm: use g_auto(GStrv) for node_path in arm_load_dtb
  hw/arm: expose Error * to arm_load_dtb

 include/hw/arm/boot.h |  3 +-
 hw/arm/boot.c         | 75 ++++++++++++++++++-------------------------
 hw/arm/virt.c         |  6 ++--
 3 files changed, 36 insertions(+), 48 deletions(-)

-- 
2.47.2

[PATCH 1/4] hw/arm: use g_autofree for filename in arm_load_dtb

[Alex Bennée]

The function has quite a number of exit cases so lets try and clean
things up with g_autofree. As the fdt hasn't be allocated yet we can
return directly from the fail point.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/arm/boot.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d391cd01bb1..56fd13b9f7c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -528,20 +528,18 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     Error *err = NULL;
 
     if (binfo->dtb_filename) {
-        char *filename;
-        filename = qemu_find_file(QEMU_FILE_TYPE_DTB, binfo->dtb_filename);
+        g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_DTB,
+                                                   binfo->dtb_filename);
         if (!filename) {
             fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
-            goto fail;
+            return -1;
         }
 
         fdt = load_device_tree(filename, &size);
         if (!fdt) {
             fprintf(stderr, "Couldn't open dtb file %s\n", filename);
-            g_free(filename);
             goto fail;
         }
-        g_free(filename);
     } else {
         fdt = binfo->get_dtb(binfo, &size);
         if (!fdt) {
-- 
2.47.2

[PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Alex Bennée]

With the fdt being protected by g_autofree we can skip the goto fail
and bail out straight away. The only thing we must take care of is
stealing the pointer in the one case when we do need it to survive.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/arm/boot.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 56fd13b9f7c..749f2d08341 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as, MachineState *ms,
                  ARMCPU *cpu)
 {
-    void *fdt = NULL;
+    g_autofree void *fdt = NULL;
     int size, rc, n = 0;
     uint32_t acells, scells;
     unsigned int i;
@@ -538,13 +538,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         fdt = load_device_tree(filename, &size);
         if (!fdt) {
             fprintf(stderr, "Couldn't open dtb file %s\n", filename);
-            goto fail;
+            return -1;
         }
     } else {
         fdt = binfo->get_dtb(binfo, &size);
         if (!fdt) {
             fprintf(stderr, "Board was unable to create a dtb blob\n");
-            goto fail;
+            return -1;
         }
     }
 
@@ -553,7 +553,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
          * Whether this constitutes failure is up to the caller to decide,
          * so just return 0 as size, i.e., no error.
          */
-        g_free(fdt);
         return 0;
     }
 
@@ -563,7 +562,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                                    NULL, &error_fatal);
     if (acells == 0 || scells == 0) {
         fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
-        goto fail;
+        return -1;
     }
 
     if (scells < 2 && binfo->ram_size >= 4 * GiB) {
@@ -572,14 +571,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
          */
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM size > 4GB\n");
-        goto fail;
+        return -1;
     }
 
     /* nop all root nodes matching /memory or /memory@unit-address */
     node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
     if (err) {
         error_report_err(err);
-        goto fail;
+        return -1;
     }
     while (node_path[n]) {
         if (g_str_has_prefix(node_path[n], "/memory")) {
@@ -611,7 +610,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
             if (rc < 0) {
                 fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
                         mem_base);
-                goto fail;
+                return -1;
             }
 
             mem_base += mem_len;
@@ -622,7 +621,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         if (rc < 0) {
             fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
                     binfo->loader_start);
-            goto fail;
+            return -1;
         }
     }
 
@@ -636,7 +635,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                                      ms->kernel_cmdline);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/bootargs\n");
-            goto fail;
+            return -1;
         }
     }
 
@@ -645,7 +644,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                                           acells, binfo->initrd_start);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
-            goto fail;
+            return -1;
         }
 
         rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
@@ -654,7 +653,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                                           binfo->initrd_size);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
-            goto fail;
+            return -1;
         }
     }
 
@@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
 
     if (fdt != ms->fdt) {
         g_free(ms->fdt);
-        ms->fdt = fdt;
+        ms->fdt = g_steal_pointer(&fdt);
     }
 
     return size;
-
-fail:
-    g_free(fdt);
-    return -1;
 }
 
 static void do_cpu_reset(void *opaque)
-- 
2.47.2

[PATCH 3/4] hw/arm: use g_auto(GStrv) for node_path in arm_load_dtb

[Alex Bennée]

This is potentially more of a bike-shed case as node_path will persist
until the end of the function.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/arm/boot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 749f2d08341..f9d0bc7011e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -520,11 +520,11 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  ARMCPU *cpu)
 {
     g_autofree void *fdt = NULL;
+    g_auto(GStrv) node_path = NULL;
     int size, rc, n = 0;
     uint32_t acells, scells;
     unsigned int i;
     hwaddr mem_base, mem_len;
-    char **node_path;
     Error *err = NULL;
 
     if (binfo->dtb_filename) {
@@ -586,7 +586,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
         n++;
     }
-    g_strfreev(node_path);
 
     /*
      * We drop all the memory nodes which correspond to empty NUMA nodes
-- 
2.47.2

[PATCH 4/4] hw/arm: expose Error * to arm_load_dtb

[Alex Bennée]

Currently all calls to arm_load_dtb will result in an exit if we fail.
By passing Error * we can use &error_fatal and properly set the error
report.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/arm/boot.h |  3 ++-
 hw/arm/boot.c         | 35 +++++++++++++++--------------------
 hw/arm/virt.c         |  6 +++---
 3 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index a2e22bda8a5..fdb99c0c1ee 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -164,6 +164,7 @@ AddressSpace *arm_boot_address_space(ARMCPU *cpu,
  * @addr_limit: upper limit of the available memory area at @addr
  * @as:         address space to load image to
  * @cpu:        ARM CPU object
+ * @errp:       Error object, often &error_fatal
  *
  * Load a device tree supplied by the machine or by the user  with the
  * '-dtb' command line option, and put it at offset @addr in target
@@ -181,7 +182,7 @@ AddressSpace *arm_boot_address_space(ARMCPU *cpu,
  */
 int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as, MachineState *ms,
-                 ARMCPU *cpu);
+                 ARMCPU *cpu, Error **errp);
 
 /* Write a secure board setup routine with a dummy handler for SMCs */
 void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index f9d0bc7011e..d28ae8b86ab 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -517,7 +517,7 @@ static void fdt_add_psci_node(void *fdt, ARMCPU *armcpu)
 
 int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as, MachineState *ms,
-                 ARMCPU *cpu)
+                 ARMCPU *cpu, Error **errp)
 {
     g_autofree void *fdt = NULL;
     g_auto(GStrv) node_path = NULL;
@@ -525,25 +525,24 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     uint32_t acells, scells;
     unsigned int i;
     hwaddr mem_base, mem_len;
-    Error *err = NULL;
 
     if (binfo->dtb_filename) {
         g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_DTB,
                                                    binfo->dtb_filename);
         if (!filename) {
-            fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
+            error_setg(errp, "Couldn't open dtb file %s", binfo->dtb_filename);
             return -1;
         }
 
         fdt = load_device_tree(filename, &size);
         if (!fdt) {
-            fprintf(stderr, "Couldn't open dtb file %s\n", filename);
+            error_setg(errp, "Couldn't open dtb file %s", filename);
             return -1;
         }
     } else {
         fdt = binfo->get_dtb(binfo, &size);
         if (!fdt) {
-            fprintf(stderr, "Board was unable to create a dtb blob\n");
+            error_setg(errp, "Board was unable to create a dtb blob");
             return -1;
         }
     }
@@ -561,7 +560,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
                                    NULL, &error_fatal);
     if (acells == 0 || scells == 0) {
-        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
+        error_setg(errp, "dtb file invalid (#address-cells or #size-cells 0)");
         return -1;
     }
 
@@ -569,15 +568,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         /* This is user error so deserves a friendlier error message
          * than the failure of setprop_sized_cells would provide
          */
-        fprintf(stderr, "qemu: dtb file not compatible with "
-                "RAM size > 4GB\n");
+        error_setg(errp, "qemu: dtb file not compatible with RAM size > 4GB");
         return -1;
     }
 
     /* nop all root nodes matching /memory or /memory@unit-address */
-    node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
-    if (err) {
-        error_report_err(err);
+    node_path = qemu_fdt_node_unit_path(fdt, "memory", errp);
+    if (!node_path) {
         return -1;
     }
     while (node_path[n]) {
@@ -607,7 +604,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
             rc = fdt_add_memory_node(fdt, acells, mem_base,
                                      scells, mem_len, i);
             if (rc < 0) {
-                fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
+                error_setg(errp, "couldn't add /memory@%"PRIx64" node",
                         mem_base);
                 return -1;
             }
@@ -618,7 +615,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         rc = fdt_add_memory_node(fdt, acells, binfo->loader_start,
                                  scells, binfo->ram_size, -1);
         if (rc < 0) {
-            fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
+            error_setg(errp, "couldn't add /memory@%"PRIx64" node",
                     binfo->loader_start);
             return -1;
         }
@@ -633,7 +630,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
                                      ms->kernel_cmdline);
         if (rc < 0) {
-            fprintf(stderr, "couldn't set /chosen/bootargs\n");
+            error_setg(errp, "couldn't set /chosen/bootargs");
             return -1;
         }
     }
@@ -642,7 +639,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
                                           acells, binfo->initrd_start);
         if (rc < 0) {
-            fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
+            error_setg(errp, "couldn't set /chosen/linux,initrd-start");
             return -1;
         }
 
@@ -651,7 +648,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                                           binfo->initrd_start +
                                           binfo->initrd_size);
         if (rc < 0) {
-            fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
+            error_setg(errp, "couldn't set /chosen/linux,initrd-end");
             return -1;
         }
     }
@@ -1321,10 +1318,8 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
      * decided whether to enable PSCI and set the psci-conduit CPU properties.
      */
     if (!info->skip_dtb_autoload && have_dtb(info)) {
-        if (arm_load_dtb(info->dtb_start, info, info->dtb_limit,
-                         as, ms, cpu) < 0) {
-            exit(1);
-        }
+        arm_load_dtb(info->dtb_start, info, info->dtb_limit,
+                     as, ms, cpu, &error_fatal);
     }
 }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9326cfc895f..6061e0ddb50 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1803,9 +1803,9 @@ void virt_machine_done(Notifier *notifier, void *data)
                                        vms->memmap[VIRT_PLATFORM_BUS].size,
                                        vms->irqmap[VIRT_PLATFORM_BUS]);
     }
-    if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as, ms, cpu) < 0) {
-        exit(1);
-    }
+
+    arm_load_dtb(info->dtb_start, info, info->dtb_limit,
+                 as, ms, cpu, &error_fatal);
 
     pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
                                        &error_abort);
-- 
2.47.2

Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Manos Pitsidianakis]

On Mon, Sep 1, 2025 at 3:53 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> With the fdt being protected by g_autofree we can skip the goto fail
> and bail out straight away. The only thing we must take care of is
> stealing the pointer in the one case when we do need it to survive.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


>  hw/arm/boot.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 56fd13b9f7c..749f2d08341 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
>                   ARMCPU *cpu)
>  {
> -    void *fdt = NULL;
> +    g_autofree void *fdt = NULL;
>      int size, rc, n = 0;
>      uint32_t acells, scells;
>      unsigned int i;
> @@ -538,13 +538,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          fdt = load_device_tree(filename, &size);
>          if (!fdt) {
>              fprintf(stderr, "Couldn't open dtb file %s\n", filename);
> -            goto fail;
> +            return -1;
>          }
>      } else {
>          fdt = binfo->get_dtb(binfo, &size);
>          if (!fdt) {
>              fprintf(stderr, "Board was unable to create a dtb blob\n");
> -            goto fail;
> +            return -1;
>          }
>      }
>
> @@ -553,7 +553,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>           * Whether this constitutes failure is up to the caller to decide,
>           * so just return 0 as size, i.e., no error.
>           */
> -        g_free(fdt);
>          return 0;
>      }
>
> @@ -563,7 +562,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                                     NULL, &error_fatal);
>      if (acells == 0 || scells == 0) {
>          fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> -        goto fail;
> +        return -1;
>      }
>
>      if (scells < 2 && binfo->ram_size >= 4 * GiB) {
> @@ -572,14 +571,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>           */
>          fprintf(stderr, "qemu: dtb file not compatible with "
>                  "RAM size > 4GB\n");
> -        goto fail;
> +        return -1;
>      }
>
>      /* nop all root nodes matching /memory or /memory@unit-address */
>      node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
>      if (err) {
>          error_report_err(err);
> -        goto fail;
> +        return -1;
>      }
>      while (node_path[n]) {
>          if (g_str_has_prefix(node_path[n], "/memory")) {
> @@ -611,7 +610,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>              if (rc < 0) {
>                  fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
>                          mem_base);
> -                goto fail;
> +                return -1;
>              }
>
>              mem_base += mem_len;
> @@ -622,7 +621,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          if (rc < 0) {
>              fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
>                      binfo->loader_start);
> -            goto fail;
> +            return -1;
>          }
>      }
>
> @@ -636,7 +635,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                                       ms->kernel_cmdline);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/bootargs\n");
> -            goto fail;
> +            return -1;
>          }
>      }
>
> @@ -645,7 +644,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                                            acells, binfo->initrd_start);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> -            goto fail;
> +            return -1;
>          }
>
>          rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> @@ -654,7 +653,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                                            binfo->initrd_size);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> -            goto fail;
> +            return -1;
>          }
>      }
>
> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>
>      if (fdt != ms->fdt) {
>          g_free(ms->fdt);
> -        ms->fdt = fdt;
> +        ms->fdt = g_steal_pointer(&fdt);
>      }
>
>      return size;
> -
> -fail:
> -    g_free(fdt);
> -    return -1;
>  }
>
>  static void do_cpu_reset(void *opaque)
> --
> 2.47.2
>
>

Re: [PATCH 4/4] hw/arm: expose Error * to arm_load_dtb

[Manos Pitsidianakis]

On Mon, Sep 1, 2025 at 3:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Currently all calls to arm_load_dtb will result in an exit if we fail.
> By passing Error * we can use &error_fatal and properly set the error
> report.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


>  include/hw/arm/boot.h |  3 ++-
>  hw/arm/boot.c         | 35 +++++++++++++++--------------------
>  hw/arm/virt.c         |  6 +++---
>  3 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
> index a2e22bda8a5..fdb99c0c1ee 100644
> --- a/include/hw/arm/boot.h
> +++ b/include/hw/arm/boot.h
> @@ -164,6 +164,7 @@ AddressSpace *arm_boot_address_space(ARMCPU *cpu,
>   * @addr_limit: upper limit of the available memory area at @addr
>   * @as:         address space to load image to
>   * @cpu:        ARM CPU object
> + * @errp:       Error object, often &error_fatal
>   *
>   * Load a device tree supplied by the machine or by the user  with the
>   * '-dtb' command line option, and put it at offset @addr in target
> @@ -181,7 +182,7 @@ AddressSpace *arm_boot_address_space(ARMCPU *cpu,
>   */
>  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
> -                 ARMCPU *cpu);
> +                 ARMCPU *cpu, Error **errp);
>
>  /* Write a secure board setup routine with a dummy handler for SMCs */
>  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index f9d0bc7011e..d28ae8b86ab 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -517,7 +517,7 @@ static void fdt_add_psci_node(void *fdt, ARMCPU *armcpu)
>
>  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
> -                 ARMCPU *cpu)
> +                 ARMCPU *cpu, Error **errp)
>  {
>      g_autofree void *fdt = NULL;
>      g_auto(GStrv) node_path = NULL;
> @@ -525,25 +525,24 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      uint32_t acells, scells;
>      unsigned int i;
>      hwaddr mem_base, mem_len;
> -    Error *err = NULL;
>
>      if (binfo->dtb_filename) {
>          g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_DTB,
>                                                     binfo->dtb_filename);
>          if (!filename) {
> -            fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
> +            error_setg(errp, "Couldn't open dtb file %s", binfo->dtb_filename);
>              return -1;
>          }
>
>          fdt = load_device_tree(filename, &size);
>          if (!fdt) {
> -            fprintf(stderr, "Couldn't open dtb file %s\n", filename);
> +            error_setg(errp, "Couldn't open dtb file %s", filename);
>              return -1;
>          }
>      } else {
>          fdt = binfo->get_dtb(binfo, &size);
>          if (!fdt) {
> -            fprintf(stderr, "Board was unable to create a dtb blob\n");
> +            error_setg(errp, "Board was unable to create a dtb blob");
>              return -1;
>          }
>      }
> @@ -561,7 +560,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
>                                     NULL, &error_fatal);
>      if (acells == 0 || scells == 0) {
> -        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> +        error_setg(errp, "dtb file invalid (#address-cells or #size-cells 0)");
>          return -1;
>      }
>
> @@ -569,15 +568,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          /* This is user error so deserves a friendlier error message
>           * than the failure of setprop_sized_cells would provide
>           */
> -        fprintf(stderr, "qemu: dtb file not compatible with "
> -                "RAM size > 4GB\n");
> +        error_setg(errp, "qemu: dtb file not compatible with RAM size > 4GB");
>          return -1;
>      }
>
>      /* nop all root nodes matching /memory or /memory@unit-address */
> -    node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
> -    if (err) {
> -        error_report_err(err);
> +    node_path = qemu_fdt_node_unit_path(fdt, "memory", errp);
> +    if (!node_path) {
>          return -1;
>      }
>      while (node_path[n]) {
> @@ -607,7 +604,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>              rc = fdt_add_memory_node(fdt, acells, mem_base,
>                                       scells, mem_len, i);
>              if (rc < 0) {
> -                fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
> +                error_setg(errp, "couldn't add /memory@%"PRIx64" node",
>                          mem_base);
>                  return -1;
>              }
> @@ -618,7 +615,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          rc = fdt_add_memory_node(fdt, acells, binfo->loader_start,
>                                   scells, binfo->ram_size, -1);
>          if (rc < 0) {
> -            fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
> +            error_setg(errp, "couldn't add /memory@%"PRIx64" node",
>                      binfo->loader_start);
>              return -1;
>          }
> @@ -633,7 +630,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
>                                       ms->kernel_cmdline);
>          if (rc < 0) {
> -            fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +            error_setg(errp, "couldn't set /chosen/bootargs");
>              return -1;
>          }
>      }
> @@ -642,7 +639,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
>                                            acells, binfo->initrd_start);
>          if (rc < 0) {
> -            fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> +            error_setg(errp, "couldn't set /chosen/linux,initrd-start");
>              return -1;
>          }
>
> @@ -651,7 +648,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                                            binfo->initrd_start +
>                                            binfo->initrd_size);
>          if (rc < 0) {
> -            fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> +            error_setg(errp, "couldn't set /chosen/linux,initrd-end");
>              return -1;
>          }
>      }
> @@ -1321,10 +1318,8 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
>       * decided whether to enable PSCI and set the psci-conduit CPU properties.
>       */
>      if (!info->skip_dtb_autoload && have_dtb(info)) {
> -        if (arm_load_dtb(info->dtb_start, info, info->dtb_limit,
> -                         as, ms, cpu) < 0) {
> -            exit(1);
> -        }
> +        arm_load_dtb(info->dtb_start, info, info->dtb_limit,
> +                     as, ms, cpu, &error_fatal);
>      }
>  }
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9326cfc895f..6061e0ddb50 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1803,9 +1803,9 @@ void virt_machine_done(Notifier *notifier, void *data)
>                                         vms->memmap[VIRT_PLATFORM_BUS].size,
>                                         vms->irqmap[VIRT_PLATFORM_BUS]);
>      }
> -    if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as, ms, cpu) < 0) {
> -        exit(1);
> -    }
> +
> +    arm_load_dtb(info->dtb_start, info, info->dtb_limit,
> +                 as, ms, cpu, &error_fatal);
>
>      pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
>                                         &error_abort);
> --
> 2.47.2
>
>

Re: [PATCH 1/4] hw/arm: use g_autofree for filename in arm_load_dtb

[Manos Pitsidianakis]

On Mon, Sep 1, 2025 at 3:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The function has quite a number of exit cases so lets try and clean
> things up with g_autofree. As the fdt hasn't be allocated yet we can
> return directly from the fail point.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


>  hw/arm/boot.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d391cd01bb1..56fd13b9f7c 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -528,20 +528,18 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      Error *err = NULL;
>
>      if (binfo->dtb_filename) {
> -        char *filename;
> -        filename = qemu_find_file(QEMU_FILE_TYPE_DTB, binfo->dtb_filename);
> +        g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_DTB,
> +                                                   binfo->dtb_filename);
>          if (!filename) {
>              fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
> -            goto fail;
> +            return -1;
>          }
>
>          fdt = load_device_tree(filename, &size);
>          if (!fdt) {
>              fprintf(stderr, "Couldn't open dtb file %s\n", filename);
> -            g_free(filename);
>              goto fail;
>          }
> -        g_free(filename);
>      } else {
>          fdt = binfo->get_dtb(binfo, &size);
>          if (!fdt) {
> --
> 2.47.2
>
>

Re: [PATCH 3/4] hw/arm: use g_auto(GStrv) for node_path in arm_load_dtb

[Manos Pitsidianakis]

On Mon, Sep 1, 2025 at 3:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This is potentially more of a bike-shed case as node_path will persist
> until the end of the function.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

>  hw/arm/boot.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 749f2d08341..f9d0bc7011e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -520,11 +520,11 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   ARMCPU *cpu)
>  {
>      g_autofree void *fdt = NULL;
> +    g_auto(GStrv) node_path = NULL;
>      int size, rc, n = 0;
>      uint32_t acells, scells;
>      unsigned int i;
>      hwaddr mem_base, mem_len;
> -    char **node_path;
>      Error *err = NULL;
>
>      if (binfo->dtb_filename) {
> @@ -586,7 +586,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          }
>          n++;
>      }
> -    g_strfreev(node_path);
>
>      /*
>       * We drop all the memory nodes which correspond to empty NUMA nodes
> --
> 2.47.2
>
>

Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Peter Maydell]

On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> With the fdt being protected by g_autofree we can skip the goto fail
> and bail out straight away. The only thing we must take care of is
> stealing the pointer in the one case when we do need it to survive.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/arm/boot.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 56fd13b9f7c..749f2d08341 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
>                   ARMCPU *cpu)
>  {
> -    void *fdt = NULL;
> +    g_autofree void *fdt = NULL;
>      int size, rc, n = 0;
>      uint32_t acells, scells;
>      unsigned int i;


> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>
>      if (fdt != ms->fdt) {
>          g_free(ms->fdt);
> -        ms->fdt = fdt;
> +        ms->fdt = g_steal_pointer(&fdt);
>      }
>
>      return size;
> -> -fail:
> -    g_free(fdt);
> -    return -1;
>  }

Previously, if we get to the end of the function and fdt == ms->fdt
then we continue to use that DTB, and we don't free it.
After this change, if fdt == ms->fdt then we will skip the
g_steal_pointer() and the g_autofree will free the memory,
but leave ms->fdt still pointing to it.

Since arm_load_dtb() is only called once it's a bit unclear
to me whether this can happen -- I think you would need to have
a board-specific arm_boot_info::get_dtb function which returned
the MachineState::fdt pointer. But as this is supposed to
just be a refactoring patch and the previous code clearly was
written to account for the possibility of fdt == ms->fdt,
I think we should continue to handle that case.

thanks
-- PMM

Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> With the fdt being protected by g_autofree we can skip the goto fail
>> and bail out straight away. The only thing we must take care of is
>> stealing the pointer in the one case when we do need it to survive.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/arm/boot.c | 29 ++++++++++++-----------------
>>  1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 56fd13b9f7c..749f2d08341 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
>>                   ARMCPU *cpu)
>>  {
>> -    void *fdt = NULL;
>> +    g_autofree void *fdt = NULL;
>>      int size, rc, n = 0;
>>      uint32_t acells, scells;
>>      unsigned int i;
>
>
>> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>
>>      if (fdt != ms->fdt) {
>>          g_free(ms->fdt);
>> -        ms->fdt = fdt;
>> +        ms->fdt = g_steal_pointer(&fdt);
>>      }
>>
>>      return size;
>> -> -fail:
>> -    g_free(fdt);
>> -    return -1;
>>  }
>
> Previously, if we get to the end of the function and fdt == ms->fdt
> then we continue to use that DTB, and we don't free it.
> After this change, if fdt == ms->fdt then we will skip the
> g_steal_pointer() and the g_autofree will free the memory,
> but leave ms->fdt still pointing to it.
>
> Since arm_load_dtb() is only called once it's a bit unclear
> to me whether this can happen -- I think you would need to have
> a board-specific arm_boot_info::get_dtb function which returned
> the MachineState::fdt pointer. But as this is supposed to
> just be a refactoring patch and the previous code clearly was
> written to account for the possibility of fdt == ms->fdt,
> I think we should continue to handle that case.

Hmm I was thinking we could assert if ms->fdt is set because we clearly
shouldn't be loading one. For arm the only thing that sets ms->fdt is
create_fdt which also implies:

  vms->bootinfo.skip_dtb_autoload = true;

so on start-up we should either create or load - not both.

but then I am confused about why we do another arm_load_dtb in the
machine_done notifier.

Either way I can't see how fdt = g_malloc0(dt_size) could ever match
what might already be in ms->fdt.


>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Peter Maydell]

On Wed, 3 Sept 2025 at 09:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> With the fdt being protected by g_autofree we can skip the goto fail
> >> and bail out straight away. The only thing we must take care of is
> >> stealing the pointer in the one case when we do need it to survive.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  hw/arm/boot.c | 29 ++++++++++++-----------------
> >>  1 file changed, 12 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> >> index 56fd13b9f7c..749f2d08341 100644
> >> --- a/hw/arm/boot.c
> >> +++ b/hw/arm/boot.c
> >> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
> >>                   ARMCPU *cpu)
> >>  {
> >> -    void *fdt = NULL;
> >> +    g_autofree void *fdt = NULL;
> >>      int size, rc, n = 0;
> >>      uint32_t acells, scells;
> >>      unsigned int i;
> >
> >
> >> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >>
> >>      if (fdt != ms->fdt) {
> >>          g_free(ms->fdt);
> >> -        ms->fdt = fdt;
> >> +        ms->fdt = g_steal_pointer(&fdt);
> >>      }
> >>
> >>      return size;
> >> -> -fail:
> >> -    g_free(fdt);
> >> -    return -1;
> >>  }
> >
> > Previously, if we get to the end of the function and fdt == ms->fdt
> > then we continue to use that DTB, and we don't free it.
> > After this change, if fdt == ms->fdt then we will skip the
> > g_steal_pointer() and the g_autofree will free the memory,
> > but leave ms->fdt still pointing to it.
> >
> > Since arm_load_dtb() is only called once it's a bit unclear
> > to me whether this can happen -- I think you would need to have
> > a board-specific arm_boot_info::get_dtb function which returned
> > the MachineState::fdt pointer. But as this is supposed to
> > just be a refactoring patch and the previous code clearly was
> > written to account for the possibility of fdt == ms->fdt,
> > I think we should continue to handle that case.
>
> Hmm I was thinking we could assert if ms->fdt is set because we clearly
> shouldn't be loading one. For arm the only thing that sets ms->fdt is
> create_fdt which also implies:
>
>   vms->bootinfo.skip_dtb_autoload = true;
>
> so on start-up we should either create or load - not both.
>
> but then I am confused about why we do another arm_load_dtb in the
> machine_done notifier.
>
> Either way I can't see how fdt = g_malloc0(dt_size) could ever match
> what might already be in ms->fdt.

Yeah, I don't think it's really going to happen (at least at the
moment: if somebody refactored hw/arm/sbsa-ref.c so that it used
MachineState::fdt rather than having a sort-of-duplicate
SBSAMachinestate::fdt then I think you can end up here).
But mostly my point is that I don't want to have to think about
this and audit all the arm boards with a get_dtb method when
I'm reviewing a patch which is supposed to just be switching this
function from explicit-free to g_autofree...

-- PMM

Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> With the fdt being protected by g_autofree we can skip the goto fail
>>> and bail out straight away. The only thing we must take care of is
>>> stealing the pointer in the one case when we do need it to survive.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  hw/arm/boot.c | 29 ++++++++++++-----------------
>>>  1 file changed, 12 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index 56fd13b9f7c..749f2d08341 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms,
>>>                   ARMCPU *cpu)
>>>  {
>>> -    void *fdt = NULL;
>>> +    g_autofree void *fdt = NULL;
>>>      int size, rc, n = 0;
>>>      uint32_t acells, scells;
>>>      unsigned int i;
>>
>>
>>> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>
>>>      if (fdt != ms->fdt) {
>>>          g_free(ms->fdt);
>>> -        ms->fdt = fdt;
>>> +        ms->fdt = g_steal_pointer(&fdt);
>>>      }
>>>
>>>      return size;
>>> -> -fail:
>>> -    g_free(fdt);
>>> -    return -1;
>>>  }
>>
>> Previously, if we get to the end of the function and fdt == ms->fdt
>> then we continue to use that DTB, and we don't free it.
>> After this change, if fdt == ms->fdt then we will skip the
>> g_steal_pointer() and the g_autofree will free the memory,
>> but leave ms->fdt still pointing to it.
>>
>> Since arm_load_dtb() is only called once it's a bit unclear
>> to me whether this can happen -- I think you would need to have
>> a board-specific arm_boot_info::get_dtb function which returned
>> the MachineState::fdt pointer. But as this is supposed to
>> just be a refactoring patch and the previous code clearly was
>> written to account for the possibility of fdt == ms->fdt,
>> I think we should continue to handle that case.
>
> Hmm I was thinking we could assert if ms->fdt is set because we clearly
> shouldn't be loading one. For arm the only thing that sets ms->fdt is
> create_fdt which also implies:
>
>   vms->bootinfo.skip_dtb_autoload = true;
>
> so on start-up we should either create or load - not both.
>
> but then I am confused about why we do another arm_load_dtb in the
> machine_done notifier.
>
> Either way I can't see how fdt = g_malloc0(dt_size) could ever match
> what might already be in ms->fdt.

Ahh I see it now.
>
>
>>
>> thanks
>> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro