Re: [Qemu-devel] [PATCH v4 3/9] target-avr: adding a sample AVR board

[Peter Maydell <peter.maydell@linaro.org>]

On 6 June 2016 at 11:37, Michael Rolnik <mrolnik@gmail.com> wrote:
> Signed-off-by: Michael Rolnik <mrolnik@gmail.com>
> ---
>  hw/Makefile.objs     |   1 +
>  hw/avr/Makefile.objs |   1 +
>  hw/avr/sample-io.c   | 217 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/sample.c      | 118 ++++++++++++++++++++++++++++
>  4 files changed, 337 insertions(+)
>  create mode 100644 hw/avr/Makefile.objs
>  create mode 100644 hw/avr/sample-io.c
>  create mode 100644 hw/avr/sample.c

You're probably better off having the device in one
patch and the board model in another, rather than combining them.

> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 4a07ed4..262ca15 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -33,6 +33,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
>  devices-dirs-$(CONFIG_SOFTMMU) += xen/
>  devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
>  devices-dirs-$(CONFIG_SMBIOS) += smbios/
> +devices-dirs-$(CONFIG_SOFTMMU) += avr/

No other target uses this for their hw/<architecture> directory,
which is a clue that you don't need it. Makefile.target adds
hw/$(TARGET_BASE_ARCH) automatically.

>  devices-dirs-y += core/
>  common-obj-y += $(devices-dirs-y)
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> new file mode 100644
> index 0000000..9f6be2f
> --- /dev/null
> +++ b/hw/avr/Makefile.objs
> @@ -0,0 +1 @@
> +obj-y   += sample.o sample-io.o
> diff --git a/hw/avr/sample-io.c b/hw/avr/sample-io.c
> new file mode 100644
> index 0000000..7bf5e48
> --- /dev/null
> +++ b/hw/avr/sample-io.c

Generally, device models don't live in hw/<arch>, only board
models. Put the device model in the appropriate subdirectory
of hw/, which is 'misc' for this one.

> @@ -0,0 +1,217 @@
> +/*
> + *  QEMU AVR CPU
> + *
> + *  Copyright (c) 2016 Michael Rolnik
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, see
> + *  <http://www.gnu.org/licenses/lgpl-2.1.html>
> + */

So what actually is this device? Is it something that
corresponds to real hardware, or to some other emulator's
debug/test device, or something we've just made up?
This is a good place to put a comment answering this kind of
question (with links or references to documentation if relevant).

> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "cpu.h"
> +#include "include/hw/sysbus.h"
> +
> +#define TYPE_SAMPLEIO   "SampleIO"
> +#define SAMPLEIO(obj)   OBJECT_CHECK(SAMPLEIOState, (obj), TYPE_SAMPLEIO)
> +
> +#ifndef DEBUG_SAMPLEIO
> +#define DEBUG_SAMPLEIO 1

Don't enable debug by default.

