From 36d820af0eddf4fc6a533579b052d8f0085a9fb8 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 29 Jan 2019 11:46:03 +0000 Subject: [PATCH 01/23] target/arm: Fix validation of 32-bit address spaces for aa32 When tsz == 0, aarch32 selects the address space via exclusion, and there are no "top_bits" remaining that require validation. Fixes: ba97be9f4a4 Reported-by: Peter Maydell Signed-off-by: Richard Henderson Message-id: 20190125184913.5970-1-richard.henderson@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/helper.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 92666e5208..e24689f767 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -10447,7 +10447,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, uint64_t ttbr; hwaddr descaddr, indexmask, indexmask_grainsize; uint32_t tableattrs; - target_ulong page_size, top_bits; + target_ulong page_size; uint32_t attrs; int32_t stride; int addrsize, inputsize; @@ -10487,12 +10487,19 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, * We determined the region when collecting the parameters, but we * have not yet validated that the address is valid for the region. * Extract the top bits and verify that they all match select. + * + * For aa32, if inputsize == addrsize, then we have selected the + * region by exclusion in aa32_va_parameters and there is no more + * validation to do here. */ - top_bits = sextract64(address, inputsize, addrsize - inputsize); - if (-top_bits != param.select || (param.select && !ttbr1_valid)) { - /* In the gap between the two regions, this is a Translation fault */ - fault_type = ARMFault_Translation; - goto do_fault; + if (inputsize < addrsize) { + target_ulong top_bits = sextract64(address, inputsize, + addrsize - inputsize); + if (-top_bits != param.select || (param.select && !ttbr1_valid)) { + /* The gap between the two regions is a Translation fault */ + fault_type = ARMFault_Translation; + goto do_fault; + } } if (param.using64k) { From 7e3f122367ec56ea2e8b7313cef82162eb7538c7 Mon Sep 17 00:00:00 2001 From: Thomas Roth Date: Tue, 29 Jan 2019 11:46:03 +0000 Subject: [PATCH 02/23] target/arm: v8m: Ensure IDAU is respected if SAU is disabled The current behavior of v8m_security_lookup in helper.c only checks whether the IDAU specifies a higher security if the SAU is enabled. If SAU.ALLNS is set to 1, this will lead to addresses being treated as non-secure, even though the IDAU indicates that they must be secure. This patch changes the behavior to also check the IDAU if the SAU is currently disabled. (This brings the behaviour here into line with the v8M Arm ARM SecurityCheck() pseudocode.) Signed-off-by: Thomas Roth Message-id: CAGGekkuc+-tvp5RJP7CM+Jy_hJF7eiRHZ96132sb=hPPCappKg@mail.gmail.com Reviewed-by: Peter Maydell [PMM: added pseudocode ref to the commit message, fixed comment style] Signed-off-by: Peter Maydell --- target/arm/helper.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index e24689f767..676059cb38 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -11078,18 +11078,19 @@ static void v8m_security_lookup(CPUARMState *env, uint32_t address, } } } - - /* The IDAU will override the SAU lookup results if it specifies - * higher security than the SAU does. - */ - if (!idau_ns) { - if (sattrs->ns || (!idau_nsc && sattrs->nsc)) { - sattrs->ns = false; - sattrs->nsc = idau_nsc; - } - } break; } + + /* + * The IDAU will override the SAU lookup results if it specifies + * higher security than the SAU does. + */ + if (!idau_ns) { + if (sattrs->ns || (!idau_nsc && sattrs->nsc)) { + sattrs->ns = false; + sattrs->nsc = idau_nsc; + } + } } static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, From ab65eed3f82b57459ef8c45f75a89241f16cbad8 Mon Sep 17 00:00:00 2001 From: Luc Michel Date: Tue, 29 Jan 2019 11:46:03 +0000 Subject: [PATCH 03/23] gdbstub: fix gdb_get_cpu(s, pid, tid) when pid and/or tid are 0 a TID or PID value means "any thread" (resp. "any process"). This commit fixes the different combinations when at least one value is 0. When both are 0, the function now returns the first attached CPU, instead of the CPU with TID 1, which is not necessarily attached or even existent. When PID is specified but TID is 0, the function returns the first CPU in the process, or NULL if the process does not exist or is not attached. In other cases, it returns the corresponding CPU, while ignoring the PID check when PID is 0. Reported-by: Peter Maydell Signed-off-by: Luc Michel Reviewed-by: Peter Maydell Message-id: 20190119140000.11767-1-luc.michel@greensocs.com Signed-off-by: Peter Maydell --- gdbstub.c | 72 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index bfc7afb509..d4cc6ecf99 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -756,35 +756,6 @@ static CPUState *gdb_next_cpu_in_process(const GDBState *s, CPUState *cpu) return cpu; } -static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) -{ - GDBProcess *process; - CPUState *cpu; - - if (!tid) { - /* 0 means any thread, we take the first one */ - tid = 1; - } - - cpu = find_cpu(tid); - - if (cpu == NULL) { - return NULL; - } - - process = gdb_get_cpu_process(s, cpu); - - if (process->pid != pid) { - return NULL; - } - - if (!process->attached) { - return NULL; - } - - return cpu; -} - /* Return the cpu following @cpu, while ignoring unattached processes. */ static CPUState *gdb_next_attached_cpu(const GDBState *s, CPUState *cpu) { @@ -814,6 +785,49 @@ static CPUState *gdb_first_attached_cpu(const GDBState *s) return cpu; } +static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) +{ + GDBProcess *process; + CPUState *cpu; + + if (!pid && !tid) { + /* 0 means any process/thread, we take the first attached one */ + return gdb_first_attached_cpu(s); + } else if (pid && !tid) { + /* any thread in a specific process */ + process = gdb_get_process(s, pid); + + if (process == NULL) { + return NULL; + } + + if (!process->attached) { + return NULL; + } + + return get_first_cpu_in_process(s, process); + } else { + /* a specific thread */ + cpu = find_cpu(tid); + + if (cpu == NULL) { + return NULL; + } + + process = gdb_get_cpu_process(s, cpu); + + if (pid && process->pid != pid) { + return NULL; + } + + if (!process->attached) { + return NULL; + } + + return cpu; + } +} + static const char *get_feature_xml(const GDBState *s, const char *p, const char **newp, GDBProcess *process) { From 9d68bf564ecd038c8939b9779f4b6d62a01ce6f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steffen=20G=C3=B6rtz?= Date: Tue, 29 Jan 2019 11:46:03 +0000 Subject: [PATCH 04/23] arm: Stub out NRF51 TWI magnetometer/accelerometer detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recent microbit firmwares panic if the TWI magnetometer/accelerometer devices are not detected during startup. We don't implement TWI (I2C) so let's stub out these devices just to let the firmware boot. Signed-off by: Steffen Görtz Signed-off-by: Stefan Hajnoczi Message-id: 20190110094020.18354-2-stefanha@redhat.com Reviewed-by: Peter Maydell [PMM: fixed comment style] Signed-off-by: Peter Maydell --- hw/arm/microbit.c | 16 +++++ hw/i2c/Makefile.objs | 1 + hw/i2c/microbit_i2c.c | 127 ++++++++++++++++++++++++++++++++++ include/hw/arm/nrf51.h | 2 + include/hw/arm/nrf51_soc.h | 1 + include/hw/i2c/microbit_i2c.h | 42 +++++++++++ 6 files changed, 189 insertions(+) create mode 100644 hw/i2c/microbit_i2c.c create mode 100644 include/hw/i2c/microbit_i2c.h diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c index a734e7f650..da67bf6d9d 100644 --- a/hw/arm/microbit.c +++ b/hw/arm/microbit.c @@ -16,11 +16,13 @@ #include "exec/address-spaces.h" #include "hw/arm/nrf51_soc.h" +#include "hw/i2c/microbit_i2c.h" typedef struct { MachineState parent; NRF51State nrf51; + MicrobitI2CState i2c; } MicrobitMachineState; #define TYPE_MICROBIT_MACHINE MACHINE_TYPE_NAME("microbit") @@ -32,7 +34,9 @@ static void microbit_init(MachineState *machine) { MicrobitMachineState *s = MICROBIT_MACHINE(machine); MemoryRegion *system_memory = get_system_memory(); + MemoryRegion *mr; Object *soc = OBJECT(&s->nrf51); + Object *i2c = OBJECT(&s->i2c); sysbus_init_child_obj(OBJECT(machine), "nrf51", soc, sizeof(s->nrf51), TYPE_NRF51_SOC); @@ -41,6 +45,18 @@ static void microbit_init(MachineState *machine) &error_fatal); object_property_set_bool(soc, true, "realized", &error_fatal); + /* + * Overlap the TWI stub device into the SoC. This is a microbit-specific + * hack until we implement the nRF51 TWI controller properly and the + * magnetometer/accelerometer devices. + */ + sysbus_init_child_obj(OBJECT(machine), "microbit.twi", i2c, + sizeof(s->i2c), TYPE_MICROBIT_I2C); + object_property_set_bool(i2c, true, "realized", &error_fatal); + mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(i2c), 0); + memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE, + mr, -1); + armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, NRF51_SOC(soc)->flash_size); } diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs index 37cacde978..82e747e1cd 100644 --- a/hw/i2c/Makefile.objs +++ b/hw/i2c/Makefile.objs @@ -7,5 +7,6 @@ common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o +common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o obj-$(CONFIG_OMAP) += omap_i2c.o obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c new file mode 100644 index 0000000000..793f1b0f8b --- /dev/null +++ b/hw/i2c/microbit_i2c.c @@ -0,0 +1,127 @@ +/* + * Microbit stub for Nordic Semiconductor nRF51 SoC Two-Wire Interface + * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf + * + * This is a microbit-specific stub for the TWI controller on the nRF51 SoC. + * We don't emulate I2C devices but the firmware probes the + * accelerometer/magnetometer on startup and panics if they are not found. + * Therefore we stub out the probing. + * + * In the future this file could evolve into a full nRF51 TWI controller + * device. + * + * Copyright 2018 Steffen Görtz + * Copyright 2019 Red Hat, Inc. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qemu/log.h" +#include "hw/i2c/microbit_i2c.h" + +static const uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40}; + +static uint64_t microbit_i2c_read(void *opaque, hwaddr addr, unsigned int size) +{ + MicrobitI2CState *s = opaque; + uint64_t data = 0x00; + + switch (addr) { + case NRF51_TWI_EVENT_STOPPED: + data = 0x01; + break; + case NRF51_TWI_EVENT_RXDREADY: + data = 0x01; + break; + case NRF51_TWI_EVENT_TXDSENT: + data = 0x01; + break; + case NRF51_TWI_REG_RXD: + data = twi_read_sequence[s->read_idx]; + if (s->read_idx < G_N_ELEMENTS(twi_read_sequence)) { + s->read_idx++; + } + break; + default: + data = s->regs[addr / sizeof(s->regs[0])]; + break; + } + + qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n", + __func__, addr, size, (uint32_t)data); + + + return data; +} + +static void microbit_i2c_write(void *opaque, hwaddr addr, uint64_t data, + unsigned int size) +{ + MicrobitI2CState *s = opaque; + + qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n", + __func__, addr, data, size); + s->regs[addr / sizeof(s->regs[0])] = data; +} + +static const MemoryRegionOps microbit_i2c_ops = { + .read = microbit_i2c_read, + .write = microbit_i2c_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl.min_access_size = 4, + .impl.max_access_size = 4, +}; + +static const VMStateDescription microbit_i2c_vmstate = { + .name = TYPE_MICROBIT_I2C, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS), + VMSTATE_UINT32(read_idx, MicrobitI2CState), + }, +}; + +static void microbit_i2c_reset(DeviceState *dev) +{ + MicrobitI2CState *s = MICROBIT_I2C(dev); + + memset(s->regs, 0, sizeof(s->regs)); + s->read_idx = 0; +} + +static void microbit_i2c_realize(DeviceState *dev, Error **errp) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + MicrobitI2CState *s = MICROBIT_I2C(dev); + + memory_region_init_io(&s->iomem, OBJECT(s), µbit_i2c_ops, s, + "microbit.twi", NRF51_TWI_SIZE); + sysbus_init_mmio(sbd, &s->iomem); +} + +static void microbit_i2c_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = µbit_i2c_vmstate; + dc->reset = microbit_i2c_reset; + dc->realize = microbit_i2c_realize; + dc->desc = "Microbit I2C controller"; +} + +static const TypeInfo microbit_i2c_info = { + .name = TYPE_MICROBIT_I2C, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(MicrobitI2CState), + .class_init = microbit_i2c_class_init, +}; + +static void microbit_i2c_register_types(void) +{ + type_register_static(µbit_i2c_info); +} + +type_init(microbit_i2c_register_types) diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h index 175bb6c301..1008fee6c9 100644 --- a/include/hw/arm/nrf51.h +++ b/include/hw/arm/nrf51.h @@ -25,6 +25,8 @@ #define NRF51_IOMEM_SIZE 0x20000000 #define NRF51_UART_BASE 0x40002000 +#define NRF51_TWI_BASE 0x40003000 +#define NRF51_TWI_SIZE 0x00001000 #define NRF51_TIMER_BASE 0x40008000 #define NRF51_TIMER_SIZE 0x00001000 #define NRF51_RNG_BASE 0x4000D000 diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h index e06f0304b4..fbdefc07e4 100644 --- a/include/hw/arm/nrf51_soc.h +++ b/include/hw/arm/nrf51_soc.h @@ -39,6 +39,7 @@ typedef struct NRF51State { MemoryRegion sram; MemoryRegion flash; MemoryRegion clock; + MemoryRegion twi; uint32_t sram_size; uint32_t flash_size; diff --git a/include/hw/i2c/microbit_i2c.h b/include/hw/i2c/microbit_i2c.h new file mode 100644 index 0000000000..aad636127e --- /dev/null +++ b/include/hw/i2c/microbit_i2c.h @@ -0,0 +1,42 @@ +/* + * Microbit stub for Nordic Semiconductor nRF51 SoC Two-Wire Interface + * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf + * + * Copyright 2019 Red Hat, Inc. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ + +#ifndef MICROBIT_I2C_H +#define MICROBIT_I2C_H + +#include "hw/sysbus.h" +#include "hw/arm/nrf51.h" + +#define NRF51_TWI_TASK_STARTRX 0x000 +#define NRF51_TWI_TASK_STARTTX 0x008 +#define NRF51_TWI_TASK_STOP 0x014 +#define NRF51_TWI_EVENT_STOPPED 0x104 +#define NRF51_TWI_EVENT_RXDREADY 0x108 +#define NRF51_TWI_EVENT_TXDSENT 0x11c +#define NRF51_TWI_REG_ENABLE 0x500 +#define NRF51_TWI_REG_RXD 0x518 +#define NRF51_TWI_REG_TXD 0x51c +#define NRF51_TWI_REG_ADDRESS 0x588 + +#define TYPE_MICROBIT_I2C "microbit.i2c" +#define MICROBIT_I2C(obj) \ + OBJECT_CHECK(MicrobitI2CState, (obj), TYPE_MICROBIT_I2C) + +#define MICROBIT_I2C_NREGS (NRF51_TWI_SIZE / sizeof(uint32_t)) + +typedef struct { + SysBusDevice parent_obj; + + MemoryRegion iomem; + uint32_t regs[MICROBIT_I2C_NREGS]; + uint32_t read_idx; +} MicrobitI2CState; + +#endif /* MICROBIT_I2C_H */ From b36356f69941bcda47ac3d97c2b05757728575c9 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 29 Jan 2019 11:46:03 +0000 Subject: [PATCH 05/23] tests/microbit-test: add TWI stub device test This test verifies that we read back the expected I2C WHO_AM_I register values for the accelerometer/magnetometer. Signed-off-by: Stefan Hajnoczi Message-id: 20190110094020.18354-3-stefanha@redhat.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- tests/microbit-test.c | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/microbit-test.c b/tests/microbit-test.c index 0c125535f6..dcdc0cd41a 100644 --- a/tests/microbit-test.c +++ b/tests/microbit-test.c @@ -21,6 +21,49 @@ #include "hw/arm/nrf51.h" #include "hw/gpio/nrf51_gpio.h" #include "hw/timer/nrf51_timer.h" +#include "hw/i2c/microbit_i2c.h" + +/* Read a byte from I2C device at @addr from register @reg */ +static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg) +{ + uint32_t val; + + writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr); + writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1); + writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg); + val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT); + g_assert_cmpuint(val, ==, 1); + writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); + + writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1); + val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY); + g_assert_cmpuint(val, ==, 1); + val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD); + writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); + + return val; +} + +static void test_microbit_i2c(void) +{ + uint32_t val; + + /* We don't program pins/irqs but at least enable the device */ + writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5); + + /* MMA8653 magnetometer detection */ + val = i2c_read_byte(0x3A, 0x0D); + g_assert_cmpuint(val, ==, 0x5A); + + val = i2c_read_byte(0x3A, 0x0D); + g_assert_cmpuint(val, ==, 0x5A); + + /* LSM303 accelerometer detection */ + val = i2c_read_byte(0x3C, 0x4F); + g_assert_cmpuint(val, ==, 0x40); + + writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0); +} static void test_nrf51_gpio(void) { @@ -247,6 +290,7 @@ int main(int argc, char **argv) qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); + qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c); ret = g_test_run(); From ea7a5330b79523540ba776c529b09dc8cf3fa0c5 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 06/23] exec.c: Use correct attrs in cpu_memory_rw_debug() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the softmmu version of cpu_memory_rw_debug(), we ask the CPU for the attributes to use for the virtual memory access, and we correctly use those to identify the address space index. However, we were not passing them in to the address_space_write_rom() and address_space_rw() functions. The effect of this was that a memory access from the gdbstub to a device which had behaviour that was sensitive to the memory attributes (such as some ARMv8M NVIC registers) was incorrectly always performed as if non-secure, rather than using the right security state for the CPU's current state. Fixes: https://bugs.launchpad.net/qemu/+bug/1812091 Signed-off-by: Peter Maydell Reviewed-by: Stefano Garzarella Reviewed-by: Philippe Mathieu-Daudé Message-id: 20190117133834.7480-1-peter.maydell@linaro.org --- exec.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/exec.c b/exec.c index 895449f926..9557a4e523 100644 --- a/exec.c +++ b/exec.c @@ -3882,12 +3882,10 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, phys_addr += (addr & ~TARGET_PAGE_MASK); if (is_write) { address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr, - MEMTXATTRS_UNSPECIFIED, - buf, l); + attrs, buf, l); } else { address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, - MEMTXATTRS_UNSPECIFIED, - buf, l, 0); + attrs, buf, l, 0); } len -= l; buf += l; From f454a54f3bf95920d236ad893344141085db061a Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 07/23] accel/tcg/user-exec: Don't parse aarch64 insns to test for read vs write In cpu_signal_handler() for aarch64 hosts, currently we parse the faulting instruction to see if it is a load or a store. Since the 3.16 kernel (~2014), the kernel has provided us with the syndrome register for a fault, which includes the WnR bit. Use this instead if it is present, only falling back to instruction parsing if not. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20190108180014.32386-1-peter.maydell@linaro.org --- accel/tcg/user-exec.c | 66 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index 941295ea49..66cc818e3f 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -479,28 +479,66 @@ int cpu_signal_handler(int host_signum, void *pinfo, #elif defined(__aarch64__) +#ifndef ESR_MAGIC +/* Pre-3.16 kernel headers don't have these, so provide fallback definitions */ +#define ESR_MAGIC 0x45535201 +struct esr_context { + struct _aarch64_ctx head; + uint64_t esr; +}; +#endif + +static inline struct _aarch64_ctx *first_ctx(ucontext_t *uc) +{ + return (struct _aarch64_ctx *)&uc->uc_mcontext.__reserved; +} + +static inline struct _aarch64_ctx *next_ctx(struct _aarch64_ctx *hdr) +{ + return (struct _aarch64_ctx *)((char *)hdr + hdr->size); +} + int cpu_signal_handler(int host_signum, void *pinfo, void *puc) { siginfo_t *info = pinfo; ucontext_t *uc = puc; uintptr_t pc = uc->uc_mcontext.pc; - uint32_t insn = *(uint32_t *)pc; bool is_write; + struct _aarch64_ctx *hdr; + struct esr_context const *esrctx = NULL; - /* XXX: need kernel patch to get write flag faster. */ - is_write = ( (insn & 0xbfff0000) == 0x0c000000 /* C3.3.1 */ - || (insn & 0xbfe00000) == 0x0c800000 /* C3.3.2 */ - || (insn & 0xbfdf0000) == 0x0d000000 /* C3.3.3 */ - || (insn & 0xbfc00000) == 0x0d800000 /* C3.3.4 */ - || (insn & 0x3f400000) == 0x08000000 /* C3.3.6 */ - || (insn & 0x3bc00000) == 0x39000000 /* C3.3.13 */ - || (insn & 0x3fc00000) == 0x3d800000 /* ... 128bit */ - /* Ingore bits 10, 11 & 21, controlling indexing. */ - || (insn & 0x3bc00000) == 0x38000000 /* C3.3.8-12 */ - || (insn & 0x3fe00000) == 0x3c800000 /* ... 128bit */ - /* Ignore bits 23 & 24, controlling indexing. */ - || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */ + /* Find the esr_context, which has the WnR bit in it */ + for (hdr = first_ctx(uc); hdr->magic; hdr = next_ctx(hdr)) { + if (hdr->magic == ESR_MAGIC) { + esrctx = (struct esr_context const *)hdr; + break; + } + } + if (esrctx) { + /* For data aborts ESR.EC is 0b10010x: then bit 6 is the WnR bit */ + uint64_t esr = esrctx->esr; + is_write = extract32(esr, 27, 5) == 0x12 && extract32(esr, 6, 1) == 1; + } else { + /* + * Fall back to parsing instructions; will only be needed + * for really ancient (pre-3.16) kernels. + */ + uint32_t insn = *(uint32_t *)pc; + + is_write = ((insn & 0xbfff0000) == 0x0c000000 /* C3.3.1 */ + || (insn & 0xbfe00000) == 0x0c800000 /* C3.3.2 */ + || (insn & 0xbfdf0000) == 0x0d000000 /* C3.3.3 */ + || (insn & 0xbfc00000) == 0x0d800000 /* C3.3.4 */ + || (insn & 0x3f400000) == 0x08000000 /* C3.3.6 */ + || (insn & 0x3bc00000) == 0x39000000 /* C3.3.13 */ + || (insn & 0x3fc00000) == 0x3d800000 /* ... 128bit */ + /* Ignore bits 10, 11 & 21, controlling indexing. */ + || (insn & 0x3bc00000) == 0x38000000 /* C3.3.8-12 */ + || (insn & 0x3fe00000) == 0x3c800000 /* ... 128bit */ + /* Ignore bits 23 & 24, controlling indexing. */ + || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */ + } return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } From c8de3f5fd696db0e0a16062de4b9c75252401e4e Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 08/23] MAINTAINERS: update microbit ARM board files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New source files were added without corresponding ./MAINTAINERS file entries. Let's get things up to date. Reviewed-by: Thomas Huth Signed-off-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Message-id: 20190123183352.11025-1-stefanha@redhat.com Signed-off-by: Peter Maydell --- MAINTAINERS | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 59e1f24d68..b334b53979 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -829,9 +829,11 @@ M: Joel Stanley M: Peter Maydell L: qemu-arm@nongnu.org S: Maintained -F: hw/arm/nrf51_soc.c -F: hw/arm/microbit.c -F: include/hw/arm/nrf51_soc.h +F: hw/*/nrf51*.c +F: hw/*/microbit*.c +F: include/hw/*/nrf51*.h +F: include/hw/*/microbit*.h +F: tests/microbit-test.c CRIS Machines ------------- From bf8d09694ccc07487cd73d7562081fdaec3370c8 Mon Sep 17 00:00:00 2001 From: Aaron Lindsay OS Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 09/23] target/arm: Don't clear supported PMU events when initializing PMCEID1 A bug was introduced during a respin of: commit 57a4a11b2b281bb548b419ca81bfafb214e4c77a target/arm: Add array for supported PMU events, generate PMCEID[01]_EL0 This patch introduced two calls to get_pmceid() during CPU initialization - one each for PMCEID0 and PMCEID1. In addition to building the register values, get_pmceid() clears an internal array mapping event numbers to their implementations (supported_event_map) before rebuilding it. This is an optimization since much of the logic is shared. However, since it was called twice, the contents of supported_event_map reflect only the events in PMCEID1 (the second call to get_pmceid()). Fix this bug by moving the initialization of PMCEID0 and PMCEID1 back into a single function call, and name it more appropriately since it is doing more than simply generating the contents of the PMCEID[01] registers. Signed-off-by: Aaron Lindsay Reviewed-by: Richard Henderson Message-id: 20190123195814.29253-1-aaron@os.amperecomputing.com Signed-off-by: Peter Maydell --- target/arm/cpu.c | 3 +-- target/arm/cpu.h | 11 +++++------ target/arm/helper.c | 27 ++++++++++++++++----------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 7e1f3dd637..d6da3f4fed 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1039,8 +1039,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) unset_feature(env, ARM_FEATURE_PMU); } if (arm_feature(env, ARM_FEATURE_PMU)) { - cpu->pmceid0 = get_pmceid(&cpu->env, 0); - cpu->pmceid1 = get_pmceid(&cpu->env, 1); + pmu_init(cpu); if (!kvm_enabled()) { arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); diff --git a/target/arm/cpu.h b/target/arm/cpu.h index ff81db420d..b8161cb6d7 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1012,14 +1012,13 @@ void pmu_pre_el_change(ARMCPU *cpu, void *ignored); void pmu_post_el_change(ARMCPU *cpu, void *ignored); /* - * get_pmceid - * @env: CPUARMState - * @which: which PMCEID register to return (0 or 1) + * pmu_init + * @cpu: ARMCPU * - * Return the PMCEID[01]_EL0 register values corresponding to the counters - * which are supported given the current configuration + * Initialize the CPU's PMCEID[01]_EL0 registers and associated internal state + * for the current configuration */ -uint64_t get_pmceid(CPUARMState *env, unsigned which); +void pmu_init(ARMCPU *cpu); /* SCTLR bit meanings. Several bits have been reused in newer * versions of the architecture; in that case we define constants diff --git a/target/arm/helper.c b/target/arm/helper.c index 676059cb38..66faebea8e 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -1090,22 +1090,24 @@ static const pm_event pm_events[] = { static uint16_t supported_event_map[MAX_EVENT_ID + 1]; /* - * Called upon initialization to build PMCEID0_EL0 or PMCEID1_EL0 (indicated by - * 'which'). We also use it to build a map of ARM event numbers to indices in - * our pm_events array. + * Called upon CPU initialization to initialize PMCEID[01]_EL0 and build a map + * of ARM event numbers to indices in our pm_events array. * * Note: Events in the 0x40XX range are not currently supported. */ -uint64_t get_pmceid(CPUARMState *env, unsigned which) +void pmu_init(ARMCPU *cpu) { - uint64_t pmceid = 0; unsigned int i; - assert(which <= 1); - + /* + * Empty supported_event_map and cpu->pmceid[01] before adding supported + * events to them + */ for (i = 0; i < ARRAY_SIZE(supported_event_map); i++) { supported_event_map[i] = UNSUPPORTED_EVENT; } + cpu->pmceid0 = 0; + cpu->pmceid1 = 0; for (i = 0; i < ARRAY_SIZE(pm_events); i++) { const pm_event *cnt = &pm_events[i]; @@ -1113,13 +1115,16 @@ uint64_t get_pmceid(CPUARMState *env, unsigned which) /* We do not currently support events in the 0x40xx range */ assert(cnt->number <= 0x3f); - if ((cnt->number & 0x20) == (which << 6) && - cnt->supported(env)) { - pmceid |= (1 << (cnt->number & 0x1f)); + if (cnt->supported(&cpu->env)) { supported_event_map[cnt->number] = i; + uint64_t event_mask = 1 << (cnt->number & 0x1f); + if (cnt->number & 0x20) { + cpu->pmceid1 |= event_mask; + } else { + cpu->pmceid0 |= event_mask; + } } } - return pmceid; } /* From 047be4ed24b3a408acccf9316d619477c06cca42 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 10/23] memory: add memory_region_flush_rom_device() ROM devices go via MemoryRegionOps->write() callbacks for write operations and do not dirty/invalidate that memory. Device emulation must be able to mark memory ranges that have been modified internally (e.g. using memory_region_get_ram_ptr()). Introduce the memory_region_flush_rom_device() API for this purpose. Signed-off-by: Stefan Hajnoczi Message-id: 20190123212234.32068-2-stefanha@redhat.com Reviewed-by: Peter Maydell [PMM: fix block comment style] Signed-off-by: Peter Maydell --- exec.c | 13 +++++++++++++ include/exec/memory.h | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/exec.c b/exec.c index 9557a4e523..da3e635f91 100644 --- a/exec.c +++ b/exec.c @@ -3162,6 +3162,19 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr, cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask); } +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size) +{ + /* + * In principle this function would work on other memory region types too, + * but the ROM device use case is the only one where this operation is + * necessary. Other memory regions should use the + * address_space_read/write() APIs. + */ + assert(memory_region_is_romd(mr)); + + invalidate_and_set_dirty(mr, addr, size); +} + static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) { unsigned access_size_max = mr->ops->valid.max_access_size; diff --git a/include/exec/memory.h b/include/exec/memory.h index cd2f209b64..abe9cc79c0 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1344,6 +1344,24 @@ bool memory_region_snapshot_get_dirty(MemoryRegion *mr, void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr, hwaddr size, unsigned client); +/** + * memory_region_flush_rom_device: Mark a range of pages dirty and invalidate + * TBs (for self-modifying code). + * + * The MemoryRegionOps->write() callback of a ROM device must use this function + * to mark byte ranges that have been modified internally, such as by directly + * accessing the memory returned by memory_region_get_ram_ptr(). + * + * This function marks the range dirty and invalidates TBs so that TCG can + * detect self-modifying code. + * + * @mr: the region being flushed. + * @addr: the start, relative to the start of the region, of the range being + * flushed. + * @size: the size, in bytes, of the range being flushed. + */ +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size); + /** * memory_region_set_readonly: Turn a memory region read-only (or read-write) * From 6c90a82c9358c77017b94d67c1a3fa24e0500e07 Mon Sep 17 00:00:00 2001 From: Julia Suvorova Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 11/23] tests/libqtest: Introduce qtest_init_with_serial() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run qtest with a socket that connects QEMU chardev and test code. Signed-off-by: Julia Suvorova Reviewed-by: Stefan Hajnoczi Reviewed-by: Thomas Huth Reviewed-by: Alex Bennée Message-id: 20190123120759.7162-2-jusual@mail.ru Signed-off-by: Peter Maydell --- tests/libqtest.c | 25 +++++++++++++++++++++++++ tests/libqtest.h | 11 +++++++++++ 2 files changed, 36 insertions(+) diff --git a/tests/libqtest.c b/tests/libqtest.c index 55750dd68d..6fb30855fa 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -315,6 +315,31 @@ QTestState *qtest_initf(const char *fmt, ...) return s; } +QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd) +{ + int sock_fd_init; + char *sock_path, sock_dir[] = "/tmp/qtest-serial-XXXXXX"; + QTestState *qts; + + g_assert_true(mkdtemp(sock_dir) != NULL); + sock_path = g_strdup_printf("%s/sock", sock_dir); + + sock_fd_init = init_socket(sock_path); + + qts = qtest_initf("-chardev socket,id=s0,path=%s -serial chardev:s0 %s", + sock_path, extra_args); + + *sock_fd = socket_accept(sock_fd_init); + + unlink(sock_path); + g_free(sock_path); + rmdir(sock_dir); + + g_assert_true(*sock_fd >= 0); + + return qts; +} + void qtest_quit(QTestState *s) { g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s)); diff --git a/tests/libqtest.h b/tests/libqtest.h index 7ea94139b0..5937f91912 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -62,6 +62,17 @@ QTestState *qtest_init(const char *extra_args); */ QTestState *qtest_init_without_qmp_handshake(const char *extra_args); +/** + * qtest_init_with_serial: + * @extra_args: other arguments to pass to QEMU. CAUTION: these + * arguments are subject to word splitting and shell evaluation. + * @sock_fd: pointer to store the socket file descriptor for + * connection with serial. + * + * Returns: #QTestState instance. + */ +QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd); + /** * qtest_quit: * @s: #QTestState instance to operate on. From 7cf19e7352fc213011d5a051535b28ec696e02d1 Mon Sep 17 00:00:00 2001 From: Julia Suvorova Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 12/23] tests/microbit-test: Make test independent of global_qtest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using of global_qtest is not required here. Let's replace functions like readl() with the corresponding qtest_* counterparts. Signed-off-by: Julia Suvorova Reviewed-by: Philippe Mathieu-Daudé Acked-by: Thomas Huth Reviewed-by: Stefan Hajnoczi Message-id: 20190123120759.7162-3-jusual@mail.ru Signed-off-by: Peter Maydell --- tests/microbit-test.c | 247 ++++++++++++++++++++++-------------------- 1 file changed, 129 insertions(+), 118 deletions(-) diff --git a/tests/microbit-test.c b/tests/microbit-test.c index dcdc0cd41a..afeb6b082a 100644 --- a/tests/microbit-test.c +++ b/tests/microbit-test.c @@ -24,22 +24,22 @@ #include "hw/i2c/microbit_i2c.h" /* Read a byte from I2C device at @addr from register @reg */ -static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg) +static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg) { uint32_t val; - writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr); - writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1); - writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg); - val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg); + val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT); g_assert_cmpuint(val, ==, 1); - writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); - writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1); - val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1); + val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY); g_assert_cmpuint(val, ==, 1); - val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD); - writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); + val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_REG_RXD); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1); return val; } @@ -47,22 +47,25 @@ static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg) static void test_microbit_i2c(void) { uint32_t val; + QTestState *qts = qtest_init("-M microbit"); /* We don't program pins/irqs but at least enable the device */ - writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5); /* MMA8653 magnetometer detection */ - val = i2c_read_byte(0x3A, 0x0D); + val = i2c_read_byte(qts, 0x3A, 0x0D); g_assert_cmpuint(val, ==, 0x5A); - val = i2c_read_byte(0x3A, 0x0D); + val = i2c_read_byte(qts, 0x3A, 0x0D); g_assert_cmpuint(val, ==, 0x5A); /* LSM303 accelerometer detection */ - val = i2c_read_byte(0x3C, 0x4F); + val = i2c_read_byte(qts, 0x3C, 0x4F); g_assert_cmpuint(val, ==, 0x40); - writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0); + qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0); + + qtest_quit(qts); } static void test_nrf51_gpio(void) @@ -80,220 +83,228 @@ static void test_nrf51_gpio(void) {NRF51_GPIO_REG_DIRCLR, 0x00000000} }; + QTestState *qts = qtest_init("-M microbit"); + /* Check reset state */ for (i = 0; i < ARRAY_SIZE(reset_state); i++) { expected = reset_state[i].expected; - actual = readl(NRF51_GPIO_BASE + reset_state[i].addr); + actual = qtest_readl(qts, NRF51_GPIO_BASE + reset_state[i].addr); g_assert_cmpuint(actual, ==, expected); } for (i = 0; i < NRF51_GPIO_PINS; i++) { expected = 0x00000002; - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4); + actual = qtest_readl(qts, NRF51_GPIO_BASE + + NRF51_GPIO_REG_CNF_START + i * 4); g_assert_cmpuint(actual, ==, expected); } /* Check dir bit consistency between dir and cnf */ /* Check set via DIRSET */ expected = 0x80000001; - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); g_assert_cmpuint(actual, ==, expected); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) + & 0x01; g_assert_cmpuint(actual, ==, 0x01); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; g_assert_cmpuint(actual, ==, 0x01); /* Check clear via DIRCLR */ - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); g_assert_cmpuint(actual, ==, 0x00000000); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) + & 0x01; g_assert_cmpuint(actual, ==, 0x00); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; g_assert_cmpuint(actual, ==, 0x00); /* Check set via DIR */ expected = 0x80000001; - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR); g_assert_cmpuint(actual, ==, expected); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) + & 0x01; g_assert_cmpuint(actual, ==, 0x01); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01; g_assert_cmpuint(actual, ==, 0x01); /* Reset DIR */ - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000); /* Check Input propagates */ - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00); - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00); + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x00); - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x01); - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); /* Check pull-up working */ - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x00); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); /* Check pull-down working */ - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x00); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02); + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1); /* Check Output propagates */ - irq_intercept_out("/machine/nrf51"); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); - g_assert_true(get_irq(0)); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01); - g_assert_false(get_irq(0)); + qtest_irq_intercept_out(qts, "/machine/nrf51"); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); + g_assert_true(qtest_get_irq(qts, 0)); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01); + g_assert_false(qtest_get_irq(qts, 0)); /* Check self-stimulation */ - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01); - actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01; g_assert_cmpuint(actual, ==, 0x00); /* * Check short-circuit - generates an guest_error which must be checked * manually as long as qtest can not scan qemu_log messages */ - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01); - writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); - qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01); + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01); + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0); + + qtest_quit(qts); } -static void timer_task(hwaddr task) +static void timer_task(QTestState *qts, hwaddr task) { - writel(NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK); + qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK); } -static void timer_clear_event(hwaddr event) +static void timer_clear_event(QTestState *qts, hwaddr event) { - writel(NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR); + qtest_writel(qts, NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR); } -static void timer_set_bitmode(uint8_t mode) +static void timer_set_bitmode(QTestState *qts, uint8_t mode) { - writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode); + qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode); } -static void timer_set_prescaler(uint8_t prescaler) +static void timer_set_prescaler(QTestState *qts, uint8_t prescaler) { - writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler); + qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler); } -static void timer_set_cc(size_t idx, uint32_t value) +static void timer_set_cc(QTestState *qts, size_t idx, uint32_t value) { - writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value); + qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value); } -static void timer_assert_events(uint32_t ev0, uint32_t ev1, uint32_t ev2, - uint32_t ev3) +static void timer_assert_events(QTestState *qts, uint32_t ev0, uint32_t ev1, + uint32_t ev2, uint32_t ev3) { - g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0) == ev0); - g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1) == ev1); - g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2) == ev2); - g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3) == ev3); + g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0) + == ev0); + g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1) + == ev1); + g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2) + == ev2); + g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3) + == ev3); } static void test_nrf51_timer(void) { uint32_t steps_to_overflow = 408; + QTestState *qts = qtest_init("-M microbit"); /* Compare Match */ - timer_task(NRF51_TIMER_TASK_STOP); - timer_task(NRF51_TIMER_TASK_CLEAR); + timer_task(qts, NRF51_TIMER_TASK_STOP); + timer_task(qts, NRF51_TIMER_TASK_CLEAR); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3); - timer_set_bitmode(NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */ - timer_set_prescaler(0); + timer_set_bitmode(qts, NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */ + timer_set_prescaler(qts, 0); /* Swept over in first step */ - timer_set_cc(0, 2); + timer_set_cc(qts, 0, 2); /* Barely miss on first step */ - timer_set_cc(1, 162); + timer_set_cc(qts, 1, 162); /* Spot on on third step */ - timer_set_cc(2, 480); + timer_set_cc(qts, 2, 480); - timer_assert_events(0, 0, 0, 0); + timer_assert_events(qts, 0, 0, 0, 0); - timer_task(NRF51_TIMER_TASK_START); - clock_step(10000); - timer_assert_events(1, 0, 0, 0); + timer_task(qts, NRF51_TIMER_TASK_START); + qtest_clock_step(qts, 10000); + timer_assert_events(qts, 1, 0, 0, 0); /* Swept over on first overflow */ - timer_set_cc(3, 114); + timer_set_cc(qts, 3, 114); - clock_step(10000); - timer_assert_events(1, 1, 0, 0); + qtest_clock_step(qts, 10000); + timer_assert_events(qts, 1, 1, 0, 0); - clock_step(10000); - timer_assert_events(1, 1, 1, 0); + qtest_clock_step(qts, 10000); + timer_assert_events(qts, 1, 1, 1, 0); /* Wrap time until internal counter overflows */ while (steps_to_overflow--) { - timer_assert_events(1, 1, 1, 0); - clock_step(10000); + timer_assert_events(qts, 1, 1, 1, 0); + qtest_clock_step(qts, 10000); } - timer_assert_events(1, 1, 1, 1); + timer_assert_events(qts, 1, 1, 1, 1); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2); - timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3); - timer_assert_events(0, 0, 0, 0); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2); + timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3); + timer_assert_events(qts, 0, 0, 0, 0); - timer_task(NRF51_TIMER_TASK_STOP); + timer_task(qts, NRF51_TIMER_TASK_STOP); /* Test Proposal: Stop/Shutdown */ /* Test Proposal: Shortcut Compare -> Clear */ /* Test Proposal: Shortcut Compare -> Stop */ /* Test Proposal: Counter Mode */ + + qtest_quit(qts); } int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); - global_qtest = qtest_initf("-machine microbit"); - qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c); - ret = g_test_run(); - - qtest_quit(global_qtest); - return ret; + return g_test_run(); } From 46a6603ba6ea545b92c9e53b19998ade97ed8b05 Mon Sep 17 00:00:00 2001 From: Julia Suvorova Date: Tue, 29 Jan 2019 11:46:04 +0000 Subject: [PATCH 13/23] tests/microbit-test: Check nRF51 UART functionality Some functional tests for: Basic reception/transmittion Suspending INTEN* registers Signed-off-by: Julia Suvorova Reviewed-by: Stefan Hajnoczi Acked-by: Thomas Huth Message-id: 20190123120759.7162-4-jusual@mail.ru Signed-off-by: Peter Maydell --- tests/microbit-test.c | 89 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/tests/microbit-test.c b/tests/microbit-test.c index afeb6b082a..3bad947b6c 100644 --- a/tests/microbit-test.c +++ b/tests/microbit-test.c @@ -19,10 +19,98 @@ #include "libqtest.h" #include "hw/arm/nrf51.h" +#include "hw/char/nrf51_uart.h" #include "hw/gpio/nrf51_gpio.h" #include "hw/timer/nrf51_timer.h" #include "hw/i2c/microbit_i2c.h" +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr) +{ + time_t now, start = time(NULL); + + while (true) { + if (qtest_readl(qts, event_addr) == 1) { + qtest_writel(qts, event_addr, 0x00); + return true; + } + + /* Wait at most 10 minutes */ + now = time(NULL); + if (now - start > 600) { + break; + } + g_usleep(10000); + } + + return false; +} + +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in, + char *out) +{ + int i, in_len = strlen(in); + + g_assert_true(write(sock_fd, in, in_len) == in_len); + for (i = 0; i < in_len; i++) { + g_assert_true(uart_wait_for_event(qts, NRF51_UART_BASE + + A_UART_RXDRDY)); + out[i] = qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD); + } + out[i] = '\0'; +} + +static void uart_w_to_txd(QTestState *qts, const char *in) +{ + int i, in_len = strlen(in); + + for (i = 0; i < in_len; i++) { + qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, in[i]); + g_assert_true(uart_wait_for_event(qts, NRF51_UART_BASE + + A_UART_TXDRDY)); + } +} + +static void test_nrf51_uart(void) +{ + int sock_fd; + char s[10]; + QTestState *qts = qtest_init_with_serial("-M microbit", &sock_fd); + + g_assert_true(write(sock_fd, "c", 1) == 1); + g_assert_cmphex(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD), ==, 0x00); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_ENABLE, 0x04); + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTRX, 0x01); + + g_assert_true(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_RXDRDY)); + qtest_writel(qts, NRF51_UART_BASE + A_UART_RXDRDY, 0x00); + g_assert_cmphex(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD), ==, 'c'); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENSET, 0x04); + g_assert_cmphex(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN), ==, 0x04); + qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENCLR, 0x04); + g_assert_cmphex(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN), ==, 0x00); + + uart_rw_to_rxd(qts, sock_fd, "hello", s); + g_assert_true(memcmp(s, "hello", 5) == 0); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01); + uart_w_to_txd(qts, "d"); + g_assert_true(read(sock_fd, s, 10) == 1); + g_assert_cmphex(s[0], ==, 'd'); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_SUSPEND, 0x01); + qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, 'h'); + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01); + uart_w_to_txd(qts, "world"); + g_assert_true(read(sock_fd, s, 10) == 5); + g_assert_true(memcmp(s, "world", 5) == 0); + + close(sock_fd); + + qtest_quit(qts); +} + /* Read a byte from I2C device at @addr from register @reg */ static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg) { @@ -302,6 +390,7 @@ int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); + qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart); qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c); From b94e809d3e4dbfa986d7c19fd2072c313b9a3a36 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 14/23] checkpatch: Don't emit spurious warnings about block comments In checkpatch we attempt to check for and warn about block comments which start with /* or /** followed by a non-blank. Unfortunately a bug in the regex meant that we would incorrectly warn about comments starting with "/**" with no following text: git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl - WARNING: Block comments use a leading /* on a separate line #34: FILE: tests/libqtest.h:233: +/** The sequence "/\*\*?" was intended to match either "/*" or "/**", but Perl's semantics for '?' allow it to backtrack and try the "matches 0 chars" option if the "matches 1 char" choice leads to a failure of the rest of the regex to match. Switch to "/\*\*?+" which uses what perlre(1) calls the "possessive" quantifier form: this means that if it matches the "/**" string it will not later backtrack to matching just the "/*" prefix. The other end of the regex is also wrong: it is attempting to check for "/* or /** followed by something that isn't just whitespace", but [ \t]*.+[ \t]* will match on pure whitespace. This is less significant but means that a line with just a comment-starter followed by trailing whitespace will generate an incorrect warning about block comment style as well as the correct error about trailing whitespace which a different checkpatch test emits. Fixes: 8c06fbdf36bf4d ("scripts/checkpatch.pl: Enforce multiline comment syntax") Reported-by: Thomas Huth Reported-by: Eric Blake Signed-off-by: Peter Maydell Reviewed-by: Eric Blake Message-id: 20190118165050.22270-1-peter.maydell@linaro.org --- scripts/checkpatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d10dddf1be..88682cb0a9 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1624,7 +1624,7 @@ sub process { # Block comments use /* on a line of its own if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ && #inline /*...*/ - $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank + $rawline =~ m@^\+.*/\*\*?+[ \t]*[^ \t]@) { # /* or /** non-blank WARN("Block comments use a leading /* on a separate line\n" . $herecurr); } From e5b517536cb47989236fbfe5bd69e644a1ff8467 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 15/23] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we aren't going to create any RPUs, then don't create the rpu-cluster unit. This allows us to add an assertion to the cluster object that it contains at least one CPU, which helps to avoid bugs in creating clusters and putting CPUs in them. Signed-off-by: Peter Maydell Reviewed-by: Edgar E. Iglesias Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Message-id: 20190121184314.14311-1-peter.maydell@linaro.org --- hw/arm/xlnx-zynqmp.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index c67ac2e64a..70cbe6bd47 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -178,6 +178,11 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu, int i; int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS); + if (num_rpus <= 0) { + /* Don't create rpu-cluster object if there's nothing to put in it */ + return; + } + object_initialize_child(OBJECT(s), "rpu-cluster", &s->rpu_cluster, sizeof(s->rpu_cluster), TYPE_CPU_CLUSTER, &error_abort, NULL); From b617ca9223353624000017e2fa499d6d3296b293 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 16/23] aspeed/smc: fix default read value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0xFFFFFFFF should be returned for non implemented registers. Signed-off-by: Cédric Le Goater Reviewed-by: Joel Stanley Message-id: 20190124140519.13838-2-clg@kaod.org Signed-off-by: Peter Maydell --- hw/ssi/aspeed_smc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 1270842dcf..7af808c33c 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -670,7 +670,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size) } else { qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n", __func__, addr); - return 0; + return -1; } } From 597d6bb3e8a93c4c0670df93f07c321ae84d2930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 17/23] aspeed/smc: define registers for all possible CS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The model should expose one control register per possible CS. When testing the validity of the register number in the read operation, replace 's->num_cs' by 'ctrl->max_slaves' which represents the maximum number of flash devices a controller can handle. Signed-off-by: Cédric Le Goater Reviewed-by: Joel Stanley Message-id: 20190124140519.13838-3-clg@kaod.org Signed-off-by: Peter Maydell --- hw/ssi/aspeed_smc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 7af808c33c..6045ca11b9 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -665,7 +665,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size) addr == s->r_ce_ctrl || addr == R_INTR_CTRL || (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) || - (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) { + (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) { return s->regs[addr]; } else { qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n", From 9149af2a2d3609507959bb17b74a35c3cebc5f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 18/23] aspeed/smc: Add dummy data register MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SMC controllers have a register containing the byte that will be used as dummy output. It can be modified by software. Signed-off-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Joel Stanley Message-id: 20190124140519.13838-4-clg@kaod.org Signed-off-by: Peter Maydell --- hw/ssi/aspeed_smc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 6045ca11b9..9f3b6f4b45 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -98,8 +98,8 @@ /* Misc Control Register #1 */ #define R_MISC_CTRL1 (0x50 / 4) -/* Misc Control Register #2 */ -#define R_MISC_CTRL2 (0x54 / 4) +/* SPI dummy cycle data */ +#define R_DUMMY_DATA (0x54 / 4) /* DMA Control/Status Register */ #define R_DMA_CTRL (0x80 / 4) @@ -529,7 +529,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr) */ if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) { for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) { - ssi_transfer(fl->controller->spi, 0xFF); + ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff); } } } @@ -664,6 +664,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size) addr == s->r_timings || addr == s->r_ce_ctrl || addr == R_INTR_CTRL || + addr == R_DUMMY_DATA || (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) || (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) { return s->regs[addr]; @@ -697,6 +698,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data, if (value != s->regs[R_SEG_ADDR0 + cs]) { aspeed_smc_flash_set_segment(s, cs, value); } + } else if (addr == R_DUMMY_DATA) { + s->regs[addr] = value & 0xff; } else { qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n", __func__, addr); From f95c4bffdc4c53b29f89762cab4adc5a43f95daf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 19/23] aspeed/smc: snoop SPI transfers to fake dummy cycles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The m25p80 models dummy cycles using byte transfers. This works well when the transfers are initiated by the QEMU model of a SPI controller but when these are initiated by the OS, it breaks emulation. Snoop the SPI transfer to catch commands requiring dummy cycles and replace them with byte transfers compatible with the m25p80 model. Signed-off-by: Cédric Le Goater Reviewed-by: Alistair Francis Reviewed-by: Francisco Iglesias Message-id: 20190124140519.13838-5-clg@kaod.org Signed-off-by: Peter Maydell --- hw/ssi/aspeed_smc.c | 115 +++++++++++++++++++++++++++++++++++- include/hw/ssi/aspeed_smc.h | 3 + 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 9f3b6f4b45..f1e66870d7 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -145,6 +145,9 @@ /* Flash opcodes. */ #define SPI_OP_READ 0x03 /* Read data bytes (low frequency) */ +#define SNOOP_OFF 0xFF +#define SNOOP_START 0x0 + /* * Default segments mapping addresses and size for each slave per * controller. These can be changed when board is initialized with the @@ -566,6 +569,101 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size) return ret; } +/* + * TODO (clg@kaod.org): stolen from xilinx_spips.c. Should move to a + * common include header. + */ +typedef enum { + READ = 0x3, READ_4 = 0x13, + FAST_READ = 0xb, FAST_READ_4 = 0x0c, + DOR = 0x3b, DOR_4 = 0x3c, + QOR = 0x6b, QOR_4 = 0x6c, + DIOR = 0xbb, DIOR_4 = 0xbc, + QIOR = 0xeb, QIOR_4 = 0xec, + + PP = 0x2, PP_4 = 0x12, + DPP = 0xa2, + QPP = 0x32, QPP_4 = 0x34, +} FlashCMD; + +static int aspeed_smc_num_dummies(uint8_t command) +{ + switch (command) { /* check for dummies */ + case READ: /* no dummy bytes/cycles */ + case PP: + case DPP: + case QPP: + case READ_4: + case PP_4: + case QPP_4: + return 0; + case FAST_READ: + case DOR: + case QOR: + case DOR_4: + case QOR_4: + return 1; + case DIOR: + case FAST_READ_4: + case DIOR_4: + return 2; + case QIOR: + case QIOR_4: + return 4; + default: + return -1; + } +} + +static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl, uint64_t data, + unsigned size) +{ + AspeedSMCState *s = fl->controller; + uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3; + + if (s->snoop_index == SNOOP_OFF) { + return false; /* Do nothing */ + + } else if (s->snoop_index == SNOOP_START) { + uint8_t cmd = data & 0xff; + int ndummies = aspeed_smc_num_dummies(cmd); + + /* + * No dummy cycles are expected with the current command. Turn + * off snooping and let the transfer proceed normally. + */ + if (ndummies <= 0) { + s->snoop_index = SNOOP_OFF; + return false; + } + + s->snoop_dummies = ndummies * 8; + + } else if (s->snoop_index >= addr_width + 1) { + + /* The SPI transfer has reached the dummy cycles sequence */ + for (; s->snoop_dummies; s->snoop_dummies--) { + ssi_transfer(s->spi, s->regs[R_DUMMY_DATA] & 0xff); + } + + /* If no more dummy cycles are expected, turn off snooping */ + if (!s->snoop_dummies) { + s->snoop_index = SNOOP_OFF; + } else { + s->snoop_index += size; + } + + /* + * Dummy cycles have been faked already. Ignore the current + * SPI transfer + */ + return true; + } + + s->snoop_index += size; + return false; +} + static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data, unsigned size) { @@ -581,6 +679,10 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data, switch (aspeed_smc_flash_mode(fl)) { case CTRL_USERMODE: + if (aspeed_smc_do_snoop(fl, data, size)) { + break; + } + for (i = 0; i < size; i++) { ssi_transfer(s->spi, (data >> (8 * i)) & 0xff); } @@ -613,7 +715,9 @@ static const MemoryRegionOps aspeed_smc_flash_ops = { static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl) { - const AspeedSMCState *s = fl->controller; + AspeedSMCState *s = fl->controller; + + s->snoop_index = aspeed_smc_is_ce_stop_active(fl) ? SNOOP_OFF : SNOOP_START; qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl)); } @@ -652,6 +756,9 @@ static void aspeed_smc_reset(DeviceState *d) if (s->ctrl->segments == aspeed_segments_fmc) { s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0); } + + s->snoop_index = SNOOP_OFF; + s->snoop_dummies = 0; } static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size) @@ -793,10 +900,12 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp) static const VMStateDescription vmstate_aspeed_smc = { .name = "aspeed.smc", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), + VMSTATE_UINT8(snoop_index, AspeedSMCState), + VMSTATE_UINT8(snoop_dummies, AspeedSMCState), VMSTATE_END_OF_LIST() } }; diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index 1f557313fa..3b1e7fce6c 100644 --- a/include/hw/ssi/aspeed_smc.h +++ b/include/hw/ssi/aspeed_smc.h @@ -98,6 +98,9 @@ typedef struct AspeedSMCState { uint8_t conf_enable_w0; AspeedSMCFlash *flashes; + + uint8_t snoop_index; + uint8_t snoop_dummies; } AspeedSMCState; #endif /* ASPEED_SMC_H */ From fa434424652ecfd3efba39e11ff7a1a560943abd Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 20/23] hw/arm/xlnx-zynqmp: Realize cluster after putting RPUs in it Currently the cluster implementation doesn't have any constraints on the ordering of realizing the TYPE_CPU_CLUSTER and populating it with child objects. We want to impose a constraint that realize must happen only after all the child objects are added, so move the realize of rpu_cluster. (The apu_cluster is already realized after child population.) Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Luc Michel Reviewed-by: Alistair Francis Reviewed-by: Edgar E. Iglesias Message-id: 20190121152218.9592-2-peter.maydell@linaro.org --- hw/arm/xlnx-zynqmp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 70cbe6bd47..4f8bc41d9d 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -188,8 +188,6 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu, &error_abort, NULL); qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1); - qdev_init_nofail(DEVICE(&s->rpu_cluster)); - for (i = 0; i < num_rpus; i++) { char *name; @@ -217,6 +215,8 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu, return; } } + + qdev_init_nofail(DEVICE(&s->rpu_cluster)); } static void xlnx_zynqmp_init(Object *obj) From 7ea7b9ad532e59c3efbcabff0e3484f4df06104c Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:05 +0000 Subject: [PATCH 21/23] qom/cpu: Add cluster_index to CPUState For TCG we want to distinguish which cluster a CPU is in, and we need to do it quickly. Cache the cluster index in the CPUState struct, by having the cluster object set cpu->cluster_index for each CPU child when it is realized. This means that board/SoC code must add all CPUs to the cluster before realizing the cluster object. Regrettably QOM provides no way to prevent adding children to a realized object and no way for the parent to be notified when a new child is added to it, so we don't have any way to enforce/assert this constraint; all we can do is document it in a comment. We can at least put in a check that the cluster contains at least one CPU, which should catch the typical cases of "realized cluster too early" or "forgot to parent the CPUs into it". The restriction on how many clusters can exist in the system is imposed by TCG code which will be added in a subsequent commit, but the check to enforce it in cluster.c fits better in this one. Signed-off-by: Peter Maydell Reviewed-by: Edgar E. Iglesias Reviewed-by: Alistair Francis Message-id: 20190121152218.9592-3-peter.maydell@linaro.org --- hw/cpu/cluster.c | 46 ++++++++++++++++++++++++++++++++++++++++ include/hw/cpu/cluster.h | 24 +++++++++++++++++++++ include/qom/cpu.h | 7 ++++++ qom/cpu.c | 1 + 4 files changed, 78 insertions(+) diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c index 9d50a235d5..25f90702b1 100644 --- a/hw/cpu/cluster.c +++ b/hw/cpu/cluster.c @@ -20,19 +20,65 @@ #include "qemu/osdep.h" #include "hw/cpu/cluster.h" +#include "qom/cpu.h" #include "qapi/error.h" #include "qemu/module.h" +#include "qemu/cutils.h" static Property cpu_cluster_properties[] = { DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0), DEFINE_PROP_END_OF_LIST() }; +typedef struct CallbackData { + CPUClusterState *cluster; + int cpu_count; +} CallbackData; + +static int add_cpu_to_cluster(Object *obj, void *opaque) +{ + CallbackData *cbdata = opaque; + CPUState *cpu = (CPUState *)object_dynamic_cast(obj, TYPE_CPU); + + if (cpu) { + cpu->cluster_index = cbdata->cluster->cluster_id; + cbdata->cpu_count++; + } + return 0; +} + +static void cpu_cluster_realize(DeviceState *dev, Error **errp) +{ + /* Iterate through all our CPU children and set their cluster_index */ + CPUClusterState *cluster = CPU_CLUSTER(dev); + Object *cluster_obj = OBJECT(dev); + CallbackData cbdata = { + .cluster = cluster, + .cpu_count = 0, + }; + + if (cluster->cluster_id >= MAX_CLUSTERS) { + error_setg(errp, "cluster-id must be less than %d", MAX_CLUSTERS); + return; + } + + object_child_foreach_recursive(cluster_obj, add_cpu_to_cluster, &cbdata); + + /* + * A cluster with no CPUs is a bug in the board/SoC code that created it; + * if you hit this during development of new code, check that you have + * created the CPUs and parented them into the cluster object before + * realizing the cluster object. + */ + assert(cbdata.cpu_count > 0); +} + static void cpu_cluster_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); dc->props = cpu_cluster_properties; + dc->realize = cpu_cluster_realize; } static const TypeInfo cpu_cluster_type_info = { diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h index 7381823243..549c2d31d4 100644 --- a/include/hw/cpu/cluster.h +++ b/include/hw/cpu/cluster.h @@ -34,12 +34,36 @@ * Arm big.LITTLE system) they should be in different clusters. If the CPUs do * not have the same view of memory (for example the main CPU and a management * controller processor) they should be in different clusters. + * + * A cluster is created by creating an object of TYPE_CPU_CLUSTER, and then + * adding the CPUs to it as QOM child objects (e.g. using the + * object_initialize_child() or object_property_add_child() functions). + * The CPUs may be either direct children of the cluster object, or indirect + * children (e.g. children of children of the cluster object). + * + * All CPUs must be added as children before the cluster is realized. + * (Regrettably QOM provides no way to prevent adding children to a realized + * object and no way for the parent to be notified when a new child is added + * to it, so this restriction is not checked for, but the system will not + * behave correctly if it is not adhered to. The cluster will assert that + * it contains at least one CPU, which should catch most inadvertent + * violations of this constraint.) + * + * A CPU which is not put into any cluster will be considered implicitly + * to be in a cluster with all the other "loose" CPUs, so all CPUs that are + * not assigned to clusters must be identical. */ #define TYPE_CPU_CLUSTER "cpu-cluster" #define CPU_CLUSTER(obj) \ OBJECT_CHECK(CPUClusterState, (obj), TYPE_CPU_CLUSTER) +/* + * This limit is imposed by TCG, which puts the cluster ID into an + * 8 bit field (and uses all-1s for the default "not in any cluster"). + */ +#define MAX_CLUSTERS 255 + /** * CPUClusterState: * @cluster_id: The cluster ID. This value is for internal use only and should diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 16bbed1ae0..4c2feb9c17 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -280,6 +280,11 @@ struct qemu_work_item; /** * CPUState: * @cpu_index: CPU index (informative). + * @cluster_index: Identifies which cluster this CPU is in. + * For boards which don't define clusters or for "loose" CPUs not assigned + * to a cluster this will be UNASSIGNED_CLUSTER_INDEX; otherwise it will + * be the same as the cluster-id property of the CPU object's TYPE_CPU_CLUSTER + * QOM parent. * @nr_cores: Number of cores within this CPU package. * @nr_threads: Number of threads within this CPU. * @running: #true if CPU is currently running (lockless). @@ -405,6 +410,7 @@ struct CPUState { /* TODO Move common fields from CPUArchState here. */ int cpu_index; + int cluster_index; uint32_t halted; uint32_t can_do_io; int32_t exception_index; @@ -1111,5 +1117,6 @@ extern const struct VMStateDescription vmstate_cpu_common; #endif /* NEED_CPU_H */ #define UNASSIGNED_CPU_INDEX -1 +#define UNASSIGNED_CLUSTER_INDEX -1 #endif diff --git a/qom/cpu.c b/qom/cpu.c index 5442a7323b..f5579b1cd5 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -364,6 +364,7 @@ static void cpu_common_initfn(Object *obj) CPUClass *cc = CPU_GET_CLASS(obj); cpu->cpu_index = UNASSIGNED_CPU_INDEX; + cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX; cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs; /* *-user doesn't have configurable SMP topology */ /* the default value is changed by qemu_init_vcpu() for softmmu */ From f7b78602fdc6c6e4befc90159da8e93900b4bcb1 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:06 +0000 Subject: [PATCH 22/23] accel/tcg: Add cluster number to TCG TB hash Include the cluster number in the hash we use to look up TBs. This is important because a TB that is valid for one cluster at a given physical address and set of CPU flags is not necessarily valid for another: the two clusters may have different views of physical memory, or may have different CPU features (eg FPU present or absent). We put the cluster number in the high 8 bits of the TB cflags. This gives us up to 256 clusters, which should be enough for anybody. If we ever need more, or need more bits in cflags for other purposes, we could make tb_hash_func() take more data (and expand qemu_xxhash7() to qemu_xxhash8()). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Alistair Francis Reviewed-by: Edgar E. Iglesias Message-id: 20190121152218.9592-4-peter.maydell@linaro.org --- accel/tcg/cpu-exec.c | 3 +++ accel/tcg/translate-all.c | 3 +++ include/exec/exec-all.h | 4 +++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 870027d435..6c4a33262f 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -325,6 +325,9 @@ TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc, struct tb_desc desc; uint32_t h; + cf_mask &= ~CF_CLUSTER_MASK; + cf_mask |= cpu->cluster_index << CF_CLUSTER_SHIFT; + desc.env = (CPUArchState *)cpu->env_ptr; desc.cs_base = cs_base; desc.flags = flags; diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 8cb8c8870e..7364e8a071 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1688,6 +1688,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu, cflags |= CF_NOCACHE | 1; } + cflags &= ~CF_CLUSTER_MASK; + cflags |= cpu->cluster_index << CF_CLUSTER_SHIFT; + buffer_overflow: tb = tb_alloc(pc); if (unlikely(!tb)) { diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 815e5b1e83..aa7b81aaf0 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -351,9 +351,11 @@ struct TranslationBlock { #define CF_USE_ICOUNT 0x00020000 #define CF_INVALID 0x00040000 /* TB is stale. Set with @jmp_lock held */ #define CF_PARALLEL 0x00080000 /* Generate code for a parallel context */ +#define CF_CLUSTER_MASK 0xff000000 /* Top 8 bits are cluster ID */ +#define CF_CLUSTER_SHIFT 24 /* cflags' mask for hashing/comparison */ #define CF_HASH_MASK \ - (CF_COUNT_MASK | CF_LAST_IO | CF_USE_ICOUNT | CF_PARALLEL) + (CF_COUNT_MASK | CF_LAST_IO | CF_USE_ICOUNT | CF_PARALLEL | CF_CLUSTER_MASK) /* Per-vCPU dynamic tracing state used to generate this TB */ uint32_t trace_vcpu_dstate; From 46f5abc0a2566ac3dc954eeb62fd625f0eaca120 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 29 Jan 2019 11:46:06 +0000 Subject: [PATCH 23/23] gdbstub: Simplify gdb_get_cpu_pid() to use cpu->cluster_index Now we're keeping the cluster index in the CPUState, we don't need to jump through hoops in gdb_get_cpu_pid() to find the associated cluster object. Signed-off-by: Peter Maydell Reviewed-by: Luc Michel Reviewed-by: Edgar E. Iglesias Message-id: 20190121152218.9592-5-peter.maydell@linaro.org --- gdbstub.c | 48 +++++------------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index d4cc6ecf99..3129b5c284 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -644,50 +644,12 @@ static int memtox(char *buf, const char *mem, int len) static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu) { -#ifndef CONFIG_USER_ONLY - gchar *path, *name = NULL; - Object *obj; - CPUClusterState *cluster; - uint32_t ret; - - path = object_get_canonical_path(OBJECT(cpu)); - - if (path == NULL) { - /* Return the default process' PID */ - ret = s->processes[s->process_num - 1].pid; - goto out; - } - - name = object_get_canonical_path_component(OBJECT(cpu)); - assert(name != NULL); - - /* - * Retrieve the CPU parent path by removing the last '/' and the CPU name - * from the CPU canonical path. - */ - path[strlen(path) - strlen(name) - 1] = '\0'; - - obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL); - - if (obj == NULL) { - /* Return the default process' PID */ - ret = s->processes[s->process_num - 1].pid; - goto out; - } - - cluster = CPU_CLUSTER(obj); - ret = cluster->cluster_id + 1; - -out: - g_free(name); - g_free(path); - - return ret; - -#else /* TODO: In user mode, we should use the task state PID */ - return s->processes[s->process_num - 1].pid; -#endif + if (cpu->cluster_index == UNASSIGNED_CLUSTER_INDEX) { + /* Return the default process' PID */ + return s->processes[s->process_num - 1].pid; + } + return cpu->cluster_index + 1; } static GDBProcess *gdb_get_process(const GDBState *s, uint32_t pid)