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 <address@hidden>
> > > >> Signed-off-by: Amos Kong <address@hidden>
> > > >> ---
> > > >>  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