> +#endif
> +
> +#define DPRINTF(fmt, args...)                                                 \
> +    do {                                                                      \
> +        if (DEBUG_SAMPLEIO) {                                                 \
> +            fprintf(stderr, "[%s]%s: " fmt , TYPE_SAMPLEIO, __func__, ##args);\
> +        }                                                                     \
> +    }                                                                         \
> +    while (0)

You might want to consider using tracepoints rather than
a raw debug macro. I don't insist on it, but they're pretty neat.
(You list your trace points in the trace-events file and then
that automatically generates functions trace_whatever that you
call at the relevant points in your code. There are various
backends but by default you should be able to enable them
at runtime with '-d trace:some_glob_pattern' (eg
'-d trace:avr-sample-*'). Example device doing this:
hw/intc/aspeed_vic.c.

> +
> +#define AVR_IO_CPU_REGS_SIZE    0x0020
> +#define AVR_IO_CPU_IO_SIZE      0x0040
> +#define AVR_IO_EXTERN_IO_SIZE   0x00a0
> +#define AVR_IO_SIZE             (AVR_IO_CPU_REGS_SIZE   \
> +                                + AVR_IO_CPU_IO_SIZE    \
> +                                + AVR_IO_EXTERN_IO_SIZE)
> +
> +#define AVR_IO_CPU_REGS_BASE    0x0000
> +#define AVR_IO_CPU_IO_BASE      (AVR_IO_CPU_REGS_BASE   \
> +                                + AVR_IO_CPU_REGS_SIZE)
> +#define AVR_IO_EXTERN_IO_BASE   (AVR_IO_CPU_IO_BASE     \
> +                                + AVR_IO_CPU_IO_SIZE)
> +
> +
> +typedef struct SAMPLEIOState {
> +    SysBusDevice    parent;
> +
> +    MemoryRegion    iomem;
> +
> +    AVRCPU         *cpu;
> +
> +    uint8_t         io[0x40];
> +    uint8_t         exio[0xa0];

Since you've defined constants for these you don't need to hardcode
the values here.

> +} SAMPLEIOState;
> +
> +static uint64_t sample_io_read(void *opaque, hwaddr offset, unsigned size);
> +static void sample_io_write(void *opaque, hwaddr offset, uint64_t value, unsigned size);
> +static int sample_io_init(DeviceState *sbd);
> +static void sample_io_class_init(ObjectClass *klass, void *data);
> +static void sample_io_register_types(void);
> +
> +static void write_Rx(CPUAVRState *env, int inst, uint8_t data);
> +static uint8_t read_Rx(CPUAVRState *env, int inst);

If you order things the other way up you won't need all these
forward declarations.

> +static const
> +MemoryRegionOps     sample_io_ops = {
> +    .read = sample_io_read,
> +    .write = sample_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static
> +Property            sample_io_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};

You don't need to define a property list if it's just empty.

