Re: [Qemu-devel] [PATCH 11/17] lm32: system control model

[Blue Swirl]

On Fri, Feb 11, 2011 at 1:12 AM, Michael Walle <address@hidden> wrote:
> This patch add support for a system control block. It is supposed to
> act as helper for the emulated program. E.g. shutting down the VM or
> printing test results. This model is intended for testing purposes only and
> doesn't fit to any real hardware. Therefore, it is not added to any board
> by default. Instead a user has to add it explicitly with the '-device'
> commandline parameter.
>
> Signed-off-by: Michael Walle <address@hidden>
> ---
>  Makefile.target |    1 +
>  hw/lm32_sys.c   |  156 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events    |    3 +
>  3 files changed, 160 insertions(+), 0 deletions(-)
>  create mode 100644 hw/lm32_sys.c
>
> diff --git a/Makefile.target b/Makefile.target
> index a6bc7ac..185cc96 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -253,6 +253,7 @@ obj-lm32-y += lm32_pic_cpu.o
>  obj-lm32-y += lm32_juart.o
>  obj-lm32-y += lm32_timer.o
>  obj-lm32-y += lm32_uart.o
> +obj-lm32-y += lm32_sys.o
>
>  obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
>  obj-mips-y += mips_addr.o mips_timer.o mips_int.o
> diff --git a/hw/lm32_sys.c b/hw/lm32_sys.c
> new file mode 100644
> index 0000000..66c23a5
> --- /dev/null
> +++ b/hw/lm32_sys.c
> @@ -0,0 +1,156 @@
> +/*
> + *  QEMU model of the LatticeMico32 system control block.
> + *
> + *  Copyright (c) 2010 Michael Walle <address@hidden>
> + *
> + * 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 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/>.
> + */
> +
> +/*
> + * This model is mainly intended for testing purposes and doesn't fit to any
> + * real hardware. On the one hand it provides a control register (R_CTRL) on
> + * the other hand it supports the lm32 tests.
> + *
> + * A write to the control register causes a system shutdown.
> + * Tests first write the pointer to a test name to the test name register
> + * (R_TESTNAME) and then write a zero to the pass/fail register (R_PASSFAIL) 
> if
> + * the test is passed or any non-zero value to it if the test is failed.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include "trace.h"
> +#include "qemu-log.h"
> +#include "sysemu.h"
> +#include "qemu-log.h"
> +
> +enum {
> +    R_CTRL = 0,
> +    R_PASSFAIL,
> +    R_TESTNAME,
> +    R_MAX
> +};
> +
> +#define MAX_TESTNAME_LEN 16
> +
> +struct LM32SysState {
> +    SysBusDevice busdev;
> +    uint32_t base;
> +    uint32_t regs[R_MAX];
> +    uint8_t testname[MAX_TESTNAME_LEN];
> +};
> +typedef struct LM32SysState LM32SysState;
> +
> +static void copy_testname(LM32SysState *s)
> +{
> +    cpu_physical_memory_read(s->regs[R_TESTNAME], s->testname,
> +            MAX_TESTNAME_LEN);
> +    s->testname[MAX_TESTNAME_LEN - 1] = '\0';
> +}
> +
> +static void sys_write(void *opaque, target_phys_addr_t addr, uint32_t value)
> +{
> +    LM32SysState *s = opaque;
> +    char *testname;
> +
> +    trace_lm32_sys_memory_write(addr, value);
> +
> +    addr >>= 2;
> +    switch (addr) {
> +    case R_CTRL:
> +        qemu_system_shutdown_request();
> +        break;
> +    case R_PASSFAIL:
> +        s->regs[addr] = value;
> +        testname = (char *)s->testname;
> +        qemu_log("TC  %-16s %s\n", testname, (value) ? "FAILED" : "OK");
> +        break;
> +    case R_TESTNAME:
> +        s->regs[addr] = value;
> +        copy_testname(s);
> +        break;
> +
> +    default:
> +        hw_error("lm32_sys: write access to unkown register 0x"
> +                TARGET_FMT_plx, addr << 2);

hw_error() calls abort(), so guest could terminate QEMU with this
which is insecure.

> +        break;
> +    }
> +}
> +
> +static CPUReadMemoryFunc * const sys_read_fn[] = {
> +    NULL,
> +    NULL,
> +    NULL,
> +};
> +
> +static CPUWriteMemoryFunc * const sys_write_fn[] = {
> +    NULL,
> +    NULL,
> +    &sys_write,
> +};
> +
> +static void sys_reset(void *opaque)
> +{
> +    LM32SysState *s = opaque;
> +    int i;
> +
> +    for (i = 0; i < R_MAX; i++) {
> +        s->regs[i] = 0;
> +    }
> +    memset(s->testname, 0, MAX_TESTNAME_LEN);
> +}
> +
> +static int lm32_sys_init(SysBusDevice *dev)
> +{
> +    LM32SysState *s = FROM_SYSBUS(typeof(*s), dev);
> +    int sys_regs;
> +
> +    sys_regs = cpu_register_io_memory(sys_read_fn, sys_write_fn, s,
> +            DEVICE_NATIVE_ENDIAN);
> +    sysbus_init_mmio(dev, R_MAX * 4, sys_regs);
> +    sysbus_mmio_map(dev, 0, s->base);

Devices should not map themselves or care what is their address.
Please remove base field and qdev property and move the mapping to
board level.

> +    qemu_register_reset(sys_reset, s);

There's a field in qdev structure for registering a reset function
automatically.