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

[Michael Rolnik]

Peter,

I do not understand this comment







*Exactly one of these memory regions (your main "RAM") should beallocated
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_globalyourself for the other two, but
currently we don't.)*

could you please point me to an example/documentation.



as for this one


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

currently I am concerned with CPU and not with boards/devices/models.
I hope, once there is AVR CPU, some people (including me) will join and
create real boards, this one is just a sample. it does not do any real
stuff. it allows to load a simple program and run.
If I start to design a real board/model now it will take me some time and
during this time no one is going to use/profit from AVR CPU in QEMU.
that's why it's called sample.



On Tue, Jun 7, 2016 at 12:55 AM, Peter Maydell <address@hidden>
wrote:

> On 6 June 2016 at 11:37, Michael Rolnik <address@hidden> wrote:
> > Signed-off-by: Michael Rolnik <address@hidden>
> > ---
> >  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
>



-- 
Best Regards,
Michael Rolnik