* [Qemu-devel] [PATCH 0/2] fix of preserving link status
@ 2012-12-28 9:29 Amos Kong
2012-12-28 9:29 ` [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down Amos Kong
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Amos Kong @ 2012-12-28 9:29 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, jasowang, stefanha
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2012-12-28 9:29 [Qemu-devel] [PATCH 0/2] fix of preserving link status Amos Kong
@ 2012-12-28 9:29 ` Amos Kong
2013-01-03 12:20 ` Stefan Hajnoczi
2012-12-28 9:29 ` [Qemu-devel] [PATCH 2/2] rtl8139: preserve link state across device reset Amos Kong
2013-01-03 12:21 ` [Qemu-devel] [PATCH 0/2] fix of preserving link status Stefan Hajnoczi
2 siblings, 1 reply; 10+ messages in thread
From: Amos Kong @ 2012-12-28 9:29 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, jasowang, Amos Kong, stefanha
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 2/2] rtl8139: preserve link state across device reset
2012-12-28 9:29 [Qemu-devel] [PATCH 0/2] fix of preserving link status Amos Kong
2012-12-28 9:29 ` [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down Amos Kong
@ 2012-12-28 9:29 ` Amos Kong
2013-01-03 12:21 ` [Qemu-devel] [PATCH 0/2] fix of preserving link status Stefan Hajnoczi
2 siblings, 0 replies; 10+ messages in thread
From: Amos Kong @ 2012-12-28 9:29 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, jasowang, Amos Kong, stefanha
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2012-12-28 9:29 ` [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down Amos Kong
@ 2013-01-03 12:20 ` Stefan Hajnoczi
2013-01-05 8:45 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-01-03 12:20 UTC (permalink / raw)
To: Amos Kong; +Cc: jan.kiszka, jasowang, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] fix of preserving link status
2012-12-28 9:29 [Qemu-devel] [PATCH 0/2] fix of preserving link status Amos Kong
2012-12-28 9:29 ` [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down Amos Kong
2012-12-28 9:29 ` [Qemu-devel] [PATCH 2/2] rtl8139: preserve link state across device reset Amos Kong
@ 2013-01-03 12:21 ` Stefan Hajnoczi
2 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-01-03 12:21 UTC (permalink / raw)
To: Amos Kong; +Cc: jan.kiszka, jasowang, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2013-01-03 12:20 ` Stefan Hajnoczi
@ 2013-01-05 8:45 ` Jason Wang
2013-01-06 5:11 ` Amos Kong
0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2013-01-05 8:45 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: jan.kiszka, Amos Kong, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2013-01-05 8:45 ` Jason Wang
@ 2013-01-06 5:11 ` Amos Kong
2013-01-07 12:59 ` Stefan Hajnoczi
0 siblings, 1 reply; 10+ messages in thread
From: Amos Kong @ 2013-01-06 5:11 UTC (permalink / raw)
To: Jason Wang; +Cc: jan.kiszka, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2013-01-06 5:11 ` Amos Kong
@ 2013-01-07 12:59 ` Stefan Hajnoczi
2013-01-08 9:45 ` Amos Kong
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-01-07 12:59 UTC (permalink / raw)
To: Amos Kong; +Cc: jan.kiszka, Jason Wang, qemu-devel, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2013-01-07 12:59 ` Stefan Hajnoczi
@ 2013-01-08 9:45 ` Amos Kong
2013-01-08 17:07 ` Stefan Hajnoczi
0 siblings, 1 reply; 10+ messages in thread
From: Amos Kong @ 2013-01-08 9:45 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: jan.kiszka, Jason Wang, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down
2013-01-08 9:45 ` Amos Kong
@ 2013-01-08 17:07 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-01-08 17:07 UTC (permalink / raw)
To: Amos Kong; +Cc: jan.kiszka, Jason Wang, qemu-devel, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-01-08 17:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-28 9:29 [Qemu-devel] [PATCH 0/2] fix of preserving link status Amos Kong
2012-12-28 9:29 ` [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down Amos Kong
2013-01-03 12:20 ` Stefan Hajnoczi
2013-01-05 8:45 ` Jason Wang
2013-01-06 5:11 ` Amos Kong
2013-01-07 12:59 ` Stefan Hajnoczi
2013-01-08 9:45 ` Amos Kong
2013-01-08 17:07 ` Stefan Hajnoczi
2012-12-28 9:29 ` [Qemu-devel] [PATCH 2/2] rtl8139: preserve link state across device reset Amos Kong
2013-01-03 12:21 ` [Qemu-devel] [PATCH 0/2] fix of preserving link status Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).