From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47954) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W2wQ1-0002Ch-L5 for qemu-devel@nongnu.org; Tue, 14 Jan 2014 00:20:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W2wPw-0005GH-IW for qemu-devel@nongnu.org; Tue, 14 Jan 2014 00:20:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7939) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W2wPw-0005G5-8b for qemu-devel@nongnu.org; Tue, 14 Jan 2014 00:20:00 -0500 Date: Tue, 14 Jan 2014 13:19:38 +0800 From: Stefan Hajnoczi Message-ID: <20140114051938.GC4803@stefanha-thinkpad.redhat.com> References: <1389435213-4378-1-git-send-email-b.galvani@gmail.com> <1389435213-4378-2-git-send-email-b.galvani@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Beniamino Galvani , Peter Maydell , "qemu-devel@nongnu.org Developers" , Li Guang On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote: > On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite > wrote: > > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani 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.