Re: [PATCH 5/5] hw/ppc: Pass errp to load_image_targphys() and report errors

[Vishal Chourasia <vishalc@linux.ibm.com>]

On Fri, Oct 17, 2025 at 01:56:46AM +0530, Vishal Chourasia wrote:
> On Thu, Oct 16, 2025 at 09:27:36PM +0200, BALATON Zoltan wrote:
> > On Thu, 16 Oct 2025, Vishal Chourasia wrote:
> > > Pass errp to load_image_targphys() calls in ppc machine initialization
> > > to capture detailed error information when loading firmware, kernel,
> > > and initrd images.
> > > 
> > > Use error_reportf_err() instead of error_report() to print the
> > > underlying error details along with context about which image failed
> > > to load.
> > > 
> <snipped>
> > > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > > index 7fa14fd0e6..d4703f79da 100644
> > > --- a/hw/ppc/pegasos2.c
> > > +++ b/hw/ppc/pegasos2.c
> > > @@ -129,6 +129,7 @@ static void pegasos2_init(MachineState *machine)
> > >     int i;
> > >     ssize_t sz;
> > >     uint8_t *spd_data;
> > > +    Error *errp = NULL;
> > > 
> > >     /* init CPU */
> > >     pm->cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> > > @@ -164,10 +165,10 @@ static void pegasos2_init(MachineState *machine)
> > >                   ELFDATA2MSB, PPC_ELF_MACHINE, 0, 0);
> > >     if (sz <= 0) {
> > >         sz = load_image_targphys(filename, pm->vof ? 0 : PROM_ADDR, PROM_SIZE,
> > > -                NULL);
> > > +                                 &errp);
> > >     }
> > >     if (sz <= 0 || sz > PROM_SIZE) {
> > > -        error_report("Could not load firmware '%s'", filename);
> > > +        error_reportf_err(errp, "Could not load firmware '%s': ", filename);
> > 
> > We can get here with *errp == NULL if load_elf did not fail but tried to
> > load a too large image. Is that a problem? It's the same in prep.c. Mac
> > machines also try different formats but those only check for size < 0 so
> > maybe not a problem there.
> > 
> 
> /* return < 0 if error, otherwise the number of bytes loaded in memory */
> ssize_t load_elf(const char *filename,
> 
> load_elf returns number of bytes. 
> 
> Yes, this is a problem. If load_elf() succeeds but the returned value
> is greater than PROM_SIZE, then *errp == NULL and this would cause a
> segmentation fault when trying to report the error.
> 
> Any location where an error check is performed based on a condition
> other than *errp (such as size checks) runs the risk of segmentation
> fault.
I will be sending out modifications addressing these in the next
version.

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index d4703f79da..14b679d032 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -168,7 +168,10 @@ static void pegasos2_init(MachineState *machine)
                                  &errp);
     }
     if (sz <= 0 || sz > PROM_SIZE) {
-        error_reportf_err(errp, "Could not load firmware '%s': ", filename);
+        if (!errp) {
+           error_setg(&errp, ": exceeds max supported file size %lu", PROM_SIZE / MiB);
+        }
+        error_reportf_err(errp, "Could not load firmware '%s'", filename);

Similarly for other files too. 


> 
> > >         exit(1);
> > >     }
> > >     g_free(filename);
> > > @@ -260,10 +261,10 @@ static void pegasos2_init(MachineState *machine)
> > >         pm->initrd_addr = ROUND_UP(pm->initrd_addr, 4);
> > >         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, NULL);
> > > +                                 machine->ram_size - pm->initrd_addr, &errp);
> > >         if (sz <= 0) {
> > > -            error_report("Could not load initrd '%s'",
> > > -                         machine->initrd_filename);
> > > +            error_reportf_err(errp, "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 a3e5203970..88668a700e 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -1015,6 +1015,7 @@ static void pnv_init(MachineState *machine)
> > >     char *chip_typename;
> > >     DriveInfo *pnor;
> > >     DeviceState *dev;
> > > +    Error *errp = NULL;
> > > 
> > >     if (kvm_enabled()) {
> > >         error_report("machine %s does not support the KVM accelerator",
> > > @@ -1069,9 +1070,10 @@ static void pnv_init(MachineState *machine)
> > >     }
> > > 
> > >     fw_size = load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
> > > -                                    NULL);
> > > +                                    &errp);
> > >     if (fw_size < 0) {
Also, we replace checks on size with errp.

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 88668a700e..4c94bc319d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1071,7 +1071,7 @@ static void pnv_init(MachineState *machine)

     fw_size = load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
                                     &errp);
-    if (fw_size < 0) {
+    if (errp) {
         error_reportf_err(errp, "Could not load OPAL firmware '%s': ",
                           fw_filename);
         exit(1);

Similarly, at other locations.

Thanks,
vishalc

> > > -        error_report("Could not load OPAL firmware '%s'", fw_filename);
> > > +        error_reportf_err(errp, "Could not load OPAL firmware '%s': ",
> > > +                          fw_filename);
> > >         exit(1);
> > >     }
> > >     g_free(fw_filename);
> > > @@ -1082,10 +1084,10 @@ static void pnv_init(MachineState *machine)
> > > 
> > >         kernel_size = load_image_targphys(machine->kernel_filename,
> > >                                           KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE,
> > > -                                          NULL);
> > > +                                          &errp);
> > >         if (kernel_size < 0) {
> > > -            error_report("Could not load kernel '%s'",
> > > -                         machine->kernel_filename);
> > > +            error_reportf_err(errp, "Could not load kernel '%s': ",
> > > +                              machine->kernel_filename);
> > >             exit(1);
> > >         }
> > >     }
<snipped>
> > > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > > index 00d9ab7509..a7d3de62fa 100644
> > > --- a/hw/ppc/virtex_ml507.c
> > > +++ b/hw/ppc/virtex_ml507.c
> > > @@ -195,6 +195,7 @@ static void virtex_init(MachineState *machine)
> > >     qemu_irq irq[32], cpu_irq;
> > >     int kernel_size;
> > >     int i;
> > > +    Error *errp = NULL;
> > > 
> > >     /* init CPUs */
> > >     cpu = ppc440_init_xilinx(machine->cpu_type, 400000000);
> > > @@ -253,7 +254,7 @@ static void virtex_init(MachineState *machine)
> > >             /* If we failed loading ELF's try a raw image.  */
> > >             kernel_size = load_image_targphys(kernel_filename,
> > >                                               boot_offset,
> > > -                                              machine->ram_size, NULL);
> > > +                                              machine->ram_size, &errp);
> > 
> > What checks and reports errp? Was something left out here?
> The error check was missing here. After reviewing the code, it seems a
> check should be added. I will add it in the next version.
>