From e7e5a9595ab1136845c444141830fca0d2746a73 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 3 Aug 2020 17:55:03 +0100
Subject: [PATCH 1/8] hw/arm/netduino2, netduinoplus2: Set system_clock_scale
The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale
global, which meant that if guest code used the systick timer in "use
the processor clock" mode it would hang because time never advances.
Set the global to match the documented CPU clock speed of these boards.
Judging by the data sheet this is slightly simplistic because the
SoC allows configuration of the SYSCLK source and frequency via the
RCC (reset and clock control) module, but we don't model that.
Fixes: https://bugs.launchpad.net/qemu/+bug/1876187
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20200727162617.26227-1-peter.maydell@linaro.org
---
hw/arm/netduino2.c | 10 ++++++++++
hw/arm/netduinoplus2.c | 10 ++++++++++
2 files changed, 20 insertions(+)
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 79e19392b5..8f10334144 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -30,10 +30,20 @@
#include "hw/arm/stm32f205_soc.h"
#include "hw/arm/boot.h"
+/* Main SYSCLK frequency in Hz (120MHz) */
+#define SYSCLK_FRQ 120000000ULL
+
static void netduino2_init(MachineState *machine)
{
DeviceState *dev;
+ /*
+ * TODO: ideally we would model the SoC RCC and let it handle
+ * system_clock_scale, including its ability to define different
+ * possible SYSCLK sources.
+ */
+ system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
+
dev = qdev_new(TYPE_STM32F205_SOC);
qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
index 958d21dd9f..68abd3ec69 100644
--- a/hw/arm/netduinoplus2.c
+++ b/hw/arm/netduinoplus2.c
@@ -30,10 +30,20 @@
#include "hw/arm/stm32f405_soc.h"
#include "hw/arm/boot.h"
+/* Main SYSCLK frequency in Hz (168MHz) */
+#define SYSCLK_FRQ 168000000ULL
+
static void netduinoplus2_init(MachineState *machine)
{
DeviceState *dev;
+ /*
+ * TODO: ideally we would model the SoC RCC and let it handle
+ * system_clock_scale, including its ability to define different
+ * possible SYSCLK sources.
+ */
+ system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
+
dev = qdev_new(TYPE_STM32F405_SOC);
qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
From faf7c6de34fb8eaa566726cc658ba5ad81fb32af Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 3 Aug 2020 17:55:03 +0100
Subject: [PATCH 2/8] include/hw/irq.h: New function qemu_irq_is_connected()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Mostly devices don't need to care whether one of their output
qemu_irq lines is connected, because functions like qemu_set_irq()
silently do nothing if there is nothing on the other end. However
sometimes a device might want to implement default behaviour for the
case where the machine hasn't wired the line up to anywhere.
Provide a function qemu_irq_is_connected() that devices can use for
this purpose. (The test is trivial but encapsulating it in a
function makes it easier to see where we're doing it in case we need
to change the implementation later.)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20200728103744.6909-2-peter.maydell@linaro.org
---
include/hw/irq.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 24ba0ece11..dc7abf199e 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -55,4 +55,22 @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2);
on an existing vector of qemu_irq. */
void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
+/**
+ * qemu_irq_is_connected: Return true if IRQ line is wired up
+ *
+ * If a qemu_irq has a device on the other (receiving) end of it,
+ * return true; otherwise return false.
+ *
+ * Usually device models don't need to care whether the machine model
+ * has wired up their outbound qemu_irq lines, because functions like
+ * qemu_set_irq() silently do nothing if there is nothing on the other
+ * end of the line. However occasionally a device model will want to
+ * provide default behaviour if its output is left floating, and
+ * it can use this function to identify when that is the case.
+ */
+static inline bool qemu_irq_is_connected(qemu_irq irq)
+{
+ return irq != NULL;
+}
+
#endif
From 9e60d759d38d1faae1d85de2c53411e635be3cf2 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 3 Aug 2020 17:55:03 +0100
Subject: [PATCH 3/8] hw/intc/armv7m_nvic: Provide default "reset the system"
behaviour for SYSRESETREQ
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The NVIC provides an outbound qemu_irq "SYSRESETREQ" which it signals
when the guest sets the SYSRESETREQ bit in the AIRCR register. This
matches the hardware design (where the CPU has a signal of this name
and it is up to the SoC to connect that up to an actual reset
mechanism), but in QEMU it mostly results in duplicated code in SoC
objects and bugs where SoC model implementors forget to wire up the
SYSRESETREQ line.
Provide a default behaviour for the case where SYSRESETREQ is not
actually connected to anything: use qemu_system_reset_request() to
perform a system reset. This will allow us to remove the
implementations of SYSRESETREQ handling from the boards where that's
exactly what it does, and also fixes the bugs in the board models
which forgot to wire up the signal:
* microbit
* mps2-an385
* mps2-an505
* mps2-an511
* mps2-an521
* musca-a
* musca-b1
* netduino
* netduinoplus2
We still allow the board to wire up the signal if it needs to, in case
we need to model more complicated reset controller logic or to model
buggy SoC hardware which forgot to wire up the line itself. But
defaulting to "reset the system" is more often going to be correct
than defaulting to "do nothing".
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20200728103744.6909-3-peter.maydell@linaro.org
---
hw/intc/armv7m_nvic.c | 17 ++++++++++++++++-
include/hw/arm/armv7m.h | 4 +++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 3c4b6e6d70..277a98b87b 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -19,6 +19,7 @@
#include "hw/intc/armv7m_nvic.h"
#include "hw/irq.h"
#include "hw/qdev-properties.h"
+#include "sysemu/runstate.h"
#include "target/arm/cpu.h"
#include "exec/exec-all.h"
#include "exec/memop.h"
@@ -64,6 +65,20 @@ static const uint8_t nvic_id[] = {
0x00, 0xb0, 0x1b, 0x00, 0x0d, 0xe0, 0x05, 0xb1
};
+static void signal_sysresetreq(NVICState *s)
+{
+ if (qemu_irq_is_connected(s->sysresetreq)) {
+ qemu_irq_pulse(s->sysresetreq);
+ } else {
+ /*
+ * Default behaviour if the SoC doesn't need to wire up
+ * SYSRESETREQ (eg to a system reset controller of some kind):
+ * perform a system reset via the usual QEMU API.
+ */
+ qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+ }
+}
+
static int nvic_pending_prio(NVICState *s)
{
/* return the group priority of the current pending interrupt,
@@ -1524,7 +1539,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
if (value & R_V7M_AIRCR_SYSRESETREQ_MASK) {
if (attrs.secure ||
!(cpu->env.v7m.aircr & R_V7M_AIRCR_SYSRESETREQS_MASK)) {
- qemu_irq_pulse(s->sysresetreq);
+ signal_sysresetreq(s);
}
}
if (value & R_V7M_AIRCR_VECTCLRACTIVE_MASK) {
diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index d2c74d3872..a30e3c6471 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -35,7 +35,9 @@ typedef struct {
/* ARMv7M container object.
* + Unnamed GPIO input lines: external IRQ lines for the NVIC
- * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ
+ * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ.
+ * If this GPIO is not wired up then the NVIC will default to performing
+ * a qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
* + Property "cpu-type": CPU type to instantiate
* + Property "num-irq": number of external IRQ lines
* + Property "memory": MemoryRegion defining the physical address space
From fc6bb6e67e2f2b81de765a1c1ad5956de625ab19 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 3 Aug 2020 17:55:03 +0100
Subject: [PATCH 4/8] msf2-soc, stellaris: Don't wire up SYSRESETREQ
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The MSF2 SoC model and the Stellaris board code both wire
SYSRESETREQ up to a function that just invokes
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
This is now the default action that the NVIC does if the line is
not connected, so we can delete the handling code.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20200728103744.6909-4-peter.maydell@linaro.org
---
hw/arm/msf2-soc.c | 11 -----------
hw/arm/stellaris.c | 12 ------------
2 files changed, 23 deletions(-)
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 33ea7df342..d2c29e82d1 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -30,7 +30,6 @@
#include "hw/irq.h"
#include "hw/arm/msf2-soc.h"
#include "hw/misc/unimp.h"
-#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#define MSF2_TIMER_BASE 0x40004000
@@ -59,13 +58,6 @@ static const int spi_irq[MSF2_NUM_SPIS] = { 2, 3 };
static const int uart_irq[MSF2_NUM_UARTS] = { 10, 11 };
static const int timer_irq[MSF2_NUM_TIMERS] = { 14, 15 };
-static void do_sys_reset(void *opaque, int n, int level)
-{
- if (level) {
- qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
- }
-}
-
static void m2sxxx_soc_initfn(Object *obj)
{
MSF2State *s = MSF2_SOC(obj);
@@ -130,9 +122,6 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
return;
}
- qdev_connect_gpio_out_named(DEVICE(&s->armv7m.nvic), "SYSRESETREQ", 0,
- qemu_allocate_irq(&do_sys_reset, NULL, 0));
-
system_clock_scale = NANOSECONDS_PER_SECOND / s->m3clk;
for (i = 0; i < MSF2_NUM_UARTS; i++) {
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 28eb15c76c..5f9d080180 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -18,7 +18,6 @@
#include "hw/boards.h"
#include "qemu/log.h"
#include "exec/address-spaces.h"
-#include "sysemu/runstate.h"
#include "sysemu/sysemu.h"
#include "hw/arm/armv7m.h"
#include "hw/char/pl011.h"
@@ -1206,14 +1205,6 @@ static void stellaris_adc_init(Object *obj)
qdev_init_gpio_in(dev, stellaris_adc_trigger, 1);
}
-static
-void do_sys_reset(void *opaque, int n, int level)
-{
- if (level) {
- qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
- }
-}
-
/* Board init. */
static stellaris_board_info stellaris_boards[] = {
{ "LM3S811EVB",
@@ -1317,9 +1308,6 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
/* This will exit with an error if the user passed us a bad cpu_type */
sysbus_realize_and_unref(SYS_BUS_DEVICE(nvic), &error_fatal);
- qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0,
- qemu_allocate_irq(&do_sys_reset, NULL, 0));
-
if (board->dc1 & (1 << 16)) {
dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
qdev_get_gpio_in(nvic, 14),
From 8796fe40dd30cd9ffd3c958906471715c923b341 Mon Sep 17 00:00:00 2001
From: Richard Henderson <richard.henderson@linaro.org>
Date: Mon, 3 Aug 2020 17:55:03 +0100
Subject: [PATCH 5/8] target/arm: Fix AddPAC error indication
The definition of top_bit used in this function is one higher
than that used in the Arm ARM psuedo-code, which put the error
indication at top_bit - 1 at the wrong place, which meant that
it wasn't visible to Auth.
Fixing the definition of top_bit requires more changes, because
its most common use is for the count of bits in top_bit:bot_bit,
which would then need to be computed as top_bit - bot_bit + 1.
For now, prefer the minimal fix to the error indication alone.
Fixes: 63ff0ca94cb
Reported-by: Derrick McKee <derrick.mckee@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20200728195706.11087-1-richard.henderson@linaro.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: added comment about the divergence from the pseudocode]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/pauth_helper.c | 6 +++++-
tests/tcg/aarch64/Makefile.target | 2 +-
tests/tcg/aarch64/pauth-5.c | 33 +++++++++++++++++++++++++++++++
3 files changed, 39 insertions(+), 2 deletions(-)
create mode 100644 tests/tcg/aarch64/pauth-5.c
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index b909630317..6dbab03768 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -300,7 +300,11 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
*/
test = sextract64(ptr, bot_bit, top_bit - bot_bit);
if (test != 0 && test != -1) {
- pac ^= MAKE_64BIT_MASK(top_bit - 1, 1);
+ /*
+ * Note that our top_bit is one greater than the pseudocode's
+ * version, hence "- 2" here.
+ */
+ pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
}
/*
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index b617f2ac7e..e7249915e7 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -19,7 +19,7 @@ run-fcvt: fcvt
# Pauth Tests
ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_ARMV8_3),)
-AARCH64_TESTS += pauth-1 pauth-2 pauth-4
+AARCH64_TESTS += pauth-1 pauth-2 pauth-4 pauth-5
pauth-%: CFLAGS += -march=armv8.3-a
run-pauth-%: QEMU_OPTS += -cpu max
run-plugin-pauth-%: QEMU_OPTS += -cpu max
diff --git a/tests/tcg/aarch64/pauth-5.c b/tests/tcg/aarch64/pauth-5.c
new file mode 100644
index 0000000000..67c257918b
--- /dev/null
+++ b/tests/tcg/aarch64/pauth-5.c
@@ -0,0 +1,33 @@
+#include <assert.h>
+
+static int x;
+
+int main()
+{
+ int *p0 = &x, *p1, *p2, *p3;
+ unsigned long salt = 0;
+
+ /*
+ * With TBI enabled and a 48-bit VA, there are 7 bits of auth, and so
+ * a 1/128 chance of auth = pac(ptr,key,salt) producing zero.
+ * Find a salt that creates auth != 0.
+ */
+ do {
+ salt++;
+ asm("pacda %0, %1" : "=r"(p1) : "r"(salt), "0"(p0));
+ } while (p0 == p1);
+
+ /*
+ * This pac must fail, because the input pointer bears an encryption,
+ * and so is not properly extended within bits [55:47]. This will
+ * toggle bit 54 in the output...
+ */
+ asm("pacda %0, %1" : "=r"(p2) : "r"(salt), "0"(p1));
+
+ /* ... so that the aut must fail, setting bit 53 in the output ... */
+ asm("autda %0, %1" : "=r"(p3) : "r"(salt), "0"(p2));
+
+ /* ... which means this equality must not hold. */
+ assert(p3 != p0);
+ return 0;
+}
From 88a90e3de6ae99cbcfcc04c862c51f241fdf685f Mon Sep 17 00:00:00 2001
From: Kaige Li <likaige@loongson.cn>
Date: Mon, 3 Aug 2020 17:55:04 +0100
Subject: [PATCH 6/8] target/arm: Avoid maybe-uninitialized warning with gcc
4.9
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
GCC version 4.9.4 isn't clever enough to figure out that all
execution paths in disas_ldst() that use 'fn' will have initialized
it first, and so it warns:
/home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
/home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
^
/home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was declared here
AtomicThreeOpFn *fn;
^
Make it happy by initializing the variable to NULL.
Signed-off-by: Kaige Li <likaige@loongson.cn>
Message-id: 1596110248-7366-2-git-send-email-likaige@loongson.cn
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: Clean up commit message and note which gcc version this was]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/translate-a64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8c0764957c..c98dfb17a8 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3315,7 +3315,7 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t insn,
bool r = extract32(insn, 22, 1);
bool a = extract32(insn, 23, 1);
TCGv_i64 tcg_rs, clean_addr;
- AtomicThreeOpFn *fn;
+ AtomicThreeOpFn *fn = NULL;
if (is_vector || !dc_isar_feature(aa64_atomics, s)) {
unallocated_encoding(s);
From ce4f70e81ed23c93ff39234672aff33114532640 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 27 Jul 2020 20:34:58 +0100
Subject: [PATCH 7/8] hw/arm/nrf51_soc: Set system_clock_scale
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The nrf51 SoC model wasn't setting the system_clock_scale
global.which meant that if guest code used the systick timer in "use
the processor clock" mode it would hang because time never advances.
Set the global to match the documented CPU clock speed for this SoC.
This SoC in fact doesn't have a SysTick timer (which is the only thing
currently that cares about the system_clock_scale), because it's
a configurable option in the Cortex-M0. However our Cortex-M0 and
thus our nrf51 and our micro:bit board do provide a SysTick, so
we ought to provide a functional one rather than a broken one.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20200727193458.31250-1-peter.maydell@linaro.org
---
hw/arm/nrf51_soc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 45e6cc97d7..e15981e019 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -32,6 +32,9 @@
#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
+/* HCLK (the main CPU clock) on this SoC is always 16MHz */
+#define HCLK_FRQ 16000000
+
static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
{
qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
@@ -65,6 +68,8 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
return;
}
+ system_clock_scale = NANOSECONDS_PER_SECOND / HCLK_FRQ;
+
object_property_set_link(OBJECT(&s->cpu), "memory", OBJECT(&s->container),
&error_abort);
if (!sysbus_realize(SYS_BUS_DEVICE(&s->cpu), errp)) {
From 13557fd392890cbd985bceba7f717e01efd674b8 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 27 Jul 2020 16:45:50 +0100
Subject: [PATCH 8/8] hw/timer/imx_epit: Avoid assertion when CR.SWR is written
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The imx_epit device has a software-controllable reset triggered by
setting the SWR bit in the CR register. An error in commit cc2722ec83ad9
means that we will end up assert()ing if the guest does this, because
the code in imx_epit_write() starts ptimer transactions, and then
imx_epit_reset() also starts ptimer transactions, triggering
"ptimer_transaction_begin: Assertion `!s->in_transaction' failed".
The cleanest way to avoid this double-transaction is to move the
start-transaction for the CR write handling down below the check of
the SWR bit.
Fixes: https://bugs.launchpad.net/qemu/+bug/1880424
Fixes: cc2722ec83ad944505fe
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20200727154550.3409-1-peter.maydell@linaro.org
---
hw/timer/imx_epit.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index baf6338e1a..ebd58254d1 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -199,15 +199,22 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
switch (offset >> 2) {
case 0: /* CR */
- ptimer_transaction_begin(s->timer_cmp);
- ptimer_transaction_begin(s->timer_reload);
oldcr = s->cr;
s->cr = value & 0x03ffffff;
if (s->cr & CR_SWR) {
/* handle the reset */
imx_epit_reset(DEVICE(s));
- } else {
+ /*
+ * TODO: could we 'break' here? following operations appear
+ * to duplicate the work imx_epit_reset() already did.
+ */
+ }
+
+ ptimer_transaction_begin(s->timer_cmp);
+ ptimer_transaction_begin(s->timer_reload);
+
+ if (!(s->cr & CR_SWR)) {
imx_epit_set_freq(s);
}