From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U1Hd5-0008NR-IB for qemu-devel@nongnu.org; Fri, 01 Feb 2013 09:30:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U1Hd1-00016s-Cp for qemu-devel@nongnu.org; Fri, 01 Feb 2013 09:30:11 -0500 Sender: fluxion Date: Fri, 1 Feb 2013 08:29:48 -0600 From: mdroth Message-ID: <20130201142948.GA2379@vm> References: <1359675832-11999-1-git-send-email-mdroth@linux.vnet.ibm.com> <20130201072059.GA12824@t430s.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130201072059.GA12824@t430s.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH for-1.4] Revert "e1000: no need auto-negotiation if link was down" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: jasowang@redhat.com, qemu-stable@nongnu.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, mst@redhat.com On Fri, Feb 01, 2013 at 03:20:59PM +0800, Amos Kong wrote: > On Thu, Jan 31, 2013 at 05:43:51PM -0600, Michael Roth wrote: > > This reverts commit 84dd2120247a7d25ff1bb337de21c0e76816ad2d. > > > > I'm not sure what issue the original commit was meant to fix, or if > > the logic is actually wrong, but it causes e1000 to stop working > > after a guest issues a reset. > > Hi Michael, > > What's your test scenario? Nothing special, I just started a guest with -net nic,model=e1000 -net user or -net nic,model=e1000 -net tap and networking stopped working (could not lease an IP, no outbound traffic) after I rebooted. > > I tried this test with current qemu code, link status is not reseted > to 'up' after step 3. Is it the problem you said? I think it's related, but I'm not so much concerned with the qmp-visible link status changing as I am with the guest. > This problem also exists with current virtio (existed in the past) / > rtl8139 (introduced in 83f58e570f21c3e7227e7fbef1fc0e18b5ed7ea9) > > 1) boot a guest with e1000 nic > 2) set link down in monitor > (hmp) set_link e1000.0 down > 3) reset guest by 'system_reset' in monitor > (hmp) system_reset > > > My original patch is used to restore the link status after guest > reboot(execute 'reboot' insider guest system). > The link status should always be up after virtual 'hardware' reset > (execute 'system_reset' in monitor). You sure you don't have that backwards? It seems to me that your original patch was meant to *prevent* the link status from changing after a system reset, which makes sense from the perspective of a qmp-issued "set_link down" meaning "unplug the cable". > > Thanks, Amos > > > >From what I can tell a guest with an e1000 nic has no way of changing > > the link status, as far as it's NetClient peer is concerned, except > > in the auto-negotiation path, so with this patch in place there's no > > recovery after a reset, since the link goes down and stays that way. > > > > Revert this patch now to fix the bigger problem, and handle any > > lingering issues with a follow-up. > > > > Reproduced/tested with qemu-jeos and Ubuntu 12.10. > > > > Signed-off-by: Michael Roth > > --- > > hw/e1000.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/hw/e1000.c b/hw/e1000.c > > index ef06ca1..563a58f 100644 > > --- a/hw/e1000.c > > +++ b/hw/e1000.c > > @@ -166,11 +166,6 @@ static void > > set_phy_ctrl(E1000State *s, int index, uint16_t val) > > { > > if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) { > > - /* no need auto-negotiation if link was down */ > > - if (s->nic->nc.link_down) { > > - s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE; > > - return; > > - } > > s->nic->nc.link_down = true; > > e1000_link_down(s); > > s->phy_reg[PHY_STATUS] &= ~MII_SR_AUTONEG_COMPLETE; > > -- > > 1.7.9.5 >