On 7/27/20 5:45 PM, Peter Maydell wrote:
> 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 ptimre 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
Thanks for looking at this.
> Fixes: cc2722ec83ad944505fe
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> I don't have a test image for KZM so this is the minimal
> obviously-safe change. I'm pretty sure that actually we could
> add a "break" after the imx_epit_reset() call because all of
> the work done by the following code is duplicating the ptimer
> setup done by the reset function. But I'm not really happy making
> that change without a test image...
Agreed. I'd add a comment in the code to not forget about this...
> ---
> hw/timer/imx_epit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index baf6338e1a6..4f51e6e12da 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -199,15 +199,18 @@ 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));
... such:
/* break; ??? */
Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> - } else {
> + }
> +
> + ptimer_transaction_begin(s->timer_cmp);
> + ptimer_transaction_begin(s->timer_reload);
> +
> + if (!(s->cr & CR_SWR)) {
> imx_epit_set_freq(s);
> }
>
>
