[PATCH v2 1/4] hw/core/loader: fix error handling for load_image_targphys callers

Trieu Huynh posted 4 patches 3 weeks, 6 days ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, Corey Minyard <minyard@acm.org>, Thomas Huth <th.huth+qemu@posteo.eu>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>, Jason Wang <jasowang@redhat.com>, Jiri Pirko <jiri@resnulli.us>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, "Clément Chigot" <chigot@adacore.com>, Frederic Konrad <konrad.frederic@yahoo.fr>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>
There is a newer version of this series
[PATCH v2 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Trieu Huynh 3 weeks, 6 days ago
From: Trieu Huynh <vikingtc4@gmail.com>

Use QEMU's Error API to handle load_image_targphys() failures
consistently across callers.

- Use &error_fatal for callers that previously passed NULL, ensuring
the process exits early on failure instead of continuing in an invalid
state.
- No functional changes.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>

---
v2:
- Use &error_fatal instead of manual return value checks.
- Remove redundant checks for ppc callers that already use
&error_fatal.
- Note: This replaces the "ret < 0" approach proposed in v1.
---
 hw/alpha/dp264.c     | 2 +-
 hw/hppa/machine.c    | 2 +-
 hw/m68k/next-cube.c  | 2 +-
 hw/m68k/q800.c       | 2 +-
 hw/m68k/virt.c       | 2 +-
 hw/microblaze/boot.c | 3 ++-
 6 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 98219f0456..2ab3c14747 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -190,7 +190,7 @@ static void clipper_init(MachineState *machine)
             /* Put the initrd image as high in memory as possible.  */
             initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK;
             load_image_targphys(initrd_filename, initrd_base,
-                                ram_size - initrd_base, NULL);
+                                ram_size - initrd_base, &error_fatal);
 
             address_space_stq_le(&address_space_memory, param_offset + 0x100,
                                  initrd_base + 0xfffffc0000000000ULL,
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index ec63dc1297..99a4c22c73 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -507,7 +507,7 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
             }
 
             load_image_targphys(initrd_filename, initrd_base, initrd_size,
-                                NULL);
+                                &error_fatal);
             cpu[0]->env.initrd_base = initrd_base;
             cpu[0]->env.initrd_end  = initrd_base + initrd_size;
         }
diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 26177c7b86..4bc8e72f3e 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -1326,7 +1326,7 @@ static void next_cube_init(MachineState *machine)
     memory_region_init_alias(&m->rom2, NULL, "next.rom2", &m->rom, 0x0,
                              0x20000);
     memory_region_add_subregion(sysmem, 0x0, &m->rom2);
-    if (load_image_targphys(bios_name, 0x01000000, 0x20000, NULL) < 8) {
+    if (load_image_targphys(bios_name, 0x01000000, 0x20000, &error_fatal) < 8) {
         if (!qtest_enabled()) {
             error_report("Failed to load firmware '%s'.", bios_name);
         }
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index ded531394e..c0d78eb7d7 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -633,7 +633,7 @@ static void q800_machine_init(MachineState *machine)
 
             initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK;
             load_image_targphys(initrd_filename, initrd_base,
-                                ram_size - initrd_base, NULL);
+                                ram_size - initrd_base, &error_fatal);
             BOOTINFO2(param_ptr, BI_RAMDISK, initrd_base,
                       initrd_size);
         } else {
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index e67900c727..ffe6e23415 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -292,7 +292,7 @@ static void virt_init(MachineState *machine)
 
             initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK;
             load_image_targphys(initrd_filename, initrd_base,
-                                ram_size - initrd_base, NULL);
+                                ram_size - initrd_base, &error_fatal);
             BOOTINFO2(param_ptr, BI_RAMDISK, initrd_base,
                       initrd_size);
         } else {
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index a6f9ebab90..4ad5ffd34b 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -38,6 +38,7 @@
 #include "hw/core/loader.h"
 #include "elf.h"
 #include "qemu/cutils.h"
+#include "qapi/error.h"
 
 #include "boot.h"
 
@@ -171,7 +172,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, bool is_little_endian,
         /* Not an ELF image nor an u-boot image, try a RAW image.  */
         if (kernel_size < 0) {
             kernel_size = load_image_targphys(kernel_filename, ddr_base,
-                                              ramsize, NULL);
+                                              ramsize, &error_fatal);
             boot_info.bootstrap_pc = ddr_base;
             high = (ddr_base + kernel_size + 3) & ~3;
         }
-- 
2.43.0
Re: [PATCH v2 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Peter Maydell 3 weeks, 5 days ago
On Wed, 11 Mar 2026 at 18:35, Trieu Huynh <vikingtc4@gmail.com> wrote:
>
> From: Trieu Huynh <vikingtc4@gmail.com>
>
> Use QEMU's Error API to handle load_image_targphys() failures
> consistently across callers.
>
> - Use &error_fatal for callers that previously passed NULL, ensuring
> the process exits early on failure instead of continuing in an invalid
> state.
> - No functional changes.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
>
> ---
> v2:
> - Use &error_fatal instead of manual return value checks.
> - Remove redundant checks for ppc callers that already use
> &error_fatal.
> - Note: This replaces the "ret < 0" approach proposed in v1.

Hi; this patch breaks "make check" for me:

  13/1040 qtest+qtest-m68k - qemu:qtest-m68k/qom-test
           ERROR            1.35s   killed by signal 6 SIGABRT
 113/1040 qtest+qtest-m68k - qemu:qtest-m68k/test-hmp
           ERROR            1.36s   killed by signal 6 SIGABRT

This is because these tests check that every machine can
be started without errors (when run with the 'qtest' accelerator),
and now next-cube cannot:

$ ./build/clang/qemu-system-m68k -display none -machine next-cube -accel qtest
qemu-system-m68k: Could not open 'Rev_2.5_v66.bin': No such file or directory

Where the machine init code is calling load_image_targphys()
because the user passed in a particular filename to load,
then we definitely want to use error_fatal. But some of
these cases use these codepaths for "try to load the default
BIOS filename" and rely on ignoring the error if it doesn't
actually exist.

I think of the machines you change here, only next-cube
has this issue, and that change has other problems:

> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 26177c7b86..4bc8e72f3e 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -1326,7 +1326,7 @@ static void next_cube_init(MachineState *machine)
>      memory_region_init_alias(&m->rom2, NULL, "next.rom2", &m->rom, 0x0,
>                               0x20000);
>      memory_region_add_subregion(sysmem, 0x0, &m->rom2);
> -    if (load_image_targphys(bios_name, 0x01000000, 0x20000, NULL) < 8) {
> +    if (load_image_targphys(bios_name, 0x01000000, 0x20000, &error_fatal) < 8) {

We were already doing an error check here, by looking at the return
value and insisting it was at least 8 bytes.

>          if (!qtest_enabled()) {
>              error_report("Failed to load firmware '%s'.", bios_name);

We could if we liked report the error to the user, by passing
an Error* to load_image_targphys(), reporting the error if it
is set, and otherwise reporting "file too short" (since that's
the other reason we'll get here).

The reason for the !qtest_enabled() here, by the way, is to
get the "report this error to the user if run in the normal way,
but don't report the error if this is one of the tests that
is doing the "check every machine can be started" test.)

>          }

thanks
-- PMM