Re: [Qemu-devel] [PATCH v4 3/3] arm: Add BBC micro:bit machine

[Peter Maydell]

On 3 August 2018 at 06:21, Joel Stanley <address@hidden> wrote:
> This adds the base for a machine model of the BBC micro:bit:
>
>   https://en.wikipedia.org/wiki/Micro_Bit
>
> This is a system with a nRF51 SoC containing the main processor, with
> various peripherals on board.
>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Joel Stanley <address@hidden>
> ---
> v2:
>  - Instead of setting kernel filename property, load the image directly
>  - Add link to hardware overview website
> v3:
>  - Rebase microbit on m0 changes
>  - Remove hard-coded flash size and retrieve from the soc
>  - Add Stefan's reviewed-by
> ---
>  hw/arm/Makefile.objs |  2 +-
>  hw/arm/microbit.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/microbit.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index e31875ec69bc..2798a257921d 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -36,4 +36,4 @@ obj-$(CONFIG_MSF2) += msf2-soc.o msf2-som.o
>  obj-$(CONFIG_IOTKIT) += iotkit.o
>  obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>  obj-$(CONFIG_ARM_SMMUV3) += smmu-common.o smmuv3.o
> -obj-$(CONFIG_NRF51_SOC) += nrf51_soc.o
> +obj-$(CONFIG_NRF51_SOC) += nrf51_soc.o microbit.o
> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> new file mode 100644
> index 000000000000..ecf64e883f4f
> --- /dev/null
> +++ b/hw/arm/microbit.c
> @@ -0,0 +1,54 @@
> +/*
> + * BBC micro:bit machine
> + * http://tech.microbit.org/hardware/
> + *
> + * Copyright 2018 Joel Stanley <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/arm/arm.h"
> +#include "exec/address-spaces.h"
> +
> +#include "hw/arm/nrf51_soc.h"
> +
> +typedef struct {
> +    MachineState parent;
> +
> +    NRF51State nrf51;
> +} MICROBITMachineState;

Can we call this "MicrobitMachineState", please? I don't
think the board name is all-caps, and our convention
for struct type names is CamelCase.

> +
> +#define TYPE_MICROBIT_MACHINE "microbit"

Should be MACHINE_TYPE_NAME("microbit")

> +
> +#define MICROBIT_MACHINE(obj) \
> +    OBJECT_CHECK(MICROBITMachineState, obj, TYPE_MICROBIT_MACHINE)
> +
> +static void microbit_init(MachineState *machine)
> +{
> +    MICROBITMachineState *s = g_new(MICROBITMachineState, 1);

This is odd. The MICROBITMachineState is the state struct
for your subclass of MachineState, and that object has
already been allocated (you get a pointer to it as the
argument to the init function here). So all you need to do
is cast it to the right type:
      MICROBITMachineState *s = MICROBIT_MACHINE(machine);

You don't need to allocate a second copy. (You do need to
get the type registration right so that you declare that the
type is of size sizeof(MICROBITMachineState) rather than
just sizeof(MachineState), though -- see below.)

> +    MemoryRegion *system_memory = get_system_memory();
> +    Object *soc;
> +
> +    object_initialize(&s->nrf51, sizeof(s->nrf51), TYPE_NRF51_SOC);
> +    soc = OBJECT(&s->nrf51);
> +    object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);

You should use the new function init_sysbus_child() here
rather than doing separate initialize and add_child steps
(we realised late in the 3.0 cycle that we had refcount
leaks as a result of doing this as a 2-step process, hence
the new function).

> +    object_property_set_link(soc, OBJECT(system_memory),
> +                             "memory", &error_abort);
> +
> +    object_property_set_bool(soc, true, "realized", &error_abort);

Better to use error_fatal rather than error_abort in these two calls,
I think.

> +
> +    arm_m_profile_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> +            NRF51_SOC(soc)->flash_size);
> +}
> +
> +static void microbit_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "BBC micro:bit";
> +    mc->init = microbit_init;
> +    mc->max_cpus = 1;
> +}
> +DEFINE_MACHINE("microbit", microbit_machine_init);

Your subclass of TYPE_MACHINE has extra state, so it can't
use DEFINE_MACHINE (which creates a subclass whose instance_size
is the same as the parent TYPE_MACHINE). You need to do this
longhand:

static void machine_class_init(ObjectClass *oc, void *data)
{
    MachineClass *mc = MACHINE_CLASS(oc);

    mc->desc = "BBC micro:bit";
    mc->init = microbit_init;
    mc->max_cpus = 1;
}

static const TypeInfo microbit_info = {
    .name = TYPE_MICROBIT_MACHINE,
    .parent = TYPE_MACHINE,
    .instance_size = sizeof(MICROBITMachineState),
    .class_init = microbit_class_init,
};

static void microbit_machine_init(void)
{
    type_register_static(&microbit_info);
}

type_init(microbit_machine_init);


(code untested but should be correct; compare against other boards.)

thanks
-- PMM