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.mayd...@linaro.org> wrote: > On 6 June 2016 at 11:37, Michael Rolnik <mrol...@gmail.com> wrote: > > Signed-off-by: Michael Rolnik <mrol...@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 > -- Best Regards, Michael Rolnik
