[Qemu-devel] [PATCH 0/2] fix of preserving link status

[Qemu-devel] [PATCH 0/2] fix of preserving link status

[Amos Kong]

Set link down and reboot guest, e1000 link status will be re-set
to up by auto-negotiation, it's a regression bug. rtl8139 link
status would be reset to up always.

Problems are fixed by those two patches.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=890288

Amos Kong (2):
  e1000: no need auto-negotiation if link was down
  rtl8139: preserve link state across device reset

 hw/e1000.c   |    5 +++++
 hw/rtl8139.c |    3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
Amos Kong

[Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Amos Kong]

Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
auto-negotiation emulation, it would always set link up by
callback function. Problem exists if original link status
was down, link status should not be changed in auto-negotiation.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/e1000.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/e1000.c b/hw/e1000.c
index 92fb00a..eebcd1d 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -164,6 +164,11 @@ 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.10.1

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Stefan Hajnoczi]

On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
> auto-negotiation emulation, it would always set link up by
> callback function. Problem exists if original link status
> was down, link status should not be changed in auto-negotiation.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/e1000.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 92fb00a..eebcd1d 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -164,6 +164,11 @@ 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;

Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
The code doesn't but I wonder if we should.

Stefan

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Jason Wang]

On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote:
> On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
>> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
>> auto-negotiation emulation, it would always set link up by
>> callback function. Problem exists if original link status
>> was down, link status should not be changed in auto-negotiation.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  hw/e1000.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/e1000.c b/hw/e1000.c
>> index 92fb00a..eebcd1d 100644
>> --- a/hw/e1000.c
>> +++ b/hw/e1000.c
>> @@ -164,6 +164,11 @@ 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;
> Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
> The code doesn't but I wonder if we should.

Not in this case I think. The hack of the auto-negotiation was used to
prevent the irq to be injected before the handler is registered in
windows guest. So an irq would be raised here if we do this which breaks
the hack.
>
> Stefan

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Amos Kong]

On Sat, Jan 05, 2013 at 04:45:14PM +0800, Jason Wang wrote:
> On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote:
> > On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
> >> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
> >> auto-negotiation emulation, it would always set link up by
> >> callback function. Problem exists if original link status
> >> was down, link status should not be changed in auto-negotiation.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> Signed-off-by: Amos Kong <akong@redhat.com>
> >> ---
> >>  hw/e1000.c |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/e1000.c b/hw/e1000.c
> >> index 92fb00a..eebcd1d 100644
> >> --- a/hw/e1000.c
> >> +++ b/hw/e1000.c
> >> @@ -164,6 +164,11 @@ 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;
> > Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
> > The code doesn't but I wonder if we should.
> 
> Not in this case I think. The hack of the auto-negotiation was used to
> prevent the irq to be injected before the handler is registered in
> windows guest. So an irq would be raised here if we do this which breaks
> the hack.

In e1000_open(), after enable irq of adapter, driver will fire a link status
change interrupt to start a watchdog, which will update the link status in
system.

After auto-nego complete, the irq of adapter is still not enabled, the
early interrupt will not work.

So current code is ok.

Thanks, Amos

> > Stefan

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Stefan Hajnoczi]

On Sun, Jan 06, 2013 at 01:11:49PM +0800, Amos Kong wrote:
> On Sat, Jan 05, 2013 at 04:45:14PM +0800, Jason Wang wrote:
> > On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote:
> > > On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
> > >> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
> > >> auto-negotiation emulation, it would always set link up by
> > >> callback function. Problem exists if original link status
> > >> was down, link status should not be changed in auto-negotiation.
> > >>
> > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >> Signed-off-by: Amos Kong <akong@redhat.com>
> > >> ---
> > >>  hw/e1000.c |    5 +++++
> > >>  1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/hw/e1000.c b/hw/e1000.c
> > >> index 92fb00a..eebcd1d 100644
> > >> --- a/hw/e1000.c
> > >> +++ b/hw/e1000.c
> > >> @@ -164,6 +164,11 @@ 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;
> > > Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
> > > The code doesn't but I wonder if we should.
> > 
> > Not in this case I think. The hack of the auto-negotiation was used to
> > prevent the irq to be injected before the handler is registered in
> > windows guest. So an irq would be raised here if we do this which breaks
> > the hack.

