diff mbox series

[02/14] hw/ppc/spapr.c: fail early if no firmware found in machine_init()

Message ID 20220228175004.8862-3-danielhb413@gmail.com (mailing list archive)
State New, archived
Headers show
Series simple cleanups in spapr files | expand

Commit Message

Daniel Henrique Barboza Feb. 28, 2022, 5:49 p.m. UTC 
The firmware check consists on a file search (qemu_find_file) and load
it via load_imag_targphys(). This validation is not dependent on any
other machine state but it currently being done at the end of
spapr_machine_init(). This means that we can do a lot of stuff and end
up failing at the end for something that we can verify right out of the
gate.

Move this validation to the start of spapr_machine_init() to fail
earlier.  While we're at it, use g_autofree in the 'filename' pointer.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

BALATON Zoltan Feb. 28, 2022, 7:28 p.m. UTC | #1 
On Mon, 28 Feb 2022, Daniel Henrique Barboza wrote:
> The firmware check consists on a file search (qemu_find_file) and load
> it via load_imag_targphys(). This validation is not dependent on any
> other machine state but it currently being done at the end of
> spapr_machine_init(). This means that we can do a lot of stuff and end
> up failing at the end for something that we can verify right out of the
> gate.
>
> Move this validation to the start of spapr_machine_init() to fail
> earlier.  While we're at it, use g_autofree in the 'filename' pointer.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c74543ace3..4cc204f90d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2707,15 +2707,25 @@ static void spapr_machine_init(MachineState *machine)
>     MachineClass *mc = MACHINE_GET_CLASS(machine);
>     const char *bios_default = spapr->vof ? FW_FILE_NAME_VOF : FW_FILE_NAME;
>     const char *bios_name = machine->firmware ?: bios_default;
> +    g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>     const char *kernel_filename = machine->kernel_filename;
>     const char *initrd_filename = machine->initrd_filename;
>     PCIHostState *phb;
>     int i;
>     MemoryRegion *sysmem = get_system_memory();
>     long load_limit, fw_size;
> -    char *filename;
>     Error *resize_hpt_err = NULL;

Keeping it close to where it's used, i.e. right here at the end of the 
declarations would be easier to read, considering that it also inits it 
right away so checking the value in the next line is more straight 
forward.

Grouping with the simliar filename variables is also reasonable but unless 
the value of those change later just removing them and using the 
machine->{kernel,initrd}_filename directly may be simpler. I did that 
before to mac machines to simplify code.

By the way those are declared const char *, does this filename needs to be 
const too? Maybe moving the bios_* vars here too makes more sense to keep 
them together.

Regards,
BALATON Zoltan

> +    if (!filename) {
> +        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) {
> +        error_report("Could not load LPAR firmware '%s'", filename);
> +        exit(1);
> +    }
> +
>     /*
>      * if Secure VM (PEF) support is configured, then initialize it
>      */
> @@ -2996,18 +3006,6 @@ static void spapr_machine_init(MachineState *machine)
>         }
>     }
>
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -    if (!filename) {
> -        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) {
> -        error_report("Could not load LPAR firmware '%s'", filename);
> -        exit(1);
> -    }
> -    g_free(filename);
> -
>     /* FIXME: Should register things through the MachineState's qdev
>      * interface, this is a legacy from the sPAPREnvironment structure
>      * which predated MachineState but had a similar function */
>
David Gibson March 1, 2022, 2:24 a.m. UTC | #2 
On Mon, Feb 28, 2022 at 02:49:52PM -0300, Daniel Henrique Barboza wrote:
> The firmware check consists on a file search (qemu_find_file) and load
> it via load_imag_targphys(). This validation is not dependent on any
> other machine state but it currently being done at the end of
> spapr_machine_init(). This means that we can do a lot of stuff and end
> up failing at the end for something that we can verify right out of the
> gate.
> 
> Move this validation to the start of spapr_machine_init() to fail
> earlier.  While we're at it, use g_autofree in the 'filename' pointer.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c74543ace3..4cc204f90d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2707,15 +2707,25 @@ static void spapr_machine_init(MachineState *machine)
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      const char *bios_default = spapr->vof ? FW_FILE_NAME_VOF : FW_FILE_NAME;
>      const char *bios_name = machine->firmware ?: bios_default;
> +    g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>      const char *kernel_filename = machine->kernel_filename;
>      const char *initrd_filename = machine->initrd_filename;
>      PCIHostState *phb;
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
>      long load_limit, fw_size;
> -    char *filename;
>      Error *resize_hpt_err = NULL;
>  
> +    if (!filename) {
> +        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) {
> +        error_report("Could not load LPAR firmware '%s'", filename);
> +        exit(1);
> +    }
> +
>      /*
>       * if Secure VM (PEF) support is configured, then initialize it
>       */
> @@ -2996,18 +3006,6 @@ static void spapr_machine_init(MachineState *machine)
>          }
>      }
>  
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -    if (!filename) {
> -        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) {
> -        error_report("Could not load LPAR firmware '%s'", filename);
> -        exit(1);
> -    }
> -    g_free(filename);
> -
>      /* FIXME: Should register things through the MachineState's qdev
>       * interface, this is a legacy from the sPAPREnvironment structure
>       * which predated MachineState but had a similar function */
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c74543ace3..4cc204f90d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2707,15 +2707,25 @@  static void spapr_machine_init(MachineState *machine)
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     const char *bios_default = spapr->vof ? FW_FILE_NAME_VOF : FW_FILE_NAME;
     const char *bios_name = machine->firmware ?: bios_default;
+    g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
     PCIHostState *phb;
     int i;
     MemoryRegion *sysmem = get_system_memory();
     long load_limit, fw_size;
-    char *filename;
     Error *resize_hpt_err = NULL;
 
+    if (!filename) {
+        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) {
+        error_report("Could not load LPAR firmware '%s'", filename);
+        exit(1);
+    }
+
     /*
      * if Secure VM (PEF) support is configured, then initialize it
      */
@@ -2996,18 +3006,6 @@  static void spapr_machine_init(MachineState *machine)
         }
     }
 
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-    if (!filename) {
-        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) {
-        error_report("Could not load LPAR firmware '%s'", filename);
-        exit(1);
-    }
-    g_free(filename);
-
     /* FIXME: Should register things through the MachineState's qdev
      * interface, this is a legacy from the sPAPREnvironment structure
      * which predated MachineState but had a similar function */