Re: [Qemu-devel] [PATCH for-1.4] Revert "e1000: no need auto-negotiation if link was down"

[mdroth]

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 <address@hidden>
> > ---
> >  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
>