[v4,3/9] target-avr: adding a sample AVR board

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

On 06/06/2016 03:37 AM, Michael Rolnik wrote:
> +void write_Rx(CPUAVRState *env, int inst, uint8_t data)
> +{
> +    env->r[inst] = data;
> +}
> +uint8_t     read_Rx(CPUAVRState *env, int inst)

Spacing.  But more importantly...

> +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)) {
> +        return  write_Rx(env, offset - AVR_IO_CPU_REGS_BASE, value);

...

> +static
> +int         sample_io_init(DeviceState *dev)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    SAMPLEIOState *s = SAMPLEIO(dev);
> +
> +    assert(AVR_IO_SIZE <= TARGET_PAGE_SIZE);
> +
> +    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);

This is a memory-mapped device, and you can't modify cpu registers with a 
memory write.

You can read cpu registers, since all TCG globals will have been synced back to 
ENV before the memory operation.  But they'll still be live in TB, and so the 
next instruction may not see the value that you just wrote.

If this write is really something that you need to support, then you'll have to 
figure out some other way of implementing it.

My first guess at a workable solution is to keep the page for this memory 
device read-only.  Detect writes via tlb_fill and throw a special exception. 
In do_interrupt, either handle the write (probably tricky); or set a flag in 
ENV, which in turn sets a flag in TB, which in turn tells translate to use a 
function (helper_writeb?) instead of using tcg_gen_qemu_st_tl.  That helper 
would be marked such that modifications to TCG registers is expected.  Then 
clear the flag in ENV so that subsequent stores use the normal path.


r~

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

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 <peter.maydell@linaro.org>
wrote:

> 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
>

On 5 July 2016 at 23:22, Michael Rolnik <mrolnik@gmail.com> wrote:
> Peter,
>
> I do not understand this comment
>
> 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.)
>
> could you please point me to an example/documentation.

See for instance hw/arm/vexpress.c. We call
memory_region_allocate_system_memory() only once,
for the main RAM. The little bit of RAM "vexpress.a15sram"
is not allocated with that function.

> as for this one
>
> This isn't very descriptive; a short phrase which gives users
> a 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.

Unfortunately it's not quite that simple. Once code goes into
QEMU, we have to maintain it, and we need to continue
to support users who have started using it. That means that
we could easily still have this "sample" board around in five
years time. So you need to get it at least basically right and
useful from the beginning.

My point about the better descriptive phrase is that this
is user-facing documentation (it appears in "-M help" output
where we list all the supported boards). "sample" on its
own doesn't do much to help users or suggest to them or
remind them what this board model is.

thanks
-- PMM