diff mbox series

[v8,07/14] hw/arm: Load -bios image as a boot ROM for npcm7xx

Message ID 20200825001711.1340443-8-hskinnemoen@google.com (mailing list archive)
State New, archived
Headers show
Series Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines | expand

Commit Message

Zhijian Li (Fujitsu)" via Aug. 25, 2020, 12:17 a.m. UTC 
If a -bios option is specified on the command line, load the image into
the internal ROM memory region, which contains the first instructions
run by the CPU after reset.

If -bios is not specified, the vbootrom included with qemu is loaded by
default.

Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 hw/arm/npcm7xx_boards.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Philippe Mathieu-Daudé Sept. 3, 2020, 6:59 p.m. UTC | #1 
On 8/25/20 2:17 AM, Havard Skinnemoen via wrote:
> If a -bios option is specified on the command line, load the image into
> the internal ROM memory region, which contains the first instructions
> run by the CPU after reset.
> 
> If -bios is not specified, the vbootrom included with qemu is loaded by
> default.
> 
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  hw/arm/npcm7xx_boards.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index 0b9dce2b35..b67e45e913 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -18,12 +18,43 @@
>  
>  #include "hw/arm/npcm7xx.h"
>  #include "hw/core/cpu.h"
> +#include "hw/loader.h"
>  #include "qapi/error.h"
> +#include "qemu-common.h"
>  #include "qemu/units.h"
> +#include "sysemu/sysemu.h"
>  
>  #define NPCM750_EVB_POWER_ON_STRAPS 0x00001ff7
>  #define QUANTA_GSJ_POWER_ON_STRAPS 0x00001fff
>  
> +static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
> +
> +static void npcm7xx_load_bootrom(MachineState *machine, NPCM7xxState *soc)
> +{
> +    g_autofree char *filename = NULL;
> +    int ret;
> +
> +    if (machine->kernel_filename) {
> +        /* Don't load the bootrom if user requested direct kernel boot. */
> +        return;

Wondering why not load it always if available, and what would
happen if booting from a kernel (without bootrom) and running
"reboot" (via SysRq) in the guest I got:

# echo b > /proc/sysrq-trigger
sysrq: Resetting
CPU0: stopping
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.4.32-7dc9442-dirty-8978043 #1
Hardware name: NPCM7XX Chip family
Backtrace:
[<b0107c4c>] (dump_backtrace) from [<b010823c>] (show_stack+0x20/0x24)
 r7:00000000 r6:600f0193 r5:00000000 r4:b0b5b1b8
[<b010821c>] (show_stack) from [<b077b230>] (dump_stack+0x94/0xa8)
[<b077b19c>] (dump_stack) from [<b010a20c>] (handle_IPI+0x1f0/0x38c)
 r7:00000000 r6:00000004 r5:b0b5eb64 r4:b0b729e0
[<b010a01c>] (handle_IPI) from [<b0102264>] (gic_handle_irq+0x9c/0xa0)
 r10:10c5387d r9:b0b01ef8 r8:d0803100 r7:d0802100 r6:d080210c r5:b0b37db4
 r4:b0b04504
[<b01021c8>] (gic_handle_irq) from [<b0101a8c>] (__irq_svc+0x6c/0x90)
Exception stack(0xb0b01ef8 to 0xb0b01f40)
1ee0:                                                       00000000
00012f74
1f00: cfbc1774 b01112e0 b0b00000 00000000 b0b03e28 b0b03e6c b0b5e72c
b08e5ea8
1f20: 10c5387d b0b01f54 b0b01f58 b0b01f48 b0104114 b0104118 600f0013
ffffffff
 r9:b0b00000 r8:b0b5e72c r7:b0b01f2c r6:ffffffff r5:600f0013 r4:b0104118
[<b01040d4>] (arch_cpu_idle) from [<b014b240>] (do_idle+0xec/0x140)
[<b014b154>] (do_idle) from [<b014b56c>] (cpu_startup_entry+0x28/0x2c)
 r9:410fc090 r8:0810c000 r7:00000000 r6:b0a53a40 r5:00000001 r4:000000ce
[<b014b544>] (cpu_startup_entry) from [<b0794f6c>] (rest_init+0xb4/0xbc)
[<b0794eb8>] (rest_init) from [<b0a00c50>] (arch_call_rest_init+0x18/0x1c)
 r5:00000001 r4:b0b72754
[<b0a00c38>] (arch_call_rest_init) from [<b0a010f4>]
(start_kernel+0x428/0x458)
[<b0a00ccc>] (start_kernel) from [<00000000>] (0x0)
Reboot failed -- System halted

So not a problem so far...
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

But still I'd keep loading the bootrom regardless -kernel.

> +    }
> +
> +    if (!bios_name) {
> +        bios_name = npcm7xx_default_bootrom;
> +    }
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +    if (!filename) {
> +        error_report("Could not find ROM image '%s'", bios_name);
> +        exit(1);
> +    }
> +    ret = load_image_mr(filename, &soc->irom);
> +    if (ret < 0) {
> +        error_report("Failed to load ROM image '%s'", filename);
> +        exit(1);
> +    }
> +}
> +
>  static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
>  {
>      memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram);
> @@ -60,6 +91,7 @@ static void npcm750_evb_init(MachineState *machine)
>      npcm7xx_connect_dram(soc, machine->ram);
>      qdev_realize(DEVICE(soc), NULL, &error_fatal);
>  
> +    npcm7xx_load_bootrom(machine, soc);
>      npcm7xx_load_kernel(machine, soc);
>  }
>  
> @@ -71,6 +103,7 @@ static void quanta_gsj_init(MachineState *machine)
>      npcm7xx_connect_dram(soc, machine->ram);
>      qdev_realize(DEVICE(soc), NULL, &error_fatal);
>  
> +    npcm7xx_load_bootrom(machine, soc);
>      npcm7xx_load_kernel(machine, soc);
>  }
>  
>
Havard Skinnemoen Sept. 10, 2020, 11:52 p.m. UTC | #2 
On Thu, Sep 3, 2020 at 11:59 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 8/25/20 2:17 AM, Havard Skinnemoen via wrote:
> > If a -bios option is specified on the command line, load the image into
> > the internal ROM memory region, which contains the first instructions
> > run by the CPU after reset.
> >
> > If -bios is not specified, the vbootrom included with qemu is loaded by
> > default.
> >
> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  hw/arm/npcm7xx_boards.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> > index 0b9dce2b35..b67e45e913 100644
> > --- a/hw/arm/npcm7xx_boards.c
> > +++ b/hw/arm/npcm7xx_boards.c
> > @@ -18,12 +18,43 @@
> >
> >  #include "hw/arm/npcm7xx.h"
> >  #include "hw/core/cpu.h"
> > +#include "hw/loader.h"
> >  #include "qapi/error.h"
> > +#include "qemu-common.h"
> >  #include "qemu/units.h"
> > +#include "sysemu/sysemu.h"
> >
> >  #define NPCM750_EVB_POWER_ON_STRAPS 0x00001ff7
> >  #define QUANTA_GSJ_POWER_ON_STRAPS 0x00001fff
> >
> > +static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
> > +
> > +static void npcm7xx_load_bootrom(MachineState *machine, NPCM7xxState
> *soc)
> > +{
> > +    g_autofree char *filename = NULL;
> > +    int ret;
> > +
> > +    if (machine->kernel_filename) {
> > +        /* Don't load the bootrom if user requested direct kernel boot.
> */
> > +        return;
>
> Wondering why not load it always if available, and what would
> happen if booting from a kernel (without bootrom) and running
> "reboot" (via SysRq) in the guest I got:
>
> # echo b > /proc/sysrq-trigger
> sysrq: Resetting
> CPU0: stopping
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.4.32-7dc9442-dirty-8978043 #1
> Hardware name: NPCM7XX Chip family
> Backtrace:
> [<b0107c4c>] (dump_backtrace) from [<b010823c>] (show_stack+0x20/0x24)
>  r7:00000000 r6:600f0193 r5:00000000 r4:b0b5b1b8
> [<b010821c>] (show_stack) from [<b077b230>] (dump_stack+0x94/0xa8)
> [<b077b19c>] (dump_stack) from [<b010a20c>] (handle_IPI+0x1f0/0x38c)
>  r7:00000000 r6:00000004 r5:b0b5eb64 r4:b0b729e0
> [<b010a01c>] (handle_IPI) from [<b0102264>] (gic_handle_irq+0x9c/0xa0)
>  r10:10c5387d r9:b0b01ef8 r8:d0803100 r7:d0802100 r6:d080210c r5:b0b37db4
>  r4:b0b04504
> [<b01021c8>] (gic_handle_irq) from [<b0101a8c>] (__irq_svc+0x6c/0x90)
> Exception stack(0xb0b01ef8 to 0xb0b01f40)
> 1ee0:                                                       00000000
> 00012f74
> 1f00: cfbc1774 b01112e0 b0b00000 00000000 b0b03e28 b0b03e6c b0b5e72c
> b08e5ea8
> 1f20: 10c5387d b0b01f54 b0b01f58 b0b01f48 b0104114 b0104118 600f0013
> ffffffff
>  r9:b0b00000 r8:b0b5e72c r7:b0b01f2c r6:ffffffff r5:600f0013 r4:b0104118
> [<b01040d4>] (arch_cpu_idle) from [<b014b240>] (do_idle+0xec/0x140)
> [<b014b154>] (do_idle) from [<b014b56c>] (cpu_startup_entry+0x28/0x2c)
>  r9:410fc090 r8:0810c000 r7:00000000 r6:b0a53a40 r5:00000001 r4:000000ce
> [<b014b544>] (cpu_startup_entry) from [<b0794f6c>] (rest_init+0xb4/0xbc)
> [<b0794eb8>] (rest_init) from [<b0a00c50>] (arch_call_rest_init+0x18/0x1c)
>  r5:00000001 r4:b0b72754
> [<b0a00c38>] (arch_call_rest_init) from [<b0a010f4>]
> (start_kernel+0x428/0x458)
> [<b0a00ccc>] (start_kernel) from [<00000000>] (0x0)
> Reboot failed -- System halted
>
> So not a problem so far...
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> But still I'd keep loading the bootrom regardless -kernel.
>

So you mean something like this?

@@ -35,11 +35,6 @@ static void npcm7xx_load_bootrom(MachineState *machine,
NPCM7xxState *soc)
     g_autofree char *filename = NULL;
     int ret;

-    if (machine->kernel_filename) {
-        /* Don't load the bootrom if user requested direct kernel boot. */
-        return;
-    }
-
     if (!bios_name) {
         bios_name = npcm7xx_default_bootrom;
     }
@@ -47,7 +42,11 @@ static void npcm7xx_load_bootrom(MachineState *machine,
NPCM7xxState *soc)
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     if (!filename) {
         error_report("Could not find ROM image '%s'", bios_name);
-        exit(1);
+        if (!machine->kernel_filename) {
+            /* We can't boot without a bootrom or a kernel image. */
+            exit(1);
+        }
+        return;
     }
     ret = load_image_mr(filename, &soc->irom);
     if (ret < 0) {

I'm not sure if it will make much of a difference, as (IIUC)
arm_load_kernel installs a reset hook to jump directly into the kernel,
bypassing the bootrom entirely. I'm assuming a watchdog or SW reset would
trigger the same reset hook and jump right back into the kernel, though I
could be wrong.

Anyway, I can make the above change in any case.

Havard
diff mbox series

Patch

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 0b9dce2b35..b67e45e913 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -18,12 +18,43 @@ 
 
 #include "hw/arm/npcm7xx.h"
 #include "hw/core/cpu.h"
+#include "hw/loader.h"
 #include "qapi/error.h"
+#include "qemu-common.h"
 #include "qemu/units.h"
+#include "sysemu/sysemu.h"
 
 #define NPCM750_EVB_POWER_ON_STRAPS 0x00001ff7
 #define QUANTA_GSJ_POWER_ON_STRAPS 0x00001fff
 
+static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
+
+static void npcm7xx_load_bootrom(MachineState *machine, NPCM7xxState *soc)
+{
+    g_autofree char *filename = NULL;
+    int ret;
+
+    if (machine->kernel_filename) {
+        /* Don't load the bootrom if user requested direct kernel boot. */
+        return;
+    }
+
+    if (!bios_name) {
+        bios_name = npcm7xx_default_bootrom;
+    }
+
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+    if (!filename) {
+        error_report("Could not find ROM image '%s'", bios_name);
+        exit(1);
+    }
+    ret = load_image_mr(filename, &soc->irom);
+    if (ret < 0) {
+        error_report("Failed to load ROM image '%s'", filename);
+        exit(1);
+    }
+}
+
 static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
 {
     memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram);
@@ -60,6 +91,7 @@  static void npcm750_evb_init(MachineState *machine)
     npcm7xx_connect_dram(soc, machine->ram);
     qdev_realize(DEVICE(soc), NULL, &error_fatal);
 
+    npcm7xx_load_bootrom(machine, soc);
     npcm7xx_load_kernel(machine, soc);
 }
 
@@ -71,6 +103,7 @@  static void quanta_gsj_init(MachineState *machine)
     npcm7xx_connect_dram(soc, machine->ram);
     qdev_realize(DEVICE(soc), NULL, &error_fatal);
 
+    npcm7xx_load_bootrom(machine, soc);
     npcm7xx_load_kernel(machine, soc);
 }