[Qemu-devel] [PATCH 0/2] hw/ppc/spapr: Improve error printing

[Qemu-devel] [PATCH 0/2] hw/ppc/spapr: Improve error printing

[Thomas Huth]

While loading alternate slof.bin images with the "-bios" option
of qemu-softmmu-ppc64, I noticed that the error message is
wrong when trying to load an non-existing file. Also, the
error message is printed with hw_error() which results in a
huge CPU register dump - something you don't expect when you
just typed in a wrong filename, and is rather confusing here
since you've got to find the real error message in the huge
output first. So let's fix that, too.

Thomas Huth (2):
  ppc/spapr: Fix error message when firmware could not be loaded
  hw/ppc/spapr: Use error_report() instead of hw_error()

 hw/ppc/spapr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Fix error message when firmware could not be loaded

[Thomas Huth]

When specifying a non-existing file with the "-bios" parameter, QEMU
complained that it "could not find LPAR rtas". That's obviously a
copy-n-paste bug from the code which loads the spapr-rtas.bin, it
should complain about a missing firmware file instead.
Additionally the error message was printed with hw_error() - which
also dumps the whole CPU state. However, this does not make much
sense here since the CPU is not running yet and thus the registers
only contain zeroes. So let's use error_report() here instead.
And while we're at it, let's also bail out if the firmware file
had zero length.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 61ddc79..226f029 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1641,12 +1641,12 @@ static void ppc_spapr_init(MachineState *machine)
     }
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     if (!filename) {
-        hw_error("Could not find LPAR rtas '%s'\n", bios_name);
+        error_report("Could not find LPAR firmware '%s'", bios_name);
         exit(1);
     }
     fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
-    if (fw_size < 0) {
-        hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
+    if (fw_size <= 0) {
+        error_report("Could not load LPAR firmware '%s'", filename);
         exit(1);
     }
     g_free(filename);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use error_report() instead of hw_error()

[Thomas Huth]

hw_error() is designed for printing CPU-related error messages
(e.g. it also prints a full CPU register dump). For error messages
that are not directly related to CPU problems, a function like
error_report() should be used instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 226f029..92b1c0b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -794,8 +794,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
-        hw_error("FDT too big ! 0x%x bytes (max is 0x%x)\n",
-                 fdt_totalsize(fdt), FDT_MAX_SIZE);
+        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
+                     fdt_totalsize(fdt), FDT_MAX_SIZE);
         exit(1);
     }
 
@@ -1419,7 +1419,7 @@ static void ppc_spapr_init(MachineState *machine)
     rma_alloc_size = kvmppc_alloc_rma(&rma);
 
     if (rma_alloc_size == -1) {
-        hw_error("qemu: Unable to create RMA\n");
+        error_report("Unable to create RMA");
         exit(1);
     }
 
@@ -1520,17 +1520,17 @@ static void ppc_spapr_init(MachineState *machine)
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     if (!filename) {
-        hw_error("Could not find LPAR rtas '%s'\n", "spapr-rtas.bin");
+        error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
         exit(1);
     }
     spapr->rtas_size = get_image_size(filename);
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
-        hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
+        error_report("Could not load LPAR rtas '%s'", filename);
         exit(1);
     }
     if (spapr->rtas_size > RTAS_MAX_SIZE) {
-        hw_error("RTAS too big ! 0x%zx bytes (max is 0x%x)\n",
+        error_report("RTAS too big ! 0x%zx bytes (max is 0x%x)",
                  (size_t)spapr->rtas_size, RTAS_MAX_SIZE);
         exit(1);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] hw/ppc/spapr: Improve error printing

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Wed, Apr 29, 2015 at 09:01:06PM +0200, Thomas Huth wrote:
> While loading alternate slof.bin images with the "-bios" option
> of qemu-softmmu-ppc64, I noticed that the error message is
> wrong when trying to load an non-existing file. Also, the
> error message is printed with hw_error() which results in a
> huge CPU register dump - something you don't expect when you
> just typed in a wrong filename, and is rather confusing here
> since you've got to find the real error message in the huge
> output first. So let's fix that, too.
> 
> Thomas Huth (2):
>   ppc/spapr: Fix error message when firmware could not be loaded
>   hw/ppc/spapr: Use error_report() instead of hw_error()

Thanks, applied to spapr-next.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use error_report() instead of hw_error()

[Alexey Kardashevskiy]

On 04/30/2015 05:01 AM, Thomas Huth wrote:
> hw_error() is designed for printing CPU-related error messages
> (e.g. it also prints a full CPU register dump). For error messages
> that are not directly related to CPU problems, a function like
> error_report() should be used instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/ppc/spapr.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 226f029..92b1c0b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -794,8 +794,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>       _FDT((fdt_pack(fdt)));
>
>       if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -        hw_error("FDT too big ! 0x%x bytes (max is 0x%x)\n",
> -                 fdt_totalsize(fdt), FDT_MAX_SIZE);
> +        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> +                     fdt_totalsize(fdt), FDT_MAX_SIZE);

You did adjust indent here...


