Re: [PATCH v2] hw/ppc/pnv: Improve kernel/initrd load failure error messages

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Oct 15, 2025 at 07:17:17PM +0530, Vishal Chourasia wrote:
> When QEMU fails to load the kernel or initrd image, it previously emitted
> a generic error message such as:
> 
>   qemu-system-ppc64: Could not load kernel 'vmlinux'
> 
> This provides little context on why the failure occurred, which can make
> debugging difficult, especially for new users or when dealing with large
> images.
> 
> The new messages also include the configured size limits (in MiB) to help
> users verify that their image files are within acceptable bounds.
> 
> [v1] https://lore.kernel.org/all/20251007091214.403430-2-vishalc@linux.ibm.com/
> 
> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> ---
>  hw/alpha/dp264.c         |  4 ++--
>  hw/arm/armv7m.c          |  2 +-
>  hw/arm/boot.c            |  5 +++--
>  hw/arm/digic_boards.c    |  2 +-
>  hw/arm/highbank.c        |  3 ++-
>  hw/arm/raspi.c           |  2 +-
>  hw/arm/vexpress.c        |  2 +-
>  hw/core/generic-loader.c |  7 ++++---
>  hw/core/guest-loader.c   |  6 +++---
>  hw/core/loader.c         | 32 ++++++++++++++++++++++++--------
>  hw/hppa/machine.c        |  5 +++--
>  hw/i386/multiboot.c      |  2 +-
>  hw/i386/x86-common.c     |  4 ++--
>  hw/ipmi/ipmi_bmc_sim.c   |  2 +-
>  hw/loongarch/boot.c      |  5 ++---
>  hw/m68k/an5206.c         |  2 +-
>  hw/m68k/mcf5208.c        |  4 ++--
>  hw/m68k/next-cube.c      |  2 +-
>  hw/m68k/q800.c           |  7 ++++---
>  hw/m68k/virt.c           |  4 ++--
>  hw/microblaze/boot.c     |  5 +++--
>  hw/mips/boston.c         |  2 +-
>  hw/mips/fuloong2e.c      |  9 +++++----
>  hw/mips/jazz.c           |  2 +-
>  hw/mips/loongson3_virt.c | 10 ++++++----
>  hw/mips/malta.c          |  9 +++++----
>  hw/nubus/nubus-device.c  |  2 +-
>  hw/openrisc/boot.c       |  5 +++--
>  hw/pci/pci.c             |  2 +-
>  hw/ppc/amigaone.c        |  4 ++--
>  hw/ppc/e500.c            |  5 +++--
>  hw/ppc/mac_newworld.c    |  9 ++++++---
>  hw/ppc/mac_oldworld.c    |  9 ++++++---
>  hw/ppc/pegasos2.c        |  5 +++--
>  hw/ppc/pnv.c             | 22 +++++++++++++---------
>  hw/ppc/ppc440_bamboo.c   |  3 ++-
>  hw/ppc/prep.c            |  8 +++++---
>  hw/ppc/sam460ex.c        |  3 ++-
>  hw/ppc/spapr.c           |  5 +++--
>  hw/ppc/virtex_ml507.c    |  5 +++--
>  hw/riscv/boot.c          |  7 ++++---
>  hw/rx/rx-gdbsim.c        |  2 +-
>  hw/s390x/ipl.c           |  8 +++++---
>  hw/sh4/r2d.c             |  8 +++++---
>  hw/smbios/smbios.c       |  2 +-
>  hw/sparc/leon3.c         |  4 ++--
>  hw/sparc/sun4m.c         |  8 +++++---
>  hw/sparc64/sun4u.c       |  7 ++++---
>  hw/xtensa/xtfpga.c       |  3 ++-
>  include/hw/loader.h      |  8 +++++---
>  system/device_tree.c     |  2 +-
>  51 files changed, 170 insertions(+), 115 deletions(-)
> 

> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index e72bbde2a2..8cf5aadf1f 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -148,13 +148,14 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>  
>          if (size < 0 || s->force_raw) {
>              /* Default to the maximum size being the machine's ram size */
> -            size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
> +            size = load_image_targphys_as(s->file, s->addr,
> +                    current_machine->ram_size, as, errp);
>          } else {
>              s->addr = entry;
>          }
>  
> -        if (size < 0) {
> -            error_setg(errp, "Cannot load specified image %s", s->file);
> +        if (*errp || size < 0) {

We should not have to check both *errp and 'size < 0'.

We must ensure that every code path in 'load_image_targphys_as' that can
return -1, will *always* fills in 'errp', so that callers can be sure
that *errp is always non-NULL on failure.

> +            error_reportf_err(*errp, "Cannot load specified image %s", s->file);

This method is propagating the error to the caller in its 'errp'
parameter, so it is wrong to call error_reportf_err. The latter
should only be used at the final point in the callstack which
owns the 'Error' parameter.

The only change in this method should be to remove the existing
error_setg call.

>              return;
>          }
>      }

> diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> index 3db89d7a2e..d4f749fd6e 100644
> --- a/hw/core/guest-loader.c
> +++ b/hw/core/guest-loader.c
> @@ -101,9 +101,9 @@ static void guest_loader_realize(DeviceState *dev, Error **errp)
>  
>      /* Default to the maximum size being the machine's ram size */
>      size = load_image_targphys_as(file, s->addr, current_machine->ram_size,
> -                                  NULL);
> -    if (size < 0) {
> -        error_setg(errp, "Cannot load specified image %s", file);
> +                                  NULL, errp);
> +    if (*errp || size < 0) {
> +        error_reportf_err(*errp, "Cannot load specified image %s", file);
>          return;

Again must not be calling error_reportf_err nor chcking *errp,
just remove error_setg().

>      }
>  
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 477661a025..d8c02786d2 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -48,6 +48,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/type-helpers.h"
> +#include "qemu/units.h"
>  #include "trace.h"
>  #include "hw/hw.h"
>  #include "disas/disas.h"
> @@ -61,23 +62,31 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "system/memory.h"
>  #include "hw/boards.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "system/runstate.h"
>  #include "tcg/debuginfo.h"
>  
> +#include <errno.h>
>  #include <zlib.h>
>  
>  static int roms_loaded;
>  
>  /* return the size or -1 if error */
> -int64_t get_image_size(const char *filename)
> +int64_t get_image_size(const char *filename, Error **errp)
>  {
>      int fd;
>      int64_t size;
>      fd = open(filename, O_RDONLY | O_BINARY);
> -    if (fd < 0)
> +    if (fd < 0) {
> +        error_setg_file_open(errp, errno, filename);
>          return -1;
> +    }

This perhaps ought to be changed to call 'qemu_open' which
already fills in an Error object, and additionally protects
the fd with O_CLOEXEC and handles FD passing with /dev/fdset

>      size = lseek(fd, 0, SEEK_END);
> +    if (size < 0) {
> +        error_setg_errno(errp, errno, "lseek failure");
> +        return -1;
> +    }
>      close(fd);
>      return size;
>  }
> @@ -118,21 +127,28 @@ ssize_t read_targphys(const char *name,
>  }
>  
>  ssize_t load_image_targphys(const char *filename,
> -                            hwaddr addr, uint64_t max_sz)
> +                            hwaddr addr, uint64_t max_sz, Error **errp)
>  {
> -    return load_image_targphys_as(filename, addr, max_sz, NULL);
> +    return load_image_targphys_as(filename, addr, max_sz, NULL, errp);
>  }
>  
>  /* return the size or -1 if error */
>  ssize_t load_image_targphys_as(const char *filename,
> -                               hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +                               hwaddr addr, uint64_t max_sz, AddressSpace *as,
> +                               Error **errp)
>  {
>      ssize_t size;
>  
> -    size = get_image_size(filename);
> -    if (size < 0 || size > max_sz) {
> +    size = get_image_size(filename, errp);
> +    if (*errp || size < 0) {

Must not chck *errp, only 'size < 0'.

>          return -1;
>      }
> +
> +    if (size > max_sz) {
> +        error_setg(errp, "Exceeds maximum image size (%lu MiB)", max_sz / MiB);
> +        return -1;
> +    }
> +
>      if (size > 0) {
>          if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
>              return -1;
> @@ -150,7 +166,7 @@ ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
>          return -1;
>      }
>  
> -    size = get_image_size(filename);
> +    size = get_image_size(filename, NULL);
>  
>      if (size < 0 || size > memory_region_size(mr)) {
>          return -1;


I'd suggest that we add the Error parameter in one patch, making every
caller pass NULL. Then a second patch update the callers to pass a
non-NULL errp and use error_report_err to print details, ideally  for
more than just the 1 ppc source file. 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|