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