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

Trieu Huynh posted 4 patches 4 weeks 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>, BALATON Zoltan <balaton@eik.bme.hu>, Bernhard Beschow <shentey@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Nicholas Piggin <npiggin@gmail.com>, Aditya Gupta <adityag@linux.ibm.com>, Glenn Miles <milesg@linux.ibm.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Hannes Reinecke <hare@suse.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, "Clément Chigot" <chigot@adacore.com>, Frederic Konrad <konrad.frederic@yahoo.fr>, Artyom Tarasenko <atar4qemu@gmail.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Stefano Garzarella <sgarzare@redhat.com>
[PATCH 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Trieu Huynh 4 weeks ago
From: Trieu Huynh <vikingtc4@gmail.com>

Check return value of load_image_targphys() and return early on
failure instead of continuing with invalid state.
- Use ret < 0 to handle negative return value.
- No functional changes.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
---
 hw/alpha/dp264.c       |  8 ++++++--
 hw/hppa/machine.c      |  7 +++++--
 hw/m68k/next-cube.c    |  9 ++++++++-
 hw/m68k/q800.c         |  7 +++++--
 hw/m68k/virt.c         |  7 +++++--
 hw/microblaze/boot.c   |  4 ++++
 hw/ppc/amigaone.c      |  9 +++++++++
 hw/ppc/e500.c          | 10 ++++++++++
 hw/ppc/mac_newworld.c  | 10 ++++++++++
 hw/ppc/mac_oldworld.c  | 10 ++++++++++
 hw/ppc/pegasos.c       |  5 +++++
 hw/ppc/pnv.c           | 20 ++++++++++++++++----
 hw/ppc/ppc440_bamboo.c |  4 ++++
 hw/ppc/prep.c          | 10 ++++++++++
 hw/ppc/sam460ex.c      |  5 +++++
 hw/ppc/spapr.c         |  8 ++++++++
 hw/ppc/virtex_ml507.c  |  9 +++++++++
 17 files changed, 129 insertions(+), 13 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 98219f0456..830d14a509 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -189,8 +189,12 @@ 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);
+            if (load_image_targphys(initrd_filename, initrd_base,
+                                ram_size - initrd_base, NULL) < 0) {
+                error_report("could not load initrd '%s'",
+                             initrd_filename);
+                exit(1);
+            }
 
             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 f55e84529f..0bf3bccb07 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -507,8 +507,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
                 exit(1);
             }
 
-            load_image_targphys(initrd_filename, initrd_base, initrd_size,
-                                NULL);
+            if (load_image_targphys(initrd_filename, initrd_base, initrd_size,
+                                NULL) < 0) {
+                error_report("Could not load initrd '%s'", initrd_filename);
+                exit(1);
+            }
             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..77a88d1dde 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -1326,7 +1326,14 @@ 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) {
+    int bios_size = 0;
+    bios_size = load_image_targphys(bios_name, 0x01000000, 0x20000, NULL);
+    if (bios_size <  0) {
+        /* Shoud report failure and exit regardless of qtest enabled or not */
+        error_report("Failed to load firmware (bios size < 0) '%s'.",
+                     bios_name);
+        exit(1);
+    } else if (bios_size < 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..3266617ad7 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -632,8 +632,11 @@ 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);
+            if (load_image_targphys(initrd_filename, initrd_base,
+                                ram_size - initrd_base, NULL) < 0) {
+                error_report("Could not load initrd '%s'", initrd_filename);
+                exit(1);
+            }
             BOOTINFO2(param_ptr, BI_RAMDISK, initrd_base,
                       initrd_size);
         } else {
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index e67900c727..90f52f26e5 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -291,8 +291,11 @@ 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);
+            if (load_image_targphys(initrd_filename, initrd_base,
+                                ram_size - initrd_base, NULL) < 0) {
+                error_report("Could not load initrd '%s'", initrd_filename);
+                exit(1);
+            }
             BOOTINFO2(param_ptr, BI_RAMDISK, initrd_base,
                       initrd_size);
         } else {
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index a6f9ebab90..9fa8885dd3 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -172,6 +172,10 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, bool is_little_endian,
         if (kernel_size < 0) {
             kernel_size = load_image_targphys(kernel_filename, ddr_base,
                                               ramsize, NULL);
+            if (kernel_size < 0) {
+                error_report("could not load kernel '%s'", kernel_filename);
+                exit(EXIT_FAILURE);
+            }
             boot_info.bootstrap_pc = ddr_base;
             high = (ddr_base + kernel_size + 3) & ~3;
         }
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 8074713fbe..bb5e712c1b 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -326,6 +326,10 @@ static void amigaone_init(MachineState *machine)
             exit(1);
         }
         sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE, &error_fatal);
