[2/2] hw/arm: remove exit(1) in case of missing ROM

Commit Message

Pavel Dovgalyuk July 15, 2020, 10:19 a.m. UTC

This patch updates ARM-based machines to allow starting them without ROM.
In this case CPU starts to execute instructions from the empty memory,
but QEMU allows introspecting the machine configuration.

Signed-off-by: Pavel Dovgalyuk <[e-mail]>
---
 0 files changed

Comments

Peter Maydell July 20, 2020, 9:56 a.m. UTC | #1

On Wed, 15 Jul 2020 at 11:19, Pavel Dovgalyuk <[e-mail]> wrote:
>
> This patch updates ARM-based machines to allow starting them without ROM.
> In this case CPU starts to execute instructions from the empty memory,
> but QEMU allows introspecting the machine configuration.
>
> Signed-off-by: Pavel Dovgalyuk <[e-mail]>

I guess this makes sense -- this is how most of our machines
already behave, so consistency and being able to introspect
the machine config are both worth having. Also these errors
mostly pre-date the 'generic loader' device, which is another
way to load guest code that the error-exit prevents. (You could
even load guest code via the gdbstub if you wanted...)

arm machines affected by this patch:
 * canon-a1100
 * connex
 * verdex
 * sx1
 * sx1-v1
 * cheetah

none of which are commonly-used anyway.

> ---
>  0 files changed
>
> diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
> index b6452d918c..dbad63ffa2 100644
> --- a/hw/arm/digic_boards.c
> +++ b/hw/arm/digic_boards.c
> @@ -102,8 +102,12 @@ static void digic_load_rom(DigicState *s, hwaddr addr,
>          char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, filename);
>
>          if (!fn) {
> -            error_report("Couldn't find rom image '%s'.", filename);
> -            exit(1);
> +            if (bios_name) {
> +                error_report("Couldn't find rom image '%s'.", filename);
> +                exit(1);
> +            } else {
> +                return;
> +            }
>          }
>
>          rom_size = load_image_targphys(fn, addr, max_size);
> diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
> index 3a4bc332c4..a74bb5e27c 100644
> --- a/hw/arm/gumstix.c
> +++ b/hw/arm/gumstix.c
> @@ -60,9 +60,8 @@ static void connex_init(MachineState *machine)
>
>      dinfo = drive_get(IF_PFLASH, 0, 0);
>      if (!dinfo && !qtest_enabled()) {
> -        error_report("A flash image must be given with the "
> -                     "'pflash' parameter");
> -        exit(1);
> +        warn_report("A flash image must be given with the "
> +                    "'pflash' parameter");

I think we should just drop the error message. If we will
start without the flash image, then "A flash image
must be given" is no longer true.

>      }
>
>      if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
> @@ -90,9 +89,8 @@ static void verdex_init(MachineState *machine)
>
>      dinfo = drive_get(IF_PFLASH, 0, 0);
>      if (!dinfo && !qtest_enabled()) {
> -        error_report("A flash image must be given with the "
> -                     "'pflash' parameter");
> -        exit(1);
> +        warn_report("A flash image must be given with the "
> +                    "'pflash' parameter");

Ditto here.

>      }
>
>      if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
> diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
> index 57829b3744..c0ed3d93e9 100644
> --- a/hw/arm/omap_sx1.c
> +++ b/hw/arm/omap_sx1.c
> @@ -191,13 +191,12 @@ static void sx1_init(MachineState *machine, const int version)
>      }
>
>      if (!machine->kernel_filename && !fl_idx && !qtest_enabled()) {
> -        error_report("Kernel or Flash image must be specified");
> -        exit(1);
> +        warn_report("Kernel or Flash image must be specified");

And here.

> +    } else {
> +        /* Load the kernel.  */
> +        arm_load_kernel(mpu->cpu, machine, &sx1_binfo);
>      }

Calling arm_load_kernel() must not be conditional -- it is the
function which makes sure the guest CPU is reset.

(A handful of boards will call arm_load_kernel() only if
!qtest_enabled(), but most call it unconditionally. We should
look at why those handful of boards seem to need the conditional
and either remove it if useless or see if it should be applied
in other places or if arm_load_kernel() itself could be improved
to make the check unnecessary for all boards.)