Then we have to raise the irq in a timer callback just like the existing
code already does.

I'm worried that a guest driver could depend on the LSC interrupt.

> 
> In e1000_open(), after enable irq of adapter, driver will fire a link status
> change interrupt to start a watchdog, which will update the link status in
> system.
> 
> After auto-nego complete, the irq of adapter is still not enabled, the
> early interrupt will not work.
> 
> So current code is ok.

It's okay for the specific guest driver that you're thinking of.  But
emulation code should reflect how a real device behaves.  That way it
can work with other guest drivers too.

The question is: does a real device raise LSC when setting the
MII_SR_AUTONEG_COMPLETE bit in the PHY_STATUS register?

I found no definite answer in the datasheet but I suspect it does.  If
you have a real e1000 could you test it?

Stefan

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Amos Kong]

On Mon, Jan 07, 2013 at 01:59:54PM +0100, Stefan Hajnoczi wrote:
> On Sun, Jan 06, 2013 at 01:11:49PM +0800, Amos Kong wrote:
> > On Sat, Jan 05, 2013 at 04:45:14PM +0800, Jason Wang wrote:
> > > On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote:
> > > > On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
> > > >> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
> > > >> auto-negotiation emulation, it would always set link up by
> > > >> callback function. Problem exists if original link status
> > > >> was down, link status should not be changed in auto-negotiation.
> > > >>
> > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > >> Signed-off-by: Amos Kong <akong@redhat.com>
> > > >> ---
> > > >>  hw/e1000.c |    5 +++++
> > > >>  1 file changed, 5 insertions(+)
> > > >>
> > > >> diff --git a/hw/e1000.c b/hw/e1000.c
> > > >> index 92fb00a..eebcd1d 100644
> > > >> --- a/hw/e1000.c
> > > >> +++ b/hw/e1000.c
> > > >> @@ -164,6 +164,11 @@ 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;
> > > > Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
> > > > The code doesn't but I wonder if we should.
> > > 
> > > Not in this case I think. The hack of the auto-negotiation was used to
> > > prevent the irq to be injected before the handler is registered in
> > > windows guest. So an irq would be raised here if we do this which breaks
> > > the hack.
> 
> Then we have to raise the irq in a timer callback just like the existing
> code already does.
> 
> I'm worried that a guest driver could depend on the LSC interrupt.
> 
> > 
> > In e1000_open(), after enable irq of adapter, driver will fire a link status
> > change interrupt to start a watchdog, which will update the link status in
> > system.
> > 
> > After auto-nego complete, the irq of adapter is still not enabled, the
> > early interrupt will not work.
> > 
> > So current code is ok.
> 
> It's okay for the specific guest driver that you're thinking of.  But
> emulation code should reflect how a real device behaves.  That way it
> can work with other guest drivers too.
> 
> The question is: does a real device raise LSC when setting the
> MII_SR_AUTONEG_COMPLETE bit in the PHY_STATUS register?
> 
> I found no definite answer in the datasheet but I suspect it does.  If
> you have a real e1000 could you test it?

Hi Stefan,

I don't have e1000 (82540EM) in hand, and just tested with e1000e (82567LM-3)
This is the debug message:

| >>> setup autoneg: icr & E1000_ICR_LSC: 0
| >>> autoneg completed, icr & E1000_ICR_LSC: 0
| >>> setup autoneg: icr & E1000_ICR_LSC: 0
| >>> autoneg completed, icr & E1000_ICR_LSC: 0

No interrupt after auto-nego completed

| e1000e 0000:00:19.0: irq 49 for MSI/MSI-X
| e1000e 0000:00:19.0: irq 49 for MSI/MSI-X

irq is enabled

| >>> e1000_open: before fire an interrupt, icr & E1000_ICR_LSC: 0
^^^
ICR_LSC bit doesn't change by hardware

