Re: [Qemu-devel] [PATCH arm-ccnt v1 1/1] ARM-CCNT: Implements the ARM PMCCNTR register

[Peter Crosthwaite]

On Sun, Jan 19, 2014 at 11:06 AM, Peter Maydell
<address@hidden> wrote:
> On 19 January 2014 00:48, Peter Crosthwaite
> <address@hidden> wrote:
>> On Sun, Jan 19, 2014 at 8:01 AM, Peter Maydell <address@hidden> wrote:
>>> On 16 January 2014 04:31, Alistair Francis <address@hidden> wrote:
>>>>      env->cp15.c9_pmcr &= ~0x39;
>>>>      env->cp15.c9_pmcr |= (value & 0x39);
>>>> +
>>>> +    if (value & PMCRC) {
>>>> +        /* Reset the counter */
>>>> +        env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> +        env->cp15.c15_ccnt = 0;
>>>
>>> This is the wrong way to do this kind of thing. Don't store
>>> the actual value of a resettable count register; store the
>>> difference between the CLOCK_VIRTUAL count and
>>> the guest-visible value. Then you don't have to separately
>>> keep both emm_time and c15_ccnt as state.
>>>
>>
>> Is the state minimisation really worth that pain?
>
> You have to do calculation on read and write anyway, because
> obviously we don't keep the state field continuously updated
> to the value the guest needs to see.  (Actually the lack of
> a writefn hook for this register looks like a bug to me.)

Yes it should be easy enough.

 So it doesn't
> IMHO make much difference in terms of code complexity, and it
> keeps the CPUState tidy (in particular, you don't end up with a
> random field which is "this pertains to a single cp15 register, and
> it holds state, but it's lurking randomly at the top level"). I think
> the migration is really the key issue though: if you design the
> state such that simply writing the incoming-migration value
> to the register via the raw-write hook (and reading it via
> raw-read on an outbound migration) is sufficient, then it all
> just works. If you have this extra bit of state lurking about then
> it is all going to need nasty special casing, and I went to quite
> a lot of effort to make sure that cp15 registers didn't need
> individual special casing.
>

FWIW, is there anything wrong with just migrating the second variable
on the side? Is there no code path for migration of random ARM state
thats not a CP?

>> If only reset was
>> being implemented, it could be done as a simple case of snapping
>> clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state
>> entirely but the problem is it does make the implementation of the
>> disable feature somewhat difficult. If the timer is disabled this
>> "diff" needs to start changing as emulation time progresses. If I
>> disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to
>> recalulate this diff -= 1. The only way that calculation can be done
>> is with a second variable, which snapshots CLOCK_VIRTUAL when disabled
>> happened so this diff can be recalculated. Its also awkward as if you
>> disable for long enough (or immediately on emulation start),
>> "clock_when_ccnt_reset" can go negative.
>
> Typically for a disablable counter, when it's disabled you keep
> "current disabled value" in the state field, and then when it's
> reenabled you switch back to "diff from clock".
>

Ouch. Thats a variable with mixed meaning. You still have the
amkwardness however of this diff needing warping every time you change
the frequency. And it can go negative when you change from high
frequency to low frequency w/o a reset. Although none of this is a
functional showstopper I guess.

So to run with your idea, the register can be defined as a dual
meaning uint64_t to simplify migration but a comment should be added
to the effect that this value does represent the actual register
state:

/*
 * NOT always the value of the counter. If the counter
 * is enabled, it is the virtual clock time corresponding
 * to when the counter was reset. If the counter is disabled,
 * it is value of the counter.
 */
uint64_t c15_ccnt;

Regards,
Peter

> thanks
> -- PMM
>