>           exit(1);
>       }
>
> @@ -1419,7 +1419,7 @@ static void ppc_spapr_init(MachineState *machine)
>       rma_alloc_size = kvmppc_alloc_rma(&rma);
>
>       if (rma_alloc_size == -1) {
> -        hw_error("qemu: Unable to create RMA\n");
> +        error_report("Unable to create RMA");
>           exit(1);
>       }
>
> @@ -1520,17 +1520,17 @@ static void ppc_spapr_init(MachineState *machine)
>
>       filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
>       if (!filename) {
> -        hw_error("Could not find LPAR rtas '%s'\n", "spapr-rtas.bin");
> +        error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
>           exit(1);
>       }
>       spapr->rtas_size = get_image_size(filename);
>       spapr->rtas_blob = g_malloc(spapr->rtas_size);
>       if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
> -        hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
> +        error_report("Could not load LPAR rtas '%s'", filename);
>           exit(1);
>       }
>       if (spapr->rtas_size > RTAS_MAX_SIZE) {
> -        hw_error("RTAS too big ! 0x%zx bytes (max is 0x%x)\n",
> +        error_report("RTAS too big ! 0x%zx bytes (max is 0x%x)",
>                    (size_t)spapr->rtas_size, RTAS_MAX_SIZE);

... but not here :)


>           exit(1);
>       }
>





-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use error_report() instead of hw_error()

[Thomas Huth]

On Thu, 30 Apr 2015 13:32:05 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 04/30/2015 05:01 AM, Thomas Huth wrote:
> > hw_error() is designed for printing CPU-related error messages
> > (e.g. it also prints a full CPU register dump). For error messages
> > that are not directly related to CPU problems, a function like
> > error_report() should be used instead.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   hw/ppc/spapr.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 226f029..92b1c0b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -794,8 +794,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
> >       _FDT((fdt_pack(fdt)));
> >
> >       if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> > -        hw_error("FDT too big ! 0x%x bytes (max is 0x%x)\n",
> > -                 fdt_totalsize(fdt), FDT_MAX_SIZE);
> > +        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> > +                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> 
> You did adjust indent here...
> 
> 
> >           exit(1);
> >       }
> >
> > @@ -1419,7 +1419,7 @@ static void ppc_spapr_init(MachineState *machine)
> >       rma_alloc_size = kvmppc_alloc_rma(&rma);
> >
> >       if (rma_alloc_size == -1) {
> > -        hw_error("qemu: Unable to create RMA\n");
> > +        error_report("Unable to create RMA");
> >           exit(1);
> >       }
> >
> > @@ -1520,17 +1520,17 @@ static void ppc_spapr_init(MachineState *machine)
> >
> >       filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
> >       if (!filename) {
> > -        hw_error("Could not find LPAR rtas '%s'\n", "spapr-rtas.bin");
> > +        error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
> >           exit(1);
> >       }
> >       spapr->rtas_size = get_image_size(filename);
> >       spapr->rtas_blob = g_malloc(spapr->rtas_size);
> >       if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
> > -        hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
> > +        error_report("Could not load LPAR rtas '%s'", filename);
> >           exit(1);
> >       }
> >       if (spapr->rtas_size > RTAS_MAX_SIZE) {
> > -        hw_error("RTAS too big ! 0x%zx bytes (max is 0x%x)\n",
> > +        error_report("RTAS too big ! 0x%zx bytes (max is 0x%x)",
> >                    (size_t)spapr->rtas_size, RTAS_MAX_SIZE);
> 
> ... but not here :)

Alexey, you're right, of course.

David, since you already picked up that patch, could you maybe amend
this cosmetic cleanup to my patch? Or shall I send a new version of the
patch or a fixup patch?

 Thomas

Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use error_report() instead of hw_error()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

On Thu, Apr 30, 2015 at 07:03:20PM +0200, Thomas Huth wrote:
> On Thu, 30 Apr 2015 13:32:05 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > On 04/30/2015 05:01 AM, Thomas Huth wrote:
> > > hw_error() is designed for printing CPU-related error messages
> > > (e.g. it also prints a full CPU register dump). For error messages
> > > that are not directly related to CPU problems, a function like
> > > error_report() should be used instead.
> > >
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   hw/ppc/spapr.c | 12 ++++++------
> > >   1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 226f029..92b1c0b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -794,8 +794,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
> > >       _FDT((fdt_pack(fdt)));
> > >
> > >       if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> > > -        hw_error("FDT too big ! 0x%x bytes (max is 0x%x)\n",
> > > -                 fdt_totalsize(fdt), FDT_MAX_SIZE);
> > > +        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> > > +                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> > 
> > You did adjust indent here...
> > 
> > 
> > >           exit(1);
> > >       }
> > >
> > > @@ -1419,7 +1419,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >       rma_alloc_size = kvmppc_alloc_rma(&rma);
> > >
> > >       if (rma_alloc_size == -1) {
> > > -        hw_error("qemu: Unable to create RMA\n");
> > > +        error_report("Unable to create RMA");
> > >           exit(1);
> > >       }
> > >
> > > @@ -1520,17 +1520,17 @@ static void ppc_spapr_init(MachineState *machine)
> > >
> > >       filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
> > >       if (!filename) {
> > > -        hw_error("Could not find LPAR rtas '%s'\n", "spapr-rtas.bin");
> > > +        error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
> > >           exit(1);
> > >       }
> > >       spapr->rtas_size = get_image_size(filename);
> > >       spapr->rtas_blob = g_malloc(spapr->rtas_size);
> > >       if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
> > > -        hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
> > > +        error_report("Could not load LPAR rtas '%s'", filename);
> > >           exit(1);
> > >       }
> > >       if (spapr->rtas_size > RTAS_MAX_SIZE) {
> > > -        hw_error("RTAS too big ! 0x%zx bytes (max is 0x%x)\n",
> > > +        error_report("RTAS too big ! 0x%zx bytes (max is 0x%x)",
> > >                    (size_t)spapr->rtas_size, RTAS_MAX_SIZE);
> > 
> > ... but not here :)
> 
> Alexey, you're right, of course.
> 
> David, since you already picked up that patch, could you maybe amend
> this cosmetic cleanup to my patch? Or shall I send a new version of the
> patch or a fixup patch?

Amended, thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]