+        if (sz < 0) {
+            error_report("Could not load firmware '%s'", filename);
+            exit(1);
+        }
     }
 
     /* Articia S */
@@ -411,6 +415,11 @@ static void amigaone_init(MachineState *machine)
         loadaddr = MAX(loadaddr, INITRD_MIN_ADDR);
         sz = load_image_targphys(machine->initrd_filename, loadaddr,
                                  bi->bd_info - loadaddr, &error_fatal);
+        if (sz < 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
         bi->initrd_start = loadaddr;
         bi->initrd_end = loadaddr + sz;
     }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index d6ca2e8563..3117a05b59 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1221,6 +1221,11 @@ void ppce500_init(MachineState *machine)
         kernel_size = load_image_targphys(machine->kernel_filename,
                                       cur_base, machine->ram_size - cur_base,
                                       &error_fatal);
+        if (kernel_size < 0) {
+            error_report("Could not load kernel '%s'",
+                         machine->kernel_filename);
+            exit(1);
+        }
         cur_base += kernel_size;
     }
 
@@ -1230,6 +1235,11 @@ void ppce500_init(MachineState *machine)
         initrd_size = load_image_targphys(machine->initrd_filename, initrd_base,
                                           machine->ram_size - initrd_base,
                                           &error_fatal);
+        if (initrd_size < 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
         cur_base = initrd_base + initrd_size;
     }
 
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 3680d96ed3..5e9556af33 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -213,6 +213,11 @@ static void ppc_core99_init(MachineState *machine)
                                               kernel_base,
                                               machine->ram_size - kernel_base,
                                               &error_fatal);
+            if (kernel_size < 0) {
+                error_report("Could not load kernel '%s'",
+                             machine->kernel_filename);
+                exit(1);
+            }
         }
         /* load initrd */
         if (machine->initrd_filename) {
@@ -221,6 +226,11 @@ static void ppc_core99_init(MachineState *machine)
                                               initrd_base,
                                               machine->ram_size - initrd_base,
                                               &error_fatal);
+            if (initrd_size < 0) {
+                error_report("Could not load initrd '%s'",
+                             machine->initrd_filename);
+                exit(1);
+            }
             cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
         } else {
             cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 24d9f2e3d5..fdbbfed52a 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -169,6 +169,11 @@ static void ppc_heathrow_init(MachineState *machine)
                                               kernel_base,
                                               machine->ram_size - kernel_base,
                                               &error_fatal);
+            if (kernel_size < 0) {
+                error_report("Could not load kernel '%s'",
+                             machine->kernel_filename);
+                exit(1);
+            }
         }
         /* load initrd */
         if (machine->initrd_filename) {
@@ -178,6 +183,11 @@ static void ppc_heathrow_init(MachineState *machine)
                                               initrd_base,
                                               machine->ram_size - initrd_base,
                                               &error_fatal);
+            if (initrd_size < 0) {
+                error_report("Could not load initrd '%s'",
+                             machine->initrd_filename);
+                exit(1);
+            }
             cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
         } else {
             cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
index ac9fc5a654..0f03691f52 100644
--- a/hw/ppc/pegasos.c
+++ b/hw/ppc/pegasos.c
@@ -304,6 +304,11 @@ static void pegasos_init(MachineState *machine)
         pm->initrd_addr = MAX(pm->initrd_addr, INITRD_MIN_ADDR);
         sz = load_image_targphys(machine->initrd_filename, pm->initrd_addr,
                             machine->ram_size - pm->initrd_addr, &error_fatal);
+        if (sz < 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
         pm->initrd_size = sz;
     }
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1513575b8f..aecffb211b 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1068,14 +1068,21 @@ static void pnv_init(MachineState *machine)
         exit(1);
     }
 
-    load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
-                        &error_fatal);
+    if (load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
+                        &error_fatal) < 0) {
+        error_report("Could not load OPAL firmware '%s'", fw_filename);
+        exit(1);
+    }
     g_free(fw_filename);
 
     /* load kernel */
     if (machine->kernel_filename) {
-        load_image_targphys(machine->kernel_filename,
-                            KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, &error_fatal);
+        if (load_image_targphys(machine->kernel_filename,
+                        KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, &error_fatal) < 0) {
+            error_report("Could not load kernel '%s'",
+                         machine->kernel_filename);
+            exit(1);
+        }
     }
 
     /* load initrd */
@@ -1084,6 +1091,11 @@ static void pnv_init(MachineState *machine)
         pnv->initrd_size = load_image_targphys(machine->initrd_filename,
                                                pnv->initrd_base,
                                                INITRD_MAX_SIZE, &error_fatal);
