On 18 August 2018 at 01:23, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> Hi Peter,
>
> On 08/09/2018 10:01 AM, Peter Maydell wrote:
>> The Arm IoTKit includes a system control element which
>> provides a block of read-only ID registers and a block
>> of read-write control registers. Implement a minimal
>> version of this.
>>
>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
>> ---
>> hw/misc/Makefile.objs | 1 +
>> include/hw/misc/iotkit-sysctl.h | 50 +++++
>> hw/misc/iotkit-sysctl.c | 324 ++++++++++++++++++++++++++++++++
>> MAINTAINERS | 2 +
>> default-configs/arm-softmmu.mak | 1 +
>> hw/misc/trace-events | 7 +
>> 6 files changed, 385 insertions(+)
>> create mode 100644 include/hw/misc/iotkit-sysctl.h
>> create mode 100644 hw/misc/iotkit-sysctl.c
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 93509008451..dbadb41d57a 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -65,6 +65,7 @@ obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
>> obj-$(CONFIG_TZ_MPC) += tz-mpc.o
>> obj-$(CONFIG_TZ_PPC) += tz-ppc.o
>> obj-$(CONFIG_IOTKIT_SECCTL) += iotkit-secctl.o
>> +obj-$(CONFIG_IOTKIT_SYSCTL) += iotkit-sysctl.o
>>
>> obj-$(CONFIG_PVPANIC) += pvpanic.o
>> obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
>> diff --git a/include/hw/misc/iotkit-sysctl.h
>> b/include/hw/misc/iotkit-sysctl.h
>> new file mode 100644
>> index 00000000000..c3b14ccee4c
>> --- /dev/null
>> +++ b/include/hw/misc/iotkit-sysctl.h
>> @@ -0,0 +1,50 @@
>> +/*
>> + * ARM IoTKit system control element
>> + *
>> + * Copyright (c) 2018 Linaro Limited
>> + * Written by Peter Maydell
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or
>> + * (at your option) any later version.
>> + */
>> +
>> +/*
>> + * This is a model of the "system control element" which is part of the
>> + * Arm IoTKit and documented in
>> + *
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ecm0601256/index.html
>> + * Specifically, it implements the "system information block" and
>> + * "system control register" blocks.
>> + *
>> + * QEMU interface:
>> + * + sysbus MMIO region 0: the system information register bank
>> + * + sysbus MMIO region 1: the system control register bank
>> + */
>> +
>> +#ifndef HW_MISC_IOTKIT_SYSCTL_H
>> +#define HW_MISC_IOTKIT_SYSCTL_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_IOTKIT_SYSCTL "iotkit-sysctl"
>> +#define IOTKIT_SYSCTL(obj) OBJECT_CHECK(IoTKitSysCtl, (obj), \
>> + TYPE_IOTKIT_SYSCTL)
>> +
>> +typedef struct IoTKitSysCtl {
>> + /*< private >*/
>> + SysBusDevice parent_obj;
>> +
>> + /*< public >*/
>> + MemoryRegion sysinfo_iomem;
>> + MemoryRegion sysctl_iomem;
>> +
>> + uint32_t secure_debug;
>> + uint32_t reset_syndrome;
>> + uint32_t reset_mask;
>> + uint32_t gretreg;
>> + uint32_t initsvrtor0;
>> + uint32_t cpuwait;
>> + uint32_t wicctrl;
>> +} IoTKitSysCtl;
>> +
>> +#endif
>> diff --git a/hw/misc/iotkit-sysctl.c b/hw/misc/iotkit-sysctl.c
>> new file mode 100644
>> index 00000000000..9445500be76
>> --- /dev/null
>> +++ b/hw/misc/iotkit-sysctl.c
>> @@ -0,0 +1,324 @@
>> +/*
>> + * ARM IoTKit system control element
>> + *
>> + * Copyright (c) 2018 Linaro Limited
>> + * Written by Peter Maydell
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or
>> + * (at your option) any later version.
>> + */
>> +
>> +/*
>> + * This is a model of the "system control element" which is part of the
>> + * Arm IoTKit and documented in
>> + *
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ecm0601256/index.html
>> + * Specifically, it implements the "system information block" and
>> + * "system control register" blocks.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "trace.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/registerfields.h"
>> +#include "hw/misc/iotkit-sysctl.h"
>> +
>> +/* sysinfo block registers */
>> +REG32(SYS_VERSION, 0x0)
>> +REG32(SYS_CONFIG, 0x4)
>
> I find it a bit confuse to have the both SYSINFO/SYSCONTROL in the same
> "iotkit-sysctl" device. They share the same state but there is no
> particular need for it, then you need connect them as 2 different
> devices in iotkit_realize but they have the same name "iotkit-sysctl".
>
> Why not declare 2 different TypeInfo? I am probably missing what state
> they need to share.
You're right, they don't share anything internally. It just
seemed a bit unnecessary to have a device that implements
2 read-only registers and nothing else. It probably is better
to split them up, though.
>> + /*
>> + * Most of the state here has to do with control of reset and
>> + * similar kinds of power up -- for instance the guest can ask
>> + * what the reason for the last reset was, or forbid reset for
>> + * some causes (like the non-secure watchdog). Most of this is
>> + * not relevant to QEMU, which doesn't really model anything other
>> + * than a full power-on reset.
>> + * We just model the registers as reads-as-written.
>> + */
>> +
>> + switch (offset) {
>> + case A_RESET_SYNDROME:
>> + qemu_log_mask(LOG_UNIMP,
>> + "IoTKit SysCtl RESET_SYNDROME unimplemented\n");
>
> Maybe warn_report() or warn_once() is more appropriate than UNIMP?
LOG_UNIMP is what we generally use for warning about accesses
to unimplemented registers.
>> + case A_SWRESET:
>> + /* One w/o bit to request a reset; all other bits reserved */
>> + if (value & R_SWRESET_SWRESETREQ_MASK) {
>> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> + }
>
> Shouldn't this be:
>
> } else {
> cpu_reset(...);
> }
Why? The register function is "write 1 to request a system reset".
Writing a zero does nothing.
>> + break;
>> + case A_BUSWAIT: /* In IoTKit BUSWAIT is reserved, R/O, zero */
>> + case A_SECDBGSTAT:
>> + case A_PID4 ... A_CID3:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "IoTKit SysCtl write: write of RO offset %x\n",
>> + (int)offset);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "IoTKit SysCtl write: bad offset %x\n", (int)offset);
>> + break;
>> + }
>> +}
>> +
>> +static const MemoryRegionOps iotkit_sysctl_ops = {
>> + .read = iotkit_sysctl_read,
>> + .write = iotkit_sysctl_write,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> + /* byte/halfword accesses are just zero-padded on reads and writes */
>> + .impl.min_access_size = 4,
>> + .impl.max_access_size = 4,
>> + .valid.min_access_size = 1,
>> + .valid.max_access_size = 4,
>> +};
>> +
>> +static void iotkit_sysctl_reset(DeviceState *dev)
>> +{
>> + IoTKitSysCtl *s = IOTKIT_SYSCTL(dev);
>> +
>> + trace_iotkit_sysctl_reset();
>> + s->secure_debug = 0;
>> + s->reset_syndrome = 1;
>> + s->reset_mask = 0;
>> + s->gretreg = 0;
>> + s->initsvrtor0 = 0x10000000;
>
> This one could be a property (now now ;) ).
It could be, but given that we don't actually support letting
the guest change the SVRTOR reset value on the CPU for the
next reset I think that would be overkill.
>> + s->cpuwait = 0;
>> + s->wicctrl = 0;
>> +}
thanks
-- PMM
