[Qemu-devel] [PATCH] microbit: make -kernel optional

[Qemu-devel] [PATCH] microbit: make -kernel optional

[Stefan Hajnoczi]

ARMv7M machine types support -kernel for ELF and raw image files.
Microbit programs are typically in Intel HEX (.hex) format.  The generic
loader supports .hex files but it doesn't work as expected:

  $ qemu-system-arm -M microbit -device loader,file=microbit.hex
  Guest image must be specified (using -kernel)

This error comes from armv7m_load_kernel() but we don't have -kernel in
this case.

This patch makes -kernel optional since most of the time we'll want to
use -device loader instead.

Note that we need to register the reset handler that
armv7m_load_kernel() used to register for us.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/arm/microbit.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index a734e7f650..638f638792 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -14,6 +14,7 @@
 #include "hw/arm/arm.h"
 #include "sysemu/sysemu.h"
 #include "exec/address-spaces.h"
+#include "qemu/error-report.h"
 
 #include "hw/arm/nrf51_soc.h"
 
@@ -28,6 +29,13 @@ typedef struct {
 #define MICROBIT_MACHINE(obj) \
     OBJECT_CHECK(MicrobitMachineState, obj, TYPE_MICROBIT_MACHINE)
 
+static void microbit_cpu_reset(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    cpu_reset(CPU(cpu));
+}
+
 static void microbit_init(MachineState *machine)
 {
     MicrobitMachineState *s = MICROBIT_MACHINE(machine);
@@ -41,8 +49,13 @@ static void microbit_init(MachineState *machine)
                              &error_fatal);
     object_property_set_bool(soc, true, "realized", &error_fatal);
 
-    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
-                       NRF51_SOC(soc)->flash_size);
+    if (machine->kernel_filename) {
+        armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
+                           NRF51_SOC(soc)->flash_size);
+    } else {
+        /* armv7m_load_kernel() does this, we need to do it manually here */
+        qemu_register_reset(microbit_cpu_reset, ARM_CPU(first_cpu));
+    }
 }
 
 static void microbit_machine_class_init(ObjectClass *oc, void *data)
-- 
2.19.2

Re: [Qemu-devel] [PATCH] microbit: make -kernel optional

[Stefan Hajnoczi]

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

On Fri, Dec 14, 2018 at 02:51:52PM +0000, Stefan Hajnoczi wrote:
> ARMv7M machine types support -kernel for ELF and raw image files.
> Microbit programs are typically in Intel HEX (.hex) format.  The generic
> loader supports .hex files but it doesn't work as expected:
> 
>   $ qemu-system-arm -M microbit -device loader,file=microbit.hex
>   Guest image must be specified (using -kernel)
> 
> This error comes from armv7m_load_kernel() but we don't have -kernel in
> this case.
> 
> This patch makes -kernel optional since most of the time we'll want to
> use -device loader instead.
> 
> Note that we need to register the reset handler that
> armv7m_load_kernel() used to register for us.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/arm/microbit.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

Ping

> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> index a734e7f650..638f638792 100644
> --- a/hw/arm/microbit.c
> +++ b/hw/arm/microbit.c
> @@ -14,6 +14,7 @@
>  #include "hw/arm/arm.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
> +#include "qemu/error-report.h"
>  
>  #include "hw/arm/nrf51_soc.h"
>  
> @@ -28,6 +29,13 @@ typedef struct {
>  #define MICROBIT_MACHINE(obj) \
>      OBJECT_CHECK(MicrobitMachineState, obj, TYPE_MICROBIT_MACHINE)
>  
> +static void microbit_cpu_reset(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> +}
> +
>  static void microbit_init(MachineState *machine)
>  {
>      MicrobitMachineState *s = MICROBIT_MACHINE(machine);
> @@ -41,8 +49,13 @@ static void microbit_init(MachineState *machine)
>                               &error_fatal);
>      object_property_set_bool(soc, true, "realized", &error_fatal);
>  
> -    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> -                       NRF51_SOC(soc)->flash_size);
> +    if (machine->kernel_filename) {
> +        armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> +                           NRF51_SOC(soc)->flash_size);
> +    } else {
> +        /* armv7m_load_kernel() does this, we need to do it manually here */
> +        qemu_register_reset(microbit_cpu_reset, ARM_CPU(first_cpu));
> +    }
>  }
>  
>  static void microbit_machine_class_init(ObjectClass *oc, void *data)
> -- 
> 2.19.2
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] microbit: make -kernel optional

