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 <mdr...@linux.vnet.ibm.com>
> > ---
> > 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
>
