Re: [Qemu-arm] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

On Sep 25 16:18, Peter Maydell wrote:
> On 28 August 2018 at 21:03, Aaron Lindsay <address@hidden> wrote:
> > On Jun 28 17:40, Peter Maydell wrote:
> >> I had a look through the patchset, and I still can't see
> >> anything here which indicates how we're handling migration.
> >
> > I'll admit that I had not previously thought through everything
> > exhaustively, but I think I've convinced myself that *almost* everything
> > is already handled by calling .readfn/writefn on the registers for
> > migration... more below.
> 
> Thanks for writing up this explanation; it's very helpful. Sorry
> it's taken me a while to get to it -- you unfortunately sent it
> just as I was going on holiday...

No problem - I noticed you went quiet on the list right when I sent it
and figured I scared you away!

I've got a v6 staged - If you don't come up with significant design
changes based on my response below, I'll plan to send it in the next few
days.

> >> If this field is only ever used as a temporary while we're
> >> between op_start and op_finish, then we can just return it from
> >> start and take it as an argument to finish, and we're fine
> >> (migration of the PMCCNTR_EL0 register will be done by calling
> >> its readfn on the source and its writefn on the destination).
> >>
> >> If there's a reason for having this be a field in the state struct
> >> (I think you said counter overflow)? then we need to be sure that
> >> migration is going to do the right thing and that we don't for instance
> >> lose overflow indications during a migration.
> >
> > I think we do need additional state for supporting counter overflow
> > during normal operation (more than just between op_start and op_finish),
> > but I don't think sending additional state over the wire is necessary
> > for migration. Let me lay out my reasoning to see if you think it makes
> > sense:
> >
> > I observe that we seem to deal mostly with the following values with
> > respect to PMU counters, and I think it might be easier to discuss this
> > in terms of values rather than variables:
> >
> >         AC = current architectural value of PMCCNTR
> >         AL = architectural value of PMCCNTR when last observed/updated by
> >              the OS
> >         UC = current underlying counter value
> >         UL = underlying counter value when PMCCNTR last observed/updated by
> >              OS
> >         D = difference between underlying counter value and architectural
> >             value
> >
> > At any given point in time, the following should then be true during
> > normal execution (i.e. between OS accesses to the counter):
> >         UL - AL == D == UC - AC
> >
> > The approach taken previous to my patchset was to store D in
> > cp15.c15_ccnt most of the time, updating that variable to hold AC when
> > read (by doing `c15_ccnt = UC - c15_ccnt`... an implementation using `AC
> > = UC - D`, which is a different way to write `D = UC - AC` from above).
> > This operation was reversed after a write completes, so that c15_ccnt
> > again holds a value corresponding to D.
> >
> > Only storing D works well and minimizes memory usage if you don't care
> > about detecting overflows. However, without some frame of reference for
> > what the last observed architectural value was relative to its current
> > value, we have no way to know if the counter has overflowed in the
> > intervening time. In other words, we need to know both AC and AL to know
> > if the counter has overflowed. Assuming we store D and can obtain UC we
> > can calculate AC, but we need to store at least one additional piece of
> > information to obtain AL (either AL itself or UL).
> 
> In this scheme, how do you tell the difference between "AC > AL
> because it hasn't overflowed" and "AC > AL because it has overflowed
> and run all the way round past AL again" ? That is, I'm not sure that
> storing AL helps you to reliably detect overflow.

I wrestled with how to accomplish generic overflow detection in a
foolproof way that fits with the event-driven design. My current
implementation (see [1]) adds a function called ns_per_count for each
counter type, which is used to setup a timer to overflow at the earliest
time a given counter is expected to overflow. My thinking is that even
if this isn't perfect (i.e. if there's a little 'skid'), it is likely to
be close enough to prevent the type of wrap-around double-overflow you
describe. I'm not wedded to this design - please don't hesitate to let
me know if you think of something better. 

> > In my patchset so far, I'm storing AL in c15_ccnt and D in
> > c15_ccnt_delta during normal operation and AL/AC in c15_ccnt, UL/UC in
> > c15_ccnt_delta between op_start and op_finish (note that AL == AC, UL ==
> > UC, and D == 0 during this time). Also note that UL/UC are only stored
> > between op_start and op_finish calls to avoid losing time, and are not
> > needed to maintain architectural state across migrations.
> 
> > Now for migration - The read/writefn's already marshal/unmarshal both
> > the counter value and overflow state to their architectural values in
> > PMCCNTR and PMOVSCLR. I think this would work on its own if we made
> > two big (and wrong) assumptions:
> >         1. PMOVSCLR is read after PMCCNTR on save (ensures all overflows
> >            which have occurred are recorded in PMOVSCLR) and written before
> >            it on load (ensures any overflow bits in PMOVSCLR cause the
> >            interrupt line to be raised)
> 
> There is indeed no guaranteed ordering in which registers are saved/loaded.
> Also, the load function mustn't raise interrupt lines: it has to transfer
> the state just of this device, and trust that the device on the other
> end restores its state into one that matches the current level of the
> interrupt line.

Okay, got it.

> >         2. Time (as measured by `qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)`) 
> > does
> >            not change during the process of saving or loading CP registers
> >            (ensures PMCCNTR can't have overflowed between when it was saved
> >            and when PMOVSCLR was later saved).
> 
> I think this assumption is true, isn't it? (VM migration happens while
> the VM is stopped, and do_vm_stop() calls cpu_disable_ticks() which
> means QEMU_CLOCK_VIRTUAL won't advance.)

Ah, okay. I wasn't aware of this detail.

-Aaron

[1] - http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06843.html