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:53:22PM +0800, Amos Kong wrote:
> 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?
> > 
> > I tried this test with current qemu code, link status is not reseted
> > to 'up' after step 3. Is it the problem you said?
> > 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).
> 
> Is it expected?
> 
> When we reset the virtual system, do we need to reset the status
> of simulation of network cable?

I don't so. If we "unplug the cable", it should stay unplugged when we
reset the machine.

When we reset the machine, we should set the NIC's link status in
accordance with it's NetClient peer. So if that's what your patch was
attempting to enforce I think we're in agreement there.

I think the problem with your original patch is that it doesn't check
the link status of the nic's NetClient peer, but instead checks it's
own internal NetClient state, so we end up trying to enforce "leave the
cable unplugged" even when "set_link down" was never issued.

And if that's the case I think you can re-spin your original patch for
1.4 accordingly. But we definitely need to revert the current one ASAP
since it breaks all QEMU guests that are started using the default nic.

> I think it's deciced by that if we think simulation of network
> cable is a part of the virtual machine.
> 
> ----
> commit 436e5e53c97d8fb469306b18a0c31dc60f5e546c
> Author: aliguori <address@hidden>
> Date:   Thu Jan 8 19:44:06 2009 +0000
> 
>     Add 'set_link' monitor command (Mark McLoughlin)
>     
>     Add a monitor command to setting a given network device's link status
>     to 'up' or 'down'.
>     
>     Allows simulation of network cable disconnect.
>     
>     Signed-off-by: Mark McLoughlin <address@hidden>
>     Signed-off-by: Anthony Liguori <address@hidden>
>