[PATCH 0/2] Romless QEMU shutdown patches

[PATCH 0/2] Romless QEMU shutdown patches

[Pavel Dovgalyuk]

Some machines require custom ROMs or kernels. They can't be started without
-bios, -kernel, or -pflash options. But this requirement can't be detected
automatically.

Running a romless machine may be needed for automatic introspection of default
machine hardware, when QEMU is started with a single -machine option.

This series provides patches that enable QEMU execution for MIPS and ARM machines,
even when there is no ROM.

---

Pavel Dovgalyuk (2):
      hw/mips: remove exit(1) in case of missing ROM
      hw/arm: remove exit(1) in case of missing ROM


 0 files changed

--
Pavel Dovgalyuk

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

[Pavel Dovgalyuk]

This patch updates MIPS-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 <Pavel.Dovgalyuk@ispras.ru>
---
 0 files changed

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 8ca31e5162..3e8a073922 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -336,10 +336,8 @@ static void mips_fuloong2e_init(MachineState *machine)
         kernel_entry = load_kernel(env);
         write_bootloader(env, memory_region_get_ram_ptr(bios), kernel_entry);
     } else {
-        if (bios_name == NULL) {
-                bios_name = FULOONG_BIOSNAME;
-        }
-        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
+                                  bios_name ?: FULOONG_BIOSNAME);
         if (filename) {
             bios_size = load_image_targphys(filename, 0x1fc00000LL,
                                             BIOS_SIZE);
@@ -349,7 +347,7 @@ static void mips_fuloong2e_init(MachineState *machine)
         }
 
         if ((bios_size < 0 || bios_size > BIOS_SIZE) &&
-            !kernel_filename && !qtest_enabled()) {
+            bios_name && !qtest_enabled()) {
             error_report("Could not load MIPS bios '%s'", bios_name);
             exit(1);
         }
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index c3b0da60cc..fcd8d71262 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -205,10 +205,7 @@ static void mips_jazz_init(MachineState *machine,
     memory_region_add_subregion(address_space, 0xfff00000LL, bios2);
 
     /* load the BIOS image. */
-    if (bios_name == NULL) {
-        bios_name = BIOS_FILENAME;
-    }
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name ?: BIOS_FILENAME);
     if (filename) {
         bios_size = load_image_targphys(filename, 0xfff00000LL,
                                         MAGNUM_BIOS_SIZE);
@@ -216,7 +213,8 @@ static void mips_jazz_init(MachineState *machine,
     } else {
         bios_size = -1;
     }
-    if ((bios_size < 0 || bios_size > MAGNUM_BIOS_SIZE) && !qtest_enabled()) {
+    if ((bios_size < 0 || bios_size > MAGNUM_BIOS_SIZE)
+        && bios_name && !qtest_enabled()) {
         error_report("Could not load MIPS bios '%s'", bios_name);
         exit(1);
     }
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index d95926a89c..e48f284a8c 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1334,10 +1334,8 @@ void mips_malta_init(MachineState *machine)
         /* Load firmware from flash. */
         if (!dinfo) {
             /* Load a BIOS image. */
-            if (bios_name == NULL) {
-                bios_name = BIOS_FILENAME;
-            }
-            filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+            filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
+                                      bios_name ?: BIOS_FILENAME);
             if (filename) {
                 bios_size = load_image_targphys(filename, FLASH_ADDRESS,
                                                 BIOS_SIZE);
@@ -1346,9 +1344,8 @@ void mips_malta_init(MachineState *machine)
                 bios_size = -1;
             }
             if ((bios_size < 0 || bios_size > BIOS_SIZE) &&
-                !kernel_filename && !qtest_enabled()) {
-                error_report("Could not load MIPS bios '%s', and no "
-                             "-kernel argument was specified", bios_name);
+                bios_name && !qtest_enabled()) {
+                error_report("Could not load MIPS bios '%s'", bios_name);
                 exit(1);
             }
         }
diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
index 1b3b762203..f259e7041e 100644
--- a/hw/mips/mipssim.c
+++ b/hw/mips/mipssim.c
@@ -173,10 +173,7 @@ mips_mipssim_init(MachineState *machine)
     /* Map the BIOS / boot exception handler. */
     memory_region_add_subregion(address_space_mem, 0x1fc00000LL, bios);
     /* Load a BIOS / boot exception handler image. */
-    if (bios_name == NULL) {
-        bios_name = BIOS_FILENAME;
-    }
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name ?: BIOS_FILENAME);
     if (filename) {
         bios_size = load_image_targphys(filename, 0x1fc00000LL, BIOS_SIZE);
         g_free(filename);
@@ -184,10 +181,9 @@ mips_mipssim_init(MachineState *machine)
         bios_size = -1;
     }
     if ((bios_size < 0 || bios_size > BIOS_SIZE) &&
-        !kernel_filename && !qtest_enabled()) {
+        bios_name && !qtest_enabled()) {
         /* Bail out if we have neither a kernel image nor boot vector code. */
-        error_report("Could not load MIPS bios '%s', and no "
-                     "-kernel argument was specified", bios_name);
+        error_report("Could not load MIPS bios '%s'", bios_name);
         exit(1);
     } else {
         /* We have a boot vector start address. */

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

[Pavel Dovgalyuk]

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 <Pavel.Dovgalyuk@ispras.ru>
---
 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");
     }
 
     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)

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

[Peter Maydell]

On Wed, 15 Jul 2020 at 11:19, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> 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 <Pavel.Dovgalyuk@ispras.ru>

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

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

[Philippe Mathieu-Daudé]

On 7/20/20 11:56 AM, Peter Maydell wrote:
> On Wed, 15 Jul 2020 at 11:19, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> 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 <Pavel.Dovgalyuk@ispras.ru>
> 
> 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.