Re: [PATCH for-5.1] hw/timer/imx_epit: Avoid assertion when CR.SWR is written

[Philippe Mathieu-Daudé]

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.maydell@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é <f4bug@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);
>          }
>  
>