Re: [Qemu-devel] [PATCH v4 00/10] Clock framework API.

[Peter Maydell]

On 21 September 2018 at 06:39, Damien Hedde <address@hidden> 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 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.)

thanks
-- PMM