[Peter Maydell]

On Fri, 14 Dec 2018 at 14:51, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> ARMv7M machine types support -kernel for ELF and raw image files.
> Microbit programs are typically in Intel HEX (.hex) format.  The generic
> loader supports .hex files but it doesn't work as expected:
>
>   $ qemu-system-arm -M microbit -device loader,file=microbit.hex
>   Guest image must be specified (using -kernel)
>
> This error comes from armv7m_load_kernel() but we don't have -kernel in
> this case.
>
> This patch makes -kernel optional since most of the time we'll want to
> use -device loader instead.
>
> Note that we need to register the reset handler that
> armv7m_load_kernel() used to register for us.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi, Stefan. (Happy new year :-))

Looking at this, I think I'd prefer it if we handled
this in the armv7m_load_kernel() code, since having to register
your own reset handler to reset the CPU is a weird thing for
board level code to have to do. That then means the M-profile
kernel load code is in line with the A-profile load code, which
if it has no kernel to load will still register the reset handler,
and let the machine run however it ends up running.

I think this basically amounts to "remove the 'insist that
kernel_filename is not NULL' check from armv7m_load_kernel()",
aka "revert commit 01fd41ab3fb69971". That commit's commit
message is correct that the behaviour if the user didn't
specify anything to run is a bit unhelpful, but it predates
the generic loader so it's no longer necessarily true that
a kernel must be specified, either for this or for any other
M-profile machine. (There's also the case of "board has a
flash device that was populated via a -pflash or -drive
option".)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] microbit: make -kernel optional

[Stefan Hajnoczi]

On Thu, Jan 3, 2019 at 1:07 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Fri, 14 Dec 2018 at 14:51, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > ARMv7M machine types support -kernel for ELF and raw image files.
> > Microbit programs are typically in Intel HEX (.hex) format.  The generic
> > loader supports .hex files but it doesn't work as expected:
> >
> >   $ qemu-system-arm -M microbit -device loader,file=microbit.hex
> >   Guest image must be specified (using -kernel)
> >
> > This error comes from armv7m_load_kernel() but we don't have -kernel in
> > this case.
> >
> > This patch makes -kernel optional since most of the time we'll want to
> > use -device loader instead.
> >
> > Note that we need to register the reset handler that
> > armv7m_load_kernel() used to register for us.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Hi, Stefan. (Happy new year :-))
>
> Looking at this, I think I'd prefer it if we handled
> this in the armv7m_load_kernel() code, since having to register
> your own reset handler to reset the CPU is a weird thing for
> board level code to have to do. That then means the M-profile
> kernel load code is in line with the A-profile load code, which
> if it has no kernel to load will still register the reset handler,
> and let the machine run however it ends up running.
>
> I think this basically amounts to "remove the 'insist that
> kernel_filename is not NULL' check from armv7m_load_kernel()",
> aka "revert commit 01fd41ab3fb69971". That commit's commit
> message is correct that the behaviour if the user didn't
> specify anything to run is a bit unhelpful, but it predates
> the generic loader so it's no longer necessarily true that
> a kernel must be specified, either for this or for any other
> M-profile machine. (There's also the case of "board has a
> flash device that was populated via a -pflash or -drive
> option".)

Happy New Year!

That makes sense.  I will send a patch.

Stefan