> +static const
> +VMStateDescription  sample_io_vmstate = {
> +    .name = TYPE_SAMPLEIO,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[])
> +    {

You need to actually list your state fields here...

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +void write_Rx(CPUAVRState *env, int inst, uint8_t data)
> +{
> +    env->r[inst] = data;
> +}
> +uint8_t     read_Rx(CPUAVRState *env, int inst)
> +{
> +    return  env->r[inst];
> +}

As Richard says you have problems with trying to write
CPU registers from a device anyway, but please consider
trying to have some level of abstraction rather than
just having the device code reach into the CPU object.
The general model here is real hardware and devices, and
a real device has no access into the inside workings of
another one except via whatever interfaces the other
device explicitly provides.

(Better still would be if we don't need to do any of this
at all, because it gets pretty ugly pretty quickly.
The guest has access to its own registers by definition,
so having a second way to read and write them via memory
is a bit weird.)

> +
> +static
> +void sample_io_reset(DeviceState *dev)
> +{
> +    DPRINTF("\n");

You seem to have guest writable state, so you need to do
something in your reset function (eg memset it to zero).

> +}
> +
> +static
> +uint64_t    sample_io_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    SAMPLEIOState *s = SAMPLEIO(opaque);
> +    AVRCPU *cpu = s->cpu;
> +    CPUAVRState *env = &cpu->env;
> +    uint64_t res = 0;
> +
> +    assert(size == 1);
> +
> +    if (AVR_IO_CPU_REGS_BASE <= offset
> +        && offset < (AVR_IO_CPU_REGS_BASE + AVR_IO_CPU_REGS_SIZE)) {
> +        res = read_Rx(env, offset - AVR_IO_CPU_REGS_BASE);
> +    } else if (AVR_IO_CPU_IO_BASE <= offset
> +            && offset < (AVR_IO_CPU_IO_BASE + AVR_IO_CPU_IO_SIZE)) {
> +        /*  TODO: do IO related stuff here  */

...like what?

> +        res = s->io[offset - AVR_IO_CPU_IO_BASE];
> +    } else if (AVR_IO_EXTERN_IO_BASE <= offset
> +            && offset < (AVR_IO_EXTERN_IO_BASE + AVR_IO_EXTERN_IO_SIZE)) {
> +        /*  TODO: do IO related stuff here  */
> +        res = s->io[offset - AVR_IO_EXTERN_IO_BASE];
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
> +    return  res;
> +}
> +
> +static
> +void sample_io_write(void *opaque, hwaddr offset, uint64_t value, unsigned size)
> +{
> +    SAMPLEIOState *s = SAMPLEIO(opaque);
> +    AVRCPU *cpu = s->cpu;
> +    CPUAVRState *env = &cpu->env;
> +
> +    assert(size == 1);
> +
> +    if (AVR_IO_CPU_REGS_BASE <= offset
> +        && offset < (AVR_IO_CPU_REGS_BASE + AVR_IO_CPU_REGS_SIZE)) {

Consider using a switch with the "case LOW ... HIGH" range
syntax? It might be a little more readable, maybe.

> +        return  write_Rx(env, offset - AVR_IO_CPU_REGS_BASE, value);
> +    } else if (AVR_IO_CPU_IO_BASE <= offset
> +            && offset < (AVR_IO_CPU_IO_BASE + AVR_IO_CPU_IO_SIZE)) {
> +        /*  TODO: do IO related stuff here  */
> +        s->io[offset - AVR_IO_CPU_IO_BASE] = value;
> +    } else if (AVR_IO_EXTERN_IO_BASE <= offset
> +            && offset < (AVR_IO_EXTERN_IO_BASE + AVR_IO_EXTERN_IO_SIZE)) {
> +        /*  TODO: do IO related stuff here  */
> +        s->io[offset - AVR_IO_EXTERN_IO_BASE] = value;
> +    } else {
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static
> +int         sample_io_init(DeviceState *dev)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    SAMPLEIOState *s = SAMPLEIO(dev);
> +
> +    assert(AVR_IO_SIZE <= TARGET_PAGE_SIZE);

Why do we care whether this is true or not?

> +
> +    s->cpu = AVR_CPU(qemu_get_cpu(0));
> +
> +    memory_region_init_io(
> +            &s->iomem,
> +            OBJECT(s),
> +            &sample_io_ops,
> +            s,
> +            TYPE_SAMPLEIO,
> +            AVR_IO_SIZE);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    return  0;
> +}
> +
> +static
> +void sample_io_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    DPRINTF("\n");

All this printing of newlines doesn't seem to be very useful
debug-wise.

> +
> +    dc->init = sample_io_init;
> +    dc->reset = sample_io_reset;
> +    dc->desc = "at90 io regs";
> +    dc->vmsd = &sample_io_vmstate;
> +    dc->props = sample_io_properties;
> +}
> +
> +static const
> +TypeInfo    sample_io_info = {
> +    .name = TYPE_SAMPLEIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SAMPLEIOState),
> +    .class_init = sample_io_class_init,
> +};
> +
> +static
> +void sample_io_register_types(void)
> +{
> +    DPRINTF("\n");
> +    type_register_static(&sample_io_info);
> +}
> +
> +type_init(sample_io_register_types)
> diff --git a/hw/avr/sample.c b/hw/avr/sample.c
> new file mode 100644
> index 0000000..c616aae
> --- /dev/null
> +++ b/hw/avr/sample.c
> @@ -0,0 +1,118 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2016 Michael Rolnik
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see
> + * <http://www.gnu.org/licenses/lgpl-2.1.html>
> + */

Again, you can have a comment here explaining what this board
model is, whether it's modelling some h/w or other simulator, etc.

> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "cpu.h"
> +#include "hw/hw.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/qtest.h"
> +#include "ui/console.h"
> +#include "hw/boards.h"
> +#include "hw/devices.h"
> +#include "hw/loader.h"
> +#include "qemu/error-report.h"
> +#include "exec/address-spaces.h"
> +#include "include/hw/sysbus.h"
> +
> +#define VIRT_BASE_FLASH     0x00000000
> +#define VIRT_BASE_IOREG     0x00000000
> +#define VIRT_BASE_ISRAM     0x00000100
> +#define VIRT_BASE_EXMEM     0x00001100
> +#define VIRT_BASE_EEPROM    0x00000000
> +
> +#define VIRT_BASE_BOOT      0x0001e000
> +#define PHYS_BASE_BOOT      (PHYS_BASE_FLASH + VIRT_BASE_BOOT)
> +
> +#define SIZE_FLASH          0x00020000
> +#define SIZE_IOREG          0x00000100
> +#define SIZE_ISRAM          0x00001000
> +#define SIZE_EXMEM          0x00010000
> +#define SIZE_EEPROM         0x00001000
> +
> +#define PHYS_BASE_FLASH     (PHYS_CODE_BASE)
> +#define PHYS_BASE_IOREG     (PHYS_DATA_BASE)
> +#define PHYS_BASE_ISRAM     (PHYS_BASE_IOREG + SIZE_IOREG)
> +#define PHYS_BASE_EXMEM     (PHYS_BASE_ISRAM + SIZE_ISRAM)
> +#define PHYS_BASE_EEPROM    (PHYS_BASE_EXMEM + SIZE_EXMEM)
> +
> +
> +static void sample_init(MachineState *machine)
> +{
> +    MemoryRegion *address_space_mem = get_system_memory();
> +
> +    MemoryRegion *flash;
> +    MemoryRegion *isram;
> +    MemoryRegion *exmem;
> +
> +    AVRCPU *cpu_avr;
> +    DeviceState *io;
> +    SysBusDevice *bus;
> +
> +    flash = g_new(MemoryRegion, 1);
> +    isram = g_new(MemoryRegion, 1);
> +    exmem = g_new(MemoryRegion, 1);
> +
> +    cpu_avr = cpu_avr_init("avr5");
> +    io = qdev_create(NULL, "SampleIO");
> +    bus = SYS_BUS_DEVICE(io);
> +    qdev_init_nofail(io);
> +
> +    memory_region_init_ram(flash, NULL, "flash", SIZE_FLASH, &error_fatal);
> +    memory_region_init_ram(isram, NULL, "isram", SIZE_ISRAM, &error_fatal);
> +    memory_region_init_ram(exmem, NULL, "exmem", SIZE_EXMEM, &error_fatal);
> +
> +    memory_region_add_subregion(address_space_mem, PHYS_BASE_FLASH, flash);
> +    memory_region_add_subregion(address_space_mem, PHYS_BASE_ISRAM, isram);
> +    memory_region_add_subregion(address_space_mem, PHYS_BASE_EXMEM, exmem);
> +
> +    vmstate_register_ram_global(flash);
> +    vmstate_register_ram_global(isram);
> +    vmstate_register_ram_global(exmem);

Exactly one of these memory regions (your main "RAM") should be
allocated via memory_region_allocate_system_memory()
[which does the vmstate_register_ram_global() for that MR].
The idea is that every board has one-and-only-one main RAM MR.
(We should have a memory_region_allocate_aux_memory()
as the parallel API to save you having to do the vmstate_register_ram_global
yourself for the other two, but currently we don't.)

> +
> +    memory_region_set_readonly(flash, true);
> +
> +    char const *firmware = NULL;
> +    char const *filename;
> +
> +    if (machine->firmware) {
> +        firmware = machine->firmware;
> +    }
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
> +    if (!filename) {
> +        error_report("Could not find flash image file '%s'", firmware);
> +        exit(1);
> +    }
> +
> +    load_image_targphys(filename,   PHYS_BASE_FLASH, SIZE_FLASH);
> +
> +    sysbus_mmio_map(bus, 0, PHYS_BASE_IOREG);
> +}
> +
> +static void sample_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "sample";

This isn't very descriptive; a short phrase which gives users
a clue about whether they want to use this board would be good.

> +    mc->init = sample_init;
> +    mc->is_default = 1;
> +}
> +
> +DEFINE_MACHINE("sample", sample_machine_init)
> --
> 2.4.9 (Apple Git-60)

thanks
-- PMM