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 <jasow...@redhat.com> > > > > >> Signed-off-by: Amos Kong <ak...@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