qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alistair Francis
Subject: Re: [Qemu-devel] [PATCH arm-ccnt v1 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
Date: Mon, 20 Jan 2014 11:11:55 +1000

I have made those changes you both mentioned above and submitted v2 of
my series. There is now only one extra variable in the CPUARMState
struct.

On Sun, Jan 19, 2014 at 9:20 PM, Peter Maydell <address@hidden> wrote:
> On 19 January 2014 01:39, Peter Crosthwaite
> <address@hidden> wrote:
>> On Sun, Jan 19, 2014 at 11:06 AM, Peter Maydell
>> <address@hidden> wrote:
>>  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?
>
> It would break migration between TCG and KVM -- the assumption
> is that cp registers only need to be read and written via the
> raw_read/write interface. There is obviously a mechanism for
> migrating ARM state that isn't a coprocessor (you put an entry
> for it in the vmstate structs in machine.c), but you need to
> figure out what that state looks like for KVM as well and special
> case it in the KVM state syncing functions.
>
>>>> 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.
>
> I would assume that frequency doesn't change much, so to the
> extent we care about efficiency we should make the read
> path simple and do recalculation on frequency changes.
>
>> 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:
>
> Yeah, if you like. (Actually I think it's ok to have more state in the
> CPU struct, as long as it's always valid to save and restore just
> by saving and restoring the value through the raw accessors.
> If you really want to have two fields then you should put them both
> in the cp15 bit of the struct though; compare the handling of the
> generic timer cp14_timer fields.) Your comment seems a bit
> longwinded to me, though, since from my POV this is just the
> obvious way to implement a counter.
>
> thanks
> -- PMM
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]