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

[Peter Maydell]

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...

>> 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.

> 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.

>         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.)

thanks
-- PMM