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

[Stefan Hajnoczi]

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.

> 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?  Perhaps we should just rely on
NetClient->link_ok.