Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Thu, Jan 16, 2014 at 12:45 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Wed, Jan 15, 2014 at 06:24:24PM +1000, Peter Crosthwaite wrote:
>> On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <address@hidden> wrote:
>> > On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> >> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> >> <address@hidden> wrote:
>> >> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <address@hidden> 
>> >> > wrote:
>> >> >> +static const VMStateDescription vmstate_mii = {
>> >> >> +    .name = "allwinner_emac_mii",
>> >> >> +    .version_id = 1,
>> >> >> +    .minimum_version_id = 1,
>> >> >> +    .fields = (VMStateField[]) {
>> >> >> +        VMSTATE_UINT16(bmcr, AwEmacMii),
>> >> >> +        VMSTATE_UINT16(bmsr, AwEmacMii),
>> >> >> +        VMSTATE_UINT16(anar, AwEmacMii),
>> >> >> +        VMSTATE_UINT16(anlpar, AwEmacMii),
>> >> >> +        VMSTATE_BOOL(link_ok, AwEmacMii),
>> >> >
>> >> > The net layer should probably properly set link_ok on realize, so I
>> >> > doubt it's migratable state.
>> >
>> > The net layer is not part of live migration.  It's up to the emulated
>> > device to migrate link state and restore it after migration.  In other
>> > words, link state should be migrated as part of PHY vmstate.
>> >
>>
>> But is the saved link state meaningful? The fact that net is not
>> migrating means that this link_ok migrated variable is potentially
>> incoherent with the net layer. E.G. What happens in the following
>> case:
>>
>> Run QEMU
>> Link is Good
>> Save
>>
>> Run Again
>> Link is bad
>> Load
>>
>> The loaded emulation will have link_ok due to the vmsd load despite
>> the link being down in the new net layer environment. It will not
>> correct until net-layer activity causes a ->link_state_changed
>> callback.
>
> I would agrue that this example is not valid.  NetClient->link_down is
> influenced by two things:
> 1. User actions that disable the link (e.g. setting link status)
> 2. Backends may take the link down, e.g. if the net/socket.c connection
>    breaks.
>
> In either case, the guest should migrate with its original link status.
> Then, when it resumes on the destination host the link state may change
> or an interrupt can be raised.
>

Will you reliably get your link_state_changed callback post_load though?

>> Can this be just solved cleanly by calling the mii_set_link fn (added
>> in this patch) always on post load and removing this link_ok state?
>> This will mean that guest will see a link state transition immediately
>> on load if the link state changes from the saved-machine state to the
>> loaded-machine state.
>
> Is the link_ok field even necessary?

No. Thats my theory underpinning my proposed change anyway.

> Perhaps we should just rely on
> NetClient->link_ok.
>

I think we are in agreement then? There is no need for phy.link_ok as
same information is captured in NetClient. This means there is no
link_ok field to migrate. Just migrate the phy registers themselves.

For completeness you just need to poll NetClient state post_load which
will nicely handle my degenerate case of link activity between save
and load (same handler as link_state_changed). In the more usual case
of the link state not changing between save and load, this function
call will just be a nop, leading to no change of state on the freshly
migrated phy registers.

Regards
Peter