+        if (pnv->initrd_size < 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
     }
 
     /* load dtb if passed */
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 2dde008b0e..b9c30d8929 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -244,6 +244,10 @@ static void bamboo_init(MachineState *machine)
         initrd_size = load_image_targphys(initrd_filename, RAMDISK_ADDR,
                                           machine->ram_size - RAMDISK_ADDR,
                                           &error_fatal);
+        if (initrd_size < 0) {
+            error_report("Could not load initrd '%s'", initrd_filename);
+            exit(1);
+        }
     }
 
     /* If we're loading a kernel directly, we must load the device tree too. */
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index e973b34099..172a4afd71 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -382,6 +382,11 @@ static void ibm_40p_init(MachineState *machine)
                                           kernel_base,
                                           machine->ram_size - kernel_base,
                                           &error_fatal);
+        if (kernel_size < 0) {
+            error_report("Could not load kernel '%s'",
+                         machine->kernel_filename);
+            exit(1);
+        }
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, kernel_base);
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
         /* load initrd */
@@ -391,6 +396,11 @@ static void ibm_40p_init(MachineState *machine)
                                               initrd_base,
                                               machine->ram_size - initrd_base,
                                               &error_fatal);
+            if (initrd_size < 0) {
+                error_report("Could not load initrd '%s'",
+                             machine->initrd_filename);
+                exit(1);
+            }
             fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_base);
             fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
         }
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index b13dd224ba..0747555bc8 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -493,6 +493,11 @@ static void sam460ex_init(MachineState *machine)
                                           RAMDISK_ADDR,
                                           machine->ram_size - RAMDISK_ADDR,
                                           &error_fatal);
+        if (initrd_size < 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
     }
 
     /* If we're loading a kernel directly, we must load the device tree too. */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0ab39dfea6..7ab6a47c5b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2865,6 +2865,10 @@ static void spapr_machine_init(MachineState *machine)
         exit(1);
     }
     fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE, &error_fatal);
+    if (fw_size < 0) {
+        error_report("Could not load LPAR firmware '%s'", filename);
+        exit(1);
+    }
 
     /*
      * if Secure VM (PEF) support is configured, then initialize it
@@ -3124,6 +3128,10 @@ static void spapr_machine_init(MachineState *machine)
                                                 spapr->initrd_base,
                                                 load_limit - spapr->initrd_base,
                                                 &error_fatal);
+            if (spapr->initrd_size < 0) {
+                error_report("Could not load initrd '%s'", initrd_filename);
+                exit(1);
+            }
         }
     }
 
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 51b3d7d712..b125561dd6 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -255,6 +255,10 @@ static void virtex_init(MachineState *machine)
             kernel_size = load_image_targphys(kernel_filename,
                                               boot_offset,
                                               machine->ram_size, &error_fatal);
+            if (kernel_size < 0) {
+                error_report("could not load kernel '%s'", kernel_filename);
+                exit(1);
+            }
             boot_info.bootstrap_pc = boot_offset;
             high = boot_info.bootstrap_pc + kernel_size + 8192;
         }
@@ -267,6 +271,11 @@ static void virtex_init(MachineState *machine)
             initrd_size = load_image_targphys(machine->initrd_filename,
                                               high, machine->ram_size - high,
                                               &error_fatal);
+            if (initrd_size < 0) {
+                error_report("could not load initrd '%s'",
+                             machine->initrd_filename);
+                exit(1);
+            }
             high = ROUND_UP(high + initrd_size, 4);
         }
 
-- 
2.43.0
Re: [PATCH 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Aditya Gupta 4 weeks ago
On 10/03/26 23:57, Trieu Huynh wrote:
> From: Trieu Huynh <vikingtc4@gmail.com>
>
> Check return value of load_image_targphys() and return early on
> failure instead of continuing with invalid state.
> - Use ret < 0 to handle negative return value.
> - No functional changes.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
>
> <...snip...>
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1513575b8f..aecffb211b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1068,14 +1068,21 @@ static void pnv_init(MachineState *machine)
>           exit(1);
>       }
>   
> -    load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
> -                        &error_fatal);
> +    if (load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
> +                        &error_fatal) < 0) {
> +        error_report("Could not load OPAL firmware '%s'", fw_filename);
> +        exit(1);
> +    }
>       g_free(fw_filename);
>   
>       /* load kernel */
>       if (machine->kernel_filename) {
> -        load_image_targphys(machine->kernel_filename,
> -                            KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, &error_fatal);
> +        if (load_image_targphys(machine->kernel_filename,
> +                        KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, &error_fatal) < 0) {
> +            error_report("Could not load kernel '%s'",
> +                         machine->kernel_filename);
> +            exit(1);
> +        }
>       }
>   
>       /* load initrd */
> @@ -1084,6 +1091,11 @@ static void pnv_init(MachineState *machine)
>           pnv->initrd_size = load_image_targphys(machine->initrd_filename,
>                                                  pnv->initrd_base,
>                                                  INITRD_MAX_SIZE, &error_fatal);
> +        if (pnv->initrd_size < 0) {
> +            error_report("Could not load initrd '%s'",
> +                         machine->initrd_filename);
> +            exit(1);
> +        }
>       }
>   
>       /* load dtb if passed */
>