Software driver changes ICR_LSC bit to fire a interrupt

| >>> e1000_open: after fire an interrupt, icr & E1000_ICR_LSC: 4

| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4
^^ handle this interrupt

| IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4

^^^
E1000_ICR_LSC is changed by hardware and caused an interrupt
Our e1000 backend driver doesn't raise this interrupt.
It seems a interrupt should be raise by backend driver, but we don't
know what's the right time/point.

| e1000e: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None
| IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
| >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
| >>> set link up in watchdog task, icr & E1000_ICR_LSC: 0



In OpenSDM_8254x-37.pdf:

| ++ PHY Initialization (10/100/1000 Mb/s Copper Media)
| Once link is achieved by the PHY, software is notified when a Link
| Status Change (LSC) interrupt is generated by the Ethernet controller. 

"link is achieved by the PHY" == "auto-nego completes" ?

| + 8.6.5.2 Internal PHY Mode
| While in internal PHY mode, an internal signal provides status of the
| physical link as indicated by
| the PHY. Indication that the link is not up disables MAC operation.
| Upon determination of a valid
| link, the assertion of the internal link signal asserts the LSC
| interrupt (if enabled) to indicate to the software driver to check the link status.

Is it lost in our backend driver?


I will try to find a e1000 real nic to re-test.

Thanks, Amos

Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down

[Stefan Hajnoczi]

On Tue, Jan 08, 2013 at 05:45:30PM +0800, Amos Kong wrote:
> On Mon, Jan 07, 2013 at 01:59:54PM +0100, Stefan Hajnoczi wrote:
> > On Sun, Jan 06, 2013 at 01:11:49PM +0800, Amos Kong wrote:
> > > On Sat, Jan 05, 2013 at 04:45:14PM +0800, Jason Wang wrote:
> > > > On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote:
> > > > > On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote:
> > > > >> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link
> > > > >> auto-negotiation emulation, it would always set link up by
> > > > >> callback function. Problem exists if original link status
> > > > >> was down, link status should not be changed in auto-negotiation.
> > > > >>
> > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > >> Signed-off-by: Amos Kong <akong@redhat.com>
> > > > >> ---
> > > > >>  hw/e1000.c |    5 +++++
> > > > >>  1 file changed, 5 insertions(+)
> > > > >>
> > > > >> diff --git a/hw/e1000.c b/hw/e1000.c
> > > > >> index 92fb00a..eebcd1d 100644
> > > > >> --- a/hw/e1000.c
> > > > >> +++ b/hw/e1000.c
> > > > >> @@ -164,6 +164,11 @@ 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;
> > > > > Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes?
> > > > > The code doesn't but I wonder if we should.
> > > > 
> > > > Not in this case I think. The hack of the auto-negotiation was used to
> > > > prevent the irq to be injected before the handler is registered in
> > > > windows guest. So an irq would be raised here if we do this which breaks
> > > > the hack.
> > 
> > Then we have to raise the irq in a timer callback just like the existing
> > code already does.
> > 
> > I'm worried that a guest driver could depend on the LSC interrupt.
> > 
> > > 
> > > In e1000_open(), after enable irq of adapter, driver will fire a link status
> > > change interrupt to start a watchdog, which will update the link status in
> > > system.
> > > 
> > > After auto-nego complete, the irq of adapter is still not enabled, the
> > > early interrupt will not work.
> > > 
> > > So current code is ok.
> > 
> > It's okay for the specific guest driver that you're thinking of.  But
> > emulation code should reflect how a real device behaves.  That way it
> > can work with other guest drivers too.
> > 
> > The question is: does a real device raise LSC when setting the
> > MII_SR_AUTONEG_COMPLETE bit in the PHY_STATUS register?
> > 
> > I found no definite answer in the datasheet but I suspect it does.  If
> > you have a real e1000 could you test it?
> 
> Hi Stefan,
> 
> I don't have e1000 (82540EM) in hand, and just tested with e1000e (82567LM-3)
> This is the debug message:
> 
> | >>> setup autoneg: icr & E1000_ICR_LSC: 0
> | >>> autoneg completed, icr & E1000_ICR_LSC: 0
> | >>> setup autoneg: icr & E1000_ICR_LSC: 0
> | >>> autoneg completed, icr & E1000_ICR_LSC: 0
> 
> No interrupt after auto-nego completed
> 
> | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X
> | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X
> 
> irq is enabled
> 
> | >>> e1000_open: before fire an interrupt, icr & E1000_ICR_LSC: 0
> ^^^
> ICR_LSC bit doesn't change by hardware
> 
> Software driver changes ICR_LSC bit to fire a interrupt
> 
> | >>> e1000_open: after fire an interrupt, icr & E1000_ICR_LSC: 4
> 
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4
> ^^ handle this interrupt
> 
> | IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4
> 
> ^^^
> E1000_ICR_LSC is changed by hardware and caused an interrupt
> Our e1000 backend driver doesn't raise this interrupt.
> It seems a interrupt should be raise by backend driver, but we don't
> know what's the right time/point.
> 
> | e1000e: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None
> | IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
> | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0
> | >>> set link up in watchdog task, icr & E1000_ICR_LSC: 0
> 
> 
> 
> In OpenSDM_8254x-37.pdf:
> 
> | ++ PHY Initialization (10/100/1000 Mb/s Copper Media)
> | Once link is achieved by the PHY, software is notified when a Link
> | Status Change (LSC) interrupt is generated by the Ethernet controller. 
> 
> "link is achieved by the PHY" == "auto-nego completes" ?

