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

[Damien Hedde]

On 09/19/2018 11:30 PM, Peter Maydell wrote:
> On 17 September 2018 at 01:40,  <address@hidden> wrote:
>> Regarding the migration strategy, clocks do not hold the clock state
>> internally, so there is nothing to migrate there. The consequence is that
>> a device must update its output clocks in its post_load to propagate the
>> migrated clock state. This allows migration from old-qemu-with-no-clock
>> to new-qemu-with-clock-support: newly added clocks will be correctly
>> initialized during migration.
>> But it is more complex for input clocks handling: there is no order
>> guarantee between a device state migration and the update of its inputs 
>> clocks
>> which will occur during other device state migrations.
>> I think that, for most the cases, this does not rise problems, although there
>> might be some jitter/glitch during migration before hitting the right value
>> (with consequences such as the baudrate of a character device changing 
>> several
>> times during migration, I don't think it is a problem but may well be wrong
>> here).
> 
> This doesn't seem like a good idea to me, since as you say there is
> no guarantee on migration order. It breaks a general principle that
> devices should migrate their own state and not do anything that
> disturbs other devices.
> 
> 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 ?).

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.

(A) has the advantage of ensuring an unmigrated input clock have right
value just at the end of backward migration. But there's still the
migration order jitter. (B) does not have the jitter, but unmigrated
input clocks will be at their default values after the migration and
will be updated maybe later on.

Given your statement about disturbing other devices, (B) seems the way
to go.

> 
>> Concerning this frequency-reset port, we can obviously go back to the simple
>> frequency-only one if you think it is not a good idea.
> 
> 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 ?

Thanks,
--
Damien