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

[Peter Crosthwaite]

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:
>> >> +    } else {
>> >> +        mii->bmsr &= ~MII_BMSR_LINK_ST;
>> >> +        mii->anlpar = MII_ANAR_TX;
>> >> +    }
>> >> +    mii->link_ok = link_ok;
>> >> +}
>> >> +
>> >> +static void mii_reset(AwEmacMii *mii)
>> >> +{
>> >> +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
>> >> +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
>> >> +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
>> >> +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | 
>> >> MII_ANAR_10 |
>> >> +                MII_ANAR_CSMACD;
>> >> +    mii->anlpar = MII_ANAR_TX;
>> >> +    mii_set_link(mii, mii->link_ok);
>> >
>> > If this is actually needed, it smells like a bug in the net layer. Is
>> > the net layer not doing it's resets properly? It's quite odd that you
>> > are preserving old state across a reset.
>
> The net layer doesn't have a reset point for the link status.  The net
> client must explicitly transition states itself.
>
>> >> +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
>> >> +                               size_t size)
>> >> +{
>> >> +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> >> +    AwEmacFifo *fifo;
>> >> +    uint32_t crc, *word;
>> >> +    uint8_t *dest;
>> >> +
>> >> +    if (s->num_rx >= NUM_RX_FIFOS) {
>> >
>> > Seems inconsistent that you check for fifo vacancy but you dont check
>> > for s->ctl & EMAC_CTL_RX_EN (as above in can_recieve). Then again,
>> > both conditions should be guarded by can_recieve, so I wonder whether
>> > you can just drop this.
>> >
>> > Stefan, if you are reading, can the recieve function rely on
>> > can_recieve success and drop such checks?
>
> It is safer to perform full checks in .receive() since
> qemu_net_queue_flush() calls .receive() repeatedly without
> .can_receive().  This means especially if the state can change between
> calls, you need to do a full check.
>
>> >> +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.

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.

Regards,
Peter