>
> -    /* Load the kernel.  */
> -    arm_load_kernel(mpu->cpu, machine, &sx1_binfo);
> -
>      /* TODO: fix next line */
>      //~ qemu_console_resize(ds, 640, 480);
>  }
> diff --git a/hw/arm/palm.c b/hw/arm/palm.c
> index 97ca105d29..d4f4a8d07a 100644
> --- a/hw/arm/palm.c
> +++ b/hw/arm/palm.c
> @@ -257,12 +257,11 @@ static void palmte_init(MachineState *machine)
>      }
>
>      if (!rom_loaded && !machine->kernel_filename && !qtest_enabled()) {
> -        fprintf(stderr, "Kernel or ROM image must be specified\n");
> -        exit(1);
> +        warn_report("Kernel or ROM image must be specified");
> +    } else {
> +        /* Load the kernel.  */
> +        arm_load_kernel(mpu->cpu, machine, &palmte_binfo);

Again, drop the warning, and the call to arm_load_kernel()
must not be conditional.

>      }
> -
> -    /* Load the kernel.  */
> -    arm_load_kernel(mpu->cpu, machine, &palmte_binfo);
>  }
>
>  static void palmte_machine_init(MachineClass *mc)

thanks
-- PMM

Philippe Mathieu-Daudé Oct. 9, 2020, 4 p.m. UTC | #2

On 7/20/20 11:56 AM, Peter Maydell wrote:
> On Wed, 15 Jul 2020 at 11:19, Pavel Dovgalyuk <[e-mail]> wrote:
>>
>> This patch updates ARM-based machines to allow starting them without ROM.
>> In this case CPU starts to execute instructions from the empty memory,
>> but QEMU allows introspecting the machine configuration.
>>
>> Signed-off-by: Pavel Dovgalyuk <[e-mail]>
> 
> I guess this makes sense -- this is how most of our machines
> already behave, so consistency and being able to introspect
> the machine config are both worth having. Also these errors
> mostly pre-date the 'generic loader' device, which is another
> way to load guest code that the error-exit prevents. (You could
> even load guest code via the gdbstub if you wanted...)

Note the 'generic loader' device allows you to select any CPU
address space, while gdbstub is restricted to the first CPU.

> 
> Calling arm_load_kernel() must not be conditional -- it is the
> function which makes sure the guest CPU is reset.
> 
> (A handful of boards will call arm_load_kernel() only if
> !qtest_enabled(), but most call it unconditionally. We should
> look at why those handful of boards seem to need the conditional
> and either remove it if useless or see if it should be applied
> in other places or if arm_load_kernel() itself could be improved
> to make the check unnecessary for all boards.)

Who should look at that? Maybe add that as a byte-sized task?

Regards,

Phil.

Patch

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index b6452d918c..dbad63ffa2 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -102,8 +102,12 @@  static void digic_load_rom(DigicState *s, hwaddr addr,
         char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, filename);
 
         if (!fn) {
-            error_report("Couldn't find rom image '%s'.", filename);
-            exit(1);
+            if (bios_name) {
+                error_report("Couldn't find rom image '%s'.", filename);
+                exit(1);
+            } else {
+                return;
+            }
         }
 
         rom_size = load_image_targphys(fn, addr, max_size);
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 3a4bc332c4..a74bb5e27c 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -60,9 +60,8 @@  static void connex_init(MachineState *machine)
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
     if (!dinfo && !qtest_enabled()) {
-        error_report("A flash image must be given with the "
-                     "'pflash' parameter");
-        exit(1);
+        warn_report("A flash image must be given with the "
+                    "'pflash' parameter");
     }
 
     if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
@@ -90,9 +89,8 @@  static void verdex_init(MachineState *machine)
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
     if (!dinfo && !qtest_enabled()) {
-        error_report("A flash image must be given with the "
-                     "'pflash' parameter");
-        exit(1);
+        warn_report("A flash image must be given with the "
+                    "'pflash' parameter");
     }
 
     if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index 57829b3744..c0ed3d93e9 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -191,13 +191,12 @@  static void sx1_init(MachineState *machine, const int version)
     }
 
     if (!machine->kernel_filename && !fl_idx && !qtest_enabled()) {
-        error_report("Kernel or Flash image must be specified");
-        exit(1);
+        warn_report("Kernel or Flash image must be specified");
+    } else {
+        /* Load the kernel.  */
+        arm_load_kernel(mpu->cpu, machine, &sx1_binfo);
     }
 
-    /* Load the kernel.  */
-    arm_load_kernel(mpu->cpu, machine, &sx1_binfo);
-
     /* TODO: fix next line */
     //~ qemu_console_resize(ds, 640, 480);
 }
diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 97ca105d29..d4f4a8d07a 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -257,12 +257,11 @@  static void palmte_init(MachineState *machine)
     }
 
     if (!rom_loaded && !machine->kernel_filename && !qtest_enabled()) {
-        fprintf(stderr, "Kernel or ROM image must be specified\n");
-        exit(1);
+        warn_report("Kernel or ROM image must be specified");
+    } else {
+        /* Load the kernel.  */
+        arm_load_kernel(mpu->cpu, machine, &palmte_binfo);
     }
-
-    /* Load the kernel.  */
-    arm_load_kernel(mpu->cpu, machine, &palmte_binfo);
 }
 
 static void palmte_machine_init(MachineClass *mc)