On 9/21/18 5:37 PM, Peter Maydell wrote:
> On 21 September 2018 at 06:39, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> On 09/19/2018 11:30 PM, Peter Maydell wrote:
>>> There are several possible approaches here I think:
>>>
>>>  (1) the "clock" object holds no internal state; if a device on the
>>> destination end of a clock connection cares about clock state then
>>> it keeps and updates a copy of that state when the callback is called,
>>> and it is responsible for migrating that copy along with all its other
>>> state. This is how qemu_irq/gpio lines work.
>>>  (2) the "clock" object does hold internal state, and it is owned
>>> by the source-end device, which is responsible for migrating that
>>> state. This is how ptimer objects work -- hw/core/ptimer.c defines
>>> a vmstate struct, but it is the devices that use a ptimer that
>>> put a VMSTATE_PTIMER entry in their vmstate structs to migrate the data.
>>>  (3) the "clock" object can be a fully fledged device (ie a subclass
>>> of TYPE_DEVICE) which migrates its state entirely by itself.
>>>
>>> I don't have a firm view currently on which would be best here,
>>> but I guess I lean towards 2. 1 has the advantage of "just like
>>> qemu_irq" but the disadvantage that the destination end has no
>>> way to query the current clock value so has to manually track it
>>> itself. 3 is probably overkill here (and also makes it hard to
>>> retain migration backward compatibility when adding clock tree
>>> support to an existing machine model).
>>
>> I agree with you on doing approach 2. If the clock state needs to be at
>> the end, it seems best to put in inside the clock object. It will save
>> codelines in devices. Thanks for the tips about ptimer.
>>
>> I don't see how approach 3 solves the problem since the clock state will
>> still be migrated by another object (instead of begin the device which
>> generate the clock, it is now the clock input object). So a device (with
>> an input clock) has no guarantee on the clock value being correct when
>> it will handle its own migration. I think the clock vmstate entry needs
>> to be present in the device's vmsd (or am I missing something ?).
> 
> The point about (3) is that every TYPE_DEVICE object manages migration
> of its own state, so the device which has the clock output does not
> need to.
> 
> No device should ever care about whether other devices in the system
> have had their state loaded on a migration or not yet: their migration
> load must affect only their own internal state. If you find yourself in
> a position where you need to care then you've probably got some part of
> the design wrong.
> 
> (The difference between 2 and 3 is that in 2 the clock-object is not
> a full device, so it's just a part of the output-end device and the
> output-end device does its state migration. In 3 it is a full device
> and does its own migration.)
> 
>> Regarding backward compatibility on migration, I think we have 2 options:
>> (A) keep updating outputs clocks in post_load.
>> (B) rely on device with an input clock to setup a "good" default value
>> to unmigrated input clocks.
> 
> I think what you need (assuming a type 2 design) is for there to be a
> function on the clock object which says "here's your state, but don't
> tell the output end" (or just directly set the clock struct fields).
> That way the output end device can in its post-load function use that
> if it is doing a migration from an old version that didn't include
> the clock device.
> 
> The input end device can't help you because it is not in a position
> to change the state of the clock object, which belongs to the output
> end. (Consider also the case where one clock connects to multiple
> inputs -- the input end can't set the value, because the different
> inputs might have different ideas of the right thing.)

I was thinking of putting a state in the input clock so that it belongs
to the input end device. This would be some kind of cache of the value
and it will be loaded by the input device during migration.
If there are multiple inputs, each input will migrate its own copy.
Every copy should be identical and no action needs to be performed on
the output side on migration apart from updating its own clock state
(but I think it doesn't need one if every input has one).

During a migration, if the state was not in the source vm (migration
from an old qemu), this local copy can be initialized to a default value
by the input device. This value would be eventually updated later on if
the clock is changed at the output end.
In case there are several inputs, different values will maybe exist in
the different input end devices until the output end does an update.

For example, in the zynq, the cadence_uart today consider it has a 50Mhz
reference clock. This could be its default value on migration, whatever
the frequency set by the output. In that case, after migration, the uart
will continue working like before migration until the software does a
clock configuration (which may never happen).

It has the advantage to be simple (everything stay in the input end
perimeter) and independent of migration order.

We can still try to fetch the right value from the output instead of
putting a default value, but its a fifty-fifty chance of having the
value before migration, which means default value is imposed by the
output. Or the output can silently put a value like you said, but I
think the result depends on the migration order as well.

A problem I see with having the input doing the migration if the output
end "changed" because of a migration.
Consider a bug-fix in clock computation or a static clock whose
frequency has changed between the 2 versions of qemu. What are the
acceptable results after the migration ?

My initial point of view was to consider:
(1) the clock frequency belongs the clock controller device (ie: the
output end device) and is migrated by it.
(2) a clocked device (ie: an input end device) has a state which does
not contains the clock frequency.
(3) if the clocked device needs the clock frequency to compute some
things (eg: backend config in a uart, or visibility of mmio), it will do
the computation as many time as required until its inputs and its state
are up-to-date. In fact, some kind of clock propagation side-effect.

I can try to find a way to delay this computation until everything
(inputs + state) is up-to-date to be independent of migration order. But
I think there will be corner cases where the computation will never occurs.
If we had some kind of post_load_delayed_until_end_of_migration, it
would be easy, but I don't think we have that.

> 
>>> I don't really understand why reset is related here. Clock trees and
>>> reset domains don't sit in a 1-to-1 relationship, generally. Reset
>>> is a complicated and painful area and I think I would prefer to see
>>> a patchset which aimed to solve the clocktree modelling problem
>>> without dragging in the complexities of reset modelling.
>>
>> OK.
>>
>> Do you think I should do a reroll right now with this 2 modifications
>> without waiting further review ?
> 
> I think that's probably a good idea, yes.
> 
> (I have been thinking a bit about the reset problem this week
> and will see if I can write up my thoughts on it next week.)

Regarding the reset, the functionality I added can be implemented using
gpio. What I did is just a factorization to avoid having to connect a
clock and a gpio for the reset.

> 
> thanks
> -- PMM
>