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

[Peter Maydell]

On 24 July 2016 at 01:02, Michael Rolnik <address@hidden> wrote:
> Signed-off-by: Michael Rolnik <address@hidden>
> ---
>  MAINTAINERS          |   6 ++
>  hw/avr/Makefile.objs |  21 ++++++
>  hw/avr/sample-io.c   | 176 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/sample.c      | 137 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 hw/avr/Makefile.objs
>  create mode 100644 hw/avr/sample-io.c
>  create mode 100644 hw/avr/sample.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d1439a8..1435040 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -110,6 +110,12 @@ F: disas/arm.c
>  F: disas/arm-a64.cc
>  F: disas/libvixl/
>
> +AVR
> +M: Michael Rolnik <address@hidden>
> +S: Maintained
> +F: target-avr/
> +F: hw/avr/
> +
>  CRIS
>  M: Edgar E. Iglesias <address@hidden>
>  S: Maintained
> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> new file mode 100644
> index 0000000..c080e4e
> --- /dev/null
> +++ b/hw/avr/Makefile.objs
> @@ -0,0 +1,21 @@
> +#
> +#  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>
> +#
> +
> +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..fd657aa
> --- /dev/null
> +++ b/hw/avr/sample-io.c
> @@ -0,0 +1,176 @@
> +/*
> + *  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>
> + */
> +
> +/*
> +    NOTE:
> +        This is not a real AVR device !!! This is an example !!!
> +
> +        This example can be used to build a real AVR device.
> +
> +        AVR has the following layout of data memory
> +
> +        LD/ST(addr)                         IN/OUT(addr)
> +        -----------                         ------------
> +
> +        0000    #-----------------------#
> +           .    |                       |
> +           .    |   32 CPU registers    |
> +        001f    |                       |
> +        0020    #-----------------------#   0000
> +           .    |                       |   .
> +           .    |   64 CPU IO registers |   .
> +        005f    |                       |   003f
> +        0060    #-----------------------#
> +           .    |                       |
> +           .    |  160 EXT IO registers |
> +        00ff    |                       |
> +        0100    #-----------------------#
> +                |                       |
> +                |  Internal RAM         |
> +                |                       |
> +                #-----------------------#
> +                |                       |
> +                |  External RAM         |
> +                |                       |
> +                #-----------------------#
> +
> +        Current AVR/CPU implementation assumes that IO device responsible to
> +        implement functionality of IO and EXT IO registers is a memory mapped
> +        device, mapped to addresses in the range [0x0020 .. 0x0100)
> +
> +        IN/OUT are implemented as an alias to LD/ST instructions
> +
> +        Some of CPU IO registers are implemented within the CPU itself, any
> +        access to them either by LD/ST or IN/OUT won't be routed to the 
> device.
> +
> +*/

This device doesn't do anything, so it doesn't need to be here at all.
If you have some real hardware that you want to emulate then you
can write a model of that. (In particular it's not clear whether
some device implementing EXT IO should be like this, or whether
it ought to be provided together with an SoC container object,
and it's hard to make that design choice unless we have an actual
real world example. Better to leave it until we do.)

> diff --git a/hw/avr/sample.c b/hw/avr/sample.c
> new file mode 100644
> index 0000000..173cf9c
> --- /dev/null
> +++ b/hw/avr/sample.c
> @@ -0,0 +1,137 @@
> +/*
> + * 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>
> + */
> +
> +/*
> +    NOTE:
> +        This is not a real AVR board !!! This is an example !!!
> +
> +        This example can be used to build a real AVR board.
> +
> +        This example board loads provided binary file into flash memory and
> +        executes it from 0x00000000 address in the code memory space.
> +
> +        This example does not implement/install any AVR specific on board
> +        devices except SampleIO device which is an example as well.
> +
> +        Currently used for AVR CPU validation
> +
> +*/
> +
> +#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_ISRAM     0x00000100
> +#define VIRT_BASE_EXMEM     0x00001100
> +#define VIRT_BASE_EEPROM    0x00000000
> +
> +#define SIZE_FLASH          0x00020000
> +#define SIZE_ISRAM          0x00001000
> +#define SIZE_EXMEM          0x00010000
> +#define SIZE_EEPROM         0x00001000
> +#define SIZE_IOREG          SIZE_REGS
> +
> +#define PHYS_BASE_FLASH     (PHYS_BASE_CODE)
> +
> +#define PHYS_BASE_ISRAM     (PHYS_BASE_DATA)
> +#define PHYS_BASE_EXMEM     (PHYS_BASE_ISRAM + SIZE_ISRAM)
> +#define PHYS_BASE_EEPROM    (PHYS_BASE_EXMEM + SIZE_EXMEM)
> +
> +#define PHYS_BASE_IOREG     (PHYS_BASE_REGS + 0x20)
> +
> +static void sample_init(MachineState *machine)
> +{
> +    MemoryRegion *address_space_mem = get_system_memory();
> +
> +    MemoryRegion *ram;
> +    MemoryRegion *flash;
> +    MemoryRegion *isram;
> +    MemoryRegion *exmem;
> +    unsigned ram_size = SIZE_FLASH + SIZE_ISRAM + SIZE_EXMEM;
> +
> +    AVRCPU *cpu_avr ATTRIBUTE_UNUSED;
> +    DeviceState *io;
> +    SysBusDevice *bus;
> +
> +    ram = g_new(MemoryRegion, 1);
> +    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");
> +    qdev_init_nofail(io);
> +    bus = SYS_BUS_DEVICE(io);
> +
> +    memory_region_allocate_system_memory(ram, NULL, "avr.ram", ram_size);

You allocate this with a size which is equal to all your individual bits
of RAM added together...

> +
> +    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);

...and then you allocate the pieces of RAM again separately...

> +
> +    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);

...and then you don't actually map your main RAM region.

> +
> +    vmstate_register_ram_global(flash);
> +    vmstate_register_ram_global(isram);
> +    vmstate_register_ram_global(exmem);
> +
> +    memory_region_set_readonly(flash, true);

We have a memory_region_init_rom() now which is equivalent to
doing memory_region_init_ram() + memory_region_set_readonly().

> +
> +    char const *firmware = NULL;
> +    char const *filename;

"const char *", not "char const *", and declarations should
go at the start of {} blocks, not in the middle.

> +
> +    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 = "AVR sample/example board";
> +    mc->init = sample_init;
> +    mc->is_default = 1;
> +}
> +
> +DEFINE_MACHINE("sample", sample_machine_init)
> +
> --
> 2.4.9 (Apple Git-60)
>

thanks
-- PMM