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

[Stefan Hajnoczi]

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.