Thanks for the patch, but as balaton said, atleast in this case, the 
condition is not needed since it's have error_fatal.

Previously the if checks you added used to there, these were recently 
changed to not have the conditions,
by this patch from vishal:

     commit cd274e83d50ba52ede62d2a8ea0f0ae7cb1ef469
     Author: Vishal Chourasia <vishalc@linux.ibm.com>
     Date:   Fri Oct 24 18:36:03 2025 +0530

     hw/ppc: Pass error_fatal to load_image_targphys()

     Pass error_fatal to load_image_targphys() calls in ppc machine 
initialization
     to capture detailed error information when loading firmware, kernel,
     and initrd images.

Thanks,

- Aditya G



Re: [PATCH 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Aditya Gupta 4 weeks ago
On 11/03/26 00:32, Aditya Gupta wrote:
>
> On 10/03/26 23:57, Trieu Huynh wrote:
>> From: Trieu Huynh <vikingtc4@gmail.com>
>>
>> Check return value of load_image_targphys() and return early on
>> failure instead of continuing with invalid state.
>> - Use ret < 0 to handle negative return value.
>> - No functional changes.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
>> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
>>
>> <...snip...>
>>   diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 1513575b8f..aecffb211b 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1068,14 +1068,21 @@ static void pnv_init(MachineState *machine)
>>           exit(1);
>>       }
>>   -    load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
>> -                        &error_fatal);
>> +    if (load_image_targphys(fw_filename, pnv->fw_load_addr, 
>> FW_MAX_SIZE,
>> +                        &error_fatal) < 0) {
>> +        error_report("Could not load OPAL firmware '%s'", fw_filename);
>> +        exit(1);
>> +    }
>>       g_free(fw_filename);
>>         /* load kernel */
>>       if (machine->kernel_filename) {
>> -        load_image_targphys(machine->kernel_filename,
>> -                            KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, 
>> &error_fatal);
>> +        if (load_image_targphys(machine->kernel_filename,
>> +                        KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE, 
>> &error_fatal) < 0) {
>> +            error_report("Could not load kernel '%s'",
>> +                         machine->kernel_filename);
>> +            exit(1);
>> +        }
>>       }
>>         /* load initrd */
>> @@ -1084,6 +1091,11 @@ static void pnv_init(MachineState *machine)
>>           pnv->initrd_size = 
>> load_image_targphys(machine->initrd_filename,
>> pnv->initrd_base,
>> INITRD_MAX_SIZE, &error_fatal);
>> +        if (pnv->initrd_size < 0) {
>> +            error_report("Could not load initrd '%s'",
>> +                         machine->initrd_filename);
>> +            exit(1);
>> +        }
>>       }
>>         /* load dtb if passed */
>>
>
> Thanks for the patch, but as balaton said, atleast in this case, the 
> condition is not needed since it's have error_fatal.
>
> Previously the if checks you added used to there, these were recently 
> changed to not have the conditions,
> by this patch from vishal:
>
>     commit cd274e83d50ba52ede62d2a8ea0f0ae7cb1ef469
>     Author: Vishal Chourasia <vishalc@linux.ibm.com>
>     Date:   Fri Oct 24 18:36:03 2025 +0530
>
>     hw/ppc: Pass error_fatal to load_image_targphys()
>
>     Pass error_fatal to load_image_targphys() calls in ppc machine 
> initialization
>     to capture detailed error information when loading firmware, kernel,
>     and initrd images.


To add to this, it might be better to pass an error object to 
load_image_targphys, or error_fatal
if we are exiting in the if condition. That gives more details for why 
the load failed.

The NULL arguments were added in this: 
https://lore.kernel.org/qemu-devel/20251024130556.1942835-6-vishalc@linux.ibm.com/

That was because it was intended to not affect other machines, and hence 
it stayed with default
behavior of ignoring return value of load_image_targphys

This can be changed to use similar to PPC's approach to use error_fatal, 
if that's intended.



