Currently the ptimer design uses a QEMU bottom-half as its mechanism
for calling back into the device model using the ptimer when the
timer has expired.  Unfortunately this design is fatally flawed,
because it means that there is a lag between the ptimer updating its
own state and the device callback function updating device state, and
guest accesses to device registers between the two can return
inconsistent device state. This was reported as a bug in a specific
timer device but it's a problem with the generic ptimer code:
https://bugs.launchpad.net/qemu/+bug/1777777

This RFC patchset introduces a change to the ptimer API so that
instead of using a bottom-half for the trigger a device can choose to
use a new 'transaction' based approach.  In this design (suggested by
RTH) all calls which modify ptimer state:
     - ptimer_set_period()
     - ptimer_set_freq()
     - ptimer_set_limit()
     - ptimer_set_count()
     - ptimer_run()
     - ptimer_stop()
must be between matched calls to ptimer_transaction_begin() and
ptimer_transaction_commit().  When ptimer_transaction_commit() is
called it will evaluate the state of the timer after all the changes
in the transaction, and call the callback if necessary.

I'm sending this out as an RFC to get opinions on the new API
before I proceed to converting all the users of ptimer. (My
plan is that the final patchset will start with these patches,
have another couple of dozen patches changing the other timer
devices using ptimer, and then have a "delete the BH API/code"
patch. Or we could split the "change to new API" parts into
smaller per-architecture ones.)

Patch 1 is some new trace events I added to the arm_timer code
while I was investigating the original bug report.
Patch 2 is just a function rename so we can keep the nicer
"ptimer_init()" name for the new API.
Patch 3 adds the new API and its implementation.
Patch 4 is a sample conversion of a ptimer-using device,
which fixes the reported bug:
https://bugs.launchpad.net/qemu/+bug/1777777

thanks
-- PMM

Peter Maydell (4):
  hw/timer/arm_timer: Add trace events
  ptimer: Rename ptimer_init() to ptimer_init_with_bh()
  ptimer: Provide new transaction-based API
  hw/timer/arm_timer.c: Switch to transaction-based ptimer API

 include/hw/ptimer.h              |  77 ++++++++++++++++++--
 hw/arm/musicpal.c                |   2 +-
 hw/core/ptimer.c                 | 116 +++++++++++++++++++++++++++----
 hw/dma/xilinx_axidma.c           |   2 +-
 hw/m68k/mcf5206.c                |   2 +-
 hw/m68k/mcf5208.c                |   2 +-
 hw/net/fsl_etsec/etsec.c         |   2 +-
 hw/net/lan9118.c                 |   2 +-
 hw/timer/allwinner-a10-pit.c     |   2 +-
 hw/timer/altera_timer.c          |   2 +-
 hw/timer/arm_mptimer.c           |   6 +-
 hw/timer/arm_timer.c             |  43 +++++++++---
 hw/timer/cmsdk-apb-dualtimer.c   |   2 +-
 hw/timer/cmsdk-apb-timer.c       |   2 +-
 hw/timer/digic-timer.c           |   2 +-
 hw/timer/etraxfs_timer.c         |   6 +-
 hw/timer/exynos4210_mct.c        |   7 +-
 hw/timer/exynos4210_pwm.c        |   2 +-
 hw/timer/exynos4210_rtc.c        |   4 +-
 hw/timer/grlib_gptimer.c         |   2 +-
 hw/timer/imx_epit.c              |   4 +-
 hw/timer/imx_gpt.c               |   2 +-
 hw/timer/lm32_timer.c            |   2 +-
 hw/timer/milkymist-sysctl.c      |   4 +-
 hw/timer/mss-timer.c             |   2 +-
 hw/timer/puv3_ost.c              |   2 +-
 hw/timer/sh_timer.c              |   2 +-
 hw/timer/slavio_timer.c          |   2 +-
 hw/timer/xilinx_timer.c          |   2 +-
 hw/watchdog/cmsdk-apb-watchdog.c |   2 +-
 tests/ptimer-test.c              |  22 +++---
 hw/timer/trace-events            |   7 ++
 32 files changed, 263 insertions(+), 75 deletions(-)

-- 
2.20.1

On 04/10/19 13:48, Peter Maydell wrote:
> must be between matched calls to ptimer_transaction_begin() and
> ptimer_transaction_commit().  When ptimer_transaction_commit() is
> called it will evaluate the state of the timer after all the changes
> in the transaction, and call the callback if necessary.

Could ptimer_stop/ptimer_run act as the begin and commit functions?
That is, ptimer_set_* can be called only between ptimer_stop and ptimer_run?

Thanks,

Paolo

On Fri, 4 Oct 2019 at 12:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/10/19 13:48, Peter Maydell wrote:
> > must be between matched calls to ptimer_transaction_begin() and
> > ptimer_transaction_commit().  When ptimer_transaction_commit() is
> > called it will evaluate the state of the timer after all the changes
> > in the transaction, and call the callback if necessary.
>
> Could ptimer_stop/ptimer_run act as the begin and commit functions?
> That is, ptimer_set_* can be called only between ptimer_stop and ptimer_run?

No, because stop/run causes the ptimer to "lose time"
(we stop the underlying timer and restart it). It's
very common for a device to want to change the ptimer
properties without a stop/restart -- "set the ptimer
count value when the guest writes to the device's counter
register" is the common one. Of the three begin/commit
blocks in the arm_timer.c conversion, only one of those
involves calls to stop/run, and even there we only
call stop/run if the write to the control register
modified the enable bit.

thanks
-- PMM

On 04/10/19 14:00, Peter Maydell wrote:
> No, because stop/run causes the ptimer to "lose time"
> (we stop the underlying timer and restart it). It's
> very common for a device to want to change the ptimer
> properties without a stop/restart -- "set the ptimer
> count value when the guest writes to the device's counter
> register" is the common one. Of the three begin/commit
> blocks in the arm_timer.c conversion, only one of those
> involves calls to stop/run, and even there we only
> call stop/run if the write to the control register
> modified the enable bit.

Ok, thanks.

Paolo