"Link is achieved" is more general than just auto-negotiation, I think
it also occurs when you force a specific link speed (no
autonegotiation).  The host still wants to know if the network cable is
plugging in or not :).

> | + 8.6.5.2 Internal PHY Mode
> | While in internal PHY mode, an internal signal provides status of the
> | physical link as indicated by
> | the PHY. Indication that the link is not up disables MAC operation.
> | Upon determination of a valid
> | link, the assertion of the internal link signal asserts the LSC
> | interrupt (if enabled) to indicate to the software driver to check the link status.
> 
> Is it lost in our backend driver?

My interpretation is that hw/e1000.c should raise the LSC interrupt
whenever the link state changes or when forced to restart
auto-negotiation.

> I will try to find a e1000 real nic to re-test.

Great, thanks!

Stefan

[Qemu-devel] [PATCH 2/2] rtl8139: preserve link state across device reset

[Amos Kong]

A device reset does not affect the link state, only set_link does.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/rtl8139.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index c59ec6b..3e08062 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -1258,7 +1258,8 @@ static void rtl8139_reset(DeviceState *d)
     s->BasicModeStatus  = 0x7809;
     //s->BasicModeStatus |= 0x0040; /* UTP medium */
     s->BasicModeStatus |= 0x0020; /* autonegotiation completed */
-    s->BasicModeStatus |= 0x0004; /* link is up */
+    /* preserve link state */
+    s->BasicModeStatus |= s->nic->nc.link_down ? 0 : 0x04;
 
     s->NWayAdvert    = 0x05e1; /* all modes, full duplex */
     s->NWayLPAR      = 0x05e1; /* all modes, full duplex */
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH 0/2] fix of preserving link status

[Stefan Hajnoczi]

On Fri, Dec 28, 2012 at 05:29:09PM +0800, Amos Kong wrote:
> Set link down and reboot guest, e1000 link status will be re-set
> to up by auto-negotiation, it's a regression bug. rtl8139 link
> status would be reset to up always.
> 
> Problems are fixed by those two patches.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=890288
> 
> Amos Kong (2):
>   e1000: no need auto-negotiation if link was down
>   rtl8139: preserve link state across device reset
> 
>  hw/e1000.c   |    5 +++++
>  hw/rtl8139.c |    3 ++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> -- 
> Amos Kong
> 

Thanks, applied to the net tree:
https://github.com/stefanha/qemu/commits/net

Stefan