Thanks,
- Aditya G


Re: [PATCH 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by Trieu Huynh 4 weeks ago
Sorry, I was missed on that change.
Pls ignore patch 1/4.

BRs,

Vào 02:02 AM T.4, 11 Th3, 2026 Aditya Gupta <adityag@linux.ibm.com> đã viết:

>
> On 10/03/26 23:57, Trieu Huynh wrote:
> > From: Trieu Huynh <vikingtc4@gmail.com>
> >
> > Check return value of load_image_targphys() and return early on
> > failure instead of continuing with invalid state.
> > - Use ret < 0 to handle negative return value.
> > - No functional changes.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
> > Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> >
> > <...snip...>
> >
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 1513575b8f..aecffb211b 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1068,14 +1068,21 @@ static void pnv_init(MachineState *machine)
> >           exit(1);
> >       }
> >
> > -    load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
> > -                        &error_fatal);
> > +    if (load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
> > +                        &error_fatal) < 0) {
> > +        error_report("Could not load OPAL firmware '%s'", fw_filename);
> > +        exit(1);
> > +    }
> >       g_free(fw_filename);
> >
> >       /* load kernel */
> >       if (machine->kernel_filename) {
> > -        load_image_targphys(machine->kernel_filename,
> > -                            KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE,
> &error_fatal);
> > +        if (load_image_targphys(machine->kernel_filename,
> > +                        KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE,
> &error_fatal) < 0) {
> > +            error_report("Could not load kernel '%s'",
> > +                         machine->kernel_filename);
> > +            exit(1);
> > +        }
> >       }
> >
> >       /* load initrd */
> > @@ -1084,6 +1091,11 @@ static void pnv_init(MachineState *machine)
> >           pnv->initrd_size =
> load_image_targphys(machine->initrd_filename,
> >                                                  pnv->initrd_base,
> >                                                  INITRD_MAX_SIZE,
> &error_fatal);
> > +        if (pnv->initrd_size < 0) {
> > +            error_report("Could not load initrd '%s'",
> > +                         machine->initrd_filename);
> > +            exit(1);
> > +        }
> >       }
> >
> >       /* load dtb if passed */
> >
>
> Thanks for the patch, but as balaton said, atleast in this case, the
> condition is not needed since it's have error_fatal.
>
> Previously the if checks you added used to there, these were recently
> changed to not have the conditions,
> by this patch from vishal:
>
>      commit cd274e83d50ba52ede62d2a8ea0f0ae7cb1ef469
>      Author: Vishal Chourasia <vishalc@linux.ibm.com>
>      Date:   Fri Oct 24 18:36:03 2025 +0530
>
>      hw/ppc: Pass error_fatal to load_image_targphys()
>
>      Pass error_fatal to load_image_targphys() calls in ppc machine
> initialization
>      to capture detailed error information when loading firmware, kernel,
>      and initrd images.
>
> Thanks,
>
> - Aditya G
>
>
>
Re: [PATCH 1/4] hw/core/loader: fix error handling for load_image_targphys callers
Posted by BALATON Zoltan 4 weeks ago
On Wed, 11 Mar 2026, Trieu Huynh wrote:
> From: Trieu Huynh <vikingtc4@gmail.com>
>
> Check return value of load_image_targphys() and return early on
> failure instead of continuing with invalid state.
> - Use ret < 0 to handle negative return value.
> - No functional changes.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/413
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
> hw/alpha/dp264.c       |  8 ++++++--
> hw/hppa/machine.c      |  7 +++++--
> hw/m68k/next-cube.c    |  9 ++++++++-
> hw/m68k/q800.c         |  7 +++++--
> hw/m68k/virt.c         |  7 +++++--
> hw/microblaze/boot.c   |  4 ++++

I don't know about these above,

> hw/ppc/amigaone.c      |  9 +++++++++
> hw/ppc/e500.c          | 10 ++++++++++
> hw/ppc/mac_newworld.c  | 10 ++++++++++
> hw/ppc/mac_oldworld.c  | 10 ++++++++++
> hw/ppc/pegasos.c       |  5 +++++
> hw/ppc/pnv.c           | 20 ++++++++++++++++----
> hw/ppc/ppc440_bamboo.c |  4 ++++
> hw/ppc/prep.c          | 10 ++++++++++
> hw/ppc/sam460ex.c      |  5 +++++
> hw/ppc/spapr.c         |  8 ++++++++
> hw/ppc/virtex_ml507.c  |  9 +++++++++

but unless I missed something all these call it with error_fatal so the 
check added should never be reached therefore should not be needed.

Regards,
BALATON Zoltan