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

[Peter Maydell]

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

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

thanks
-- PMM