diff mbox

e1000 autoneg timing, piix/osx

Message ID 53B575D0.7080700@suse.de
State New
Headers show

Commit Message

Alexander Graf July 3, 2014, 3:25 p.m. UTC 
On 03.07.14 16:14, Gabriel L. Somlo wrote:
> On Thu, Jul 03, 2014 at 04:02:06PM +0200, Alexander Graf wrote:
>> On 03.07.14 15:58, Gabriel L. Somlo wrote:
>>> On Thu, Jul 03, 2014 at 03:20:50PM +0200, Alexander Graf wrote:
>>>> On 03.07.2014, at 15:17, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
>>>>
>>>>> On Thu, Jul 03, 2014 at 10:04:55AM +0200, Alexander Graf wrote:
>>>>>>> so Ethernet, SATA, and USB, all sharing IRQ 11. Is there an easy way
>>>>>>> to force one of those to use a different IRQ ?
>>>>> Oh, and on Q35, while Ethernet (and one of the USBs) is still on IRQ 11,
>>>>> SATA ended up on IRQ 10, and things are fine there...
>>>>>
>>>>>> IIRC if you plug the device in a different slot, the irq distribution
>>>>>> should be different :).
>>>>> Sorry for being thick, but I'm still trying to figure out if there's a
>>>>> command-line way of making that happen. Re-ordering the "-device"
>>>>> arguments to qemu obviously doesn't make a difference in how they're
>>>>> assigned...
>>>> The magic word is "devfn" :). It's basically the token PCI uses to find out which slot and function id a device is on. You can set "devfn" with the "addr" attribute on -device.
>>> OK. So simply doing "-device e1000-82545em,...,bus=pci.0,addr=5" to
>>> move it up one slot from its default of "4" gets it set up with IRQ 10.
>>> As long as it doesn't share an irq line with the SATA controller, the
>>> network link gets detected just fine, same as on Q35.
>>>
>>> So this is not really an autonegotiation or timing bug, it's an
>>> unfortunate side effect of the OS X built-in proprietary e1000 driver
>>> not playing nice when IRQs are shared with SATA in particular. At
>>> least as far as I'm able to tell so far... :)
>>>
>>> So I'll send out some autoneg cleanup patches after 2.1 is officially
>>> out, but the "osx link on piix not working" mystery is solved as far
>>> as I can tell :)
>> Maybe only ICR bits that actually would trigger an interrupt get cleared?
> you mean, modify e1000's mac_icr_read() to only clear bits on read if
> the mask says they should be "active" at that moment ?
>
> The e1000 spec doesn't seem to say that though, it's "you read it,
> it's now 0", regardless of the mask configuration.
>
> http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf
> on page 293.

Maybe the IRQ line is simply never shared on real Macs :). Who knows.

Does this (untested!) patch make it work for you?


Alex

Comments

Paolo Bonzini July 3, 2014, 4:09 p.m. UTC | #1 
Il 03/07/2014 17:25, Alexander Graf ha scritto:
>
> Maybe the IRQ line is simply never shared on real Macs :).

That's most likely the case, or else it uses MSI.

Consider that QEMU only uses 2 GSIs for the four PCI interrupts (10 11), 
while real PIIX-era hardware used 4 (5 9 10 11, I think).

Paolo
Gabriel L. Somlo July 3, 2014, 4:43 p.m. UTC | #2 
On Thu, Jul 03, 2014 at 05:25:04PM +0200, Alexander Graf wrote:
> Maybe the IRQ line is simply never shared on real Macs :). Who knows.
> 
> Does this (untested!) patch make it work for you?

Yep, it works. Doesn't break OSX on q35, Linux (F20) or windows (7)
either :)

Thanks,
--Gabriel

> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 0fc29a0..7db0538 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1090,9 +1090,24 @@ static uint32_t
>  mac_icr_read(E1000State *s, int index)
>  {
>      uint32_t ret = s->mac_reg[ICR];
> +    uint32_t new_icr = 0;
> 
>      DBGOUT(INTERRUPT, "ICR read: %x\n", ret);
> -    set_interrupt_cause(s, 0, 0);
> +
> +    /*
> +     * Mac OS X reads ICR on every interrupt. When the IRQ line is shared,
> +     * this may result in a race where LSC is not interpreted yet, but
> +     * already gets cleared.
> +     *
> +     * The easiest fix is to delay LSC events until after they have been
> +     * property unmasked, so let's just claim we never saw any here.
> +     */
> +    if ((ret & E1000_ICS_LSC) && !(s->mac_reg[IMS] & E1000_ICS_LSC)) {
> +        ret &= ~E1000_ICS_LSC;
> +        new_icr |= E1000_ICS_LSC;
> +    }
> +
> +    set_interrupt_cause(s, 0, new_icr);
>      return ret;
>  }
>
Alexander Graf July 3, 2014, 5:33 p.m. UTC | #3 
On 03.07.14 18:43, Gabriel L. Somlo wrote:
> On Thu, Jul 03, 2014 at 05:25:04PM +0200, Alexander Graf wrote:
>> Maybe the IRQ line is simply never shared on real Macs :). Who knows.
>>
>> Does this (untested!) patch make it work for you?
> Yep, it works. Doesn't break OSX on q35, Linux (F20) or windows (7)
> either :)

Ok, I'll post it as a real patch then :).


Alex
diff mbox

Patch

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0fc29a0..7db0538 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1090,9 +1090,24 @@  static uint32_t
  mac_icr_read(E1000State *s, int index)
  {
      uint32_t ret = s->mac_reg[ICR];
+    uint32_t new_icr = 0;

      DBGOUT(INTERRUPT, "ICR read: %x\n", ret);
-    set_interrupt_cause(s, 0, 0);
+
+    /*
+     * Mac OS X reads ICR on every interrupt. When the IRQ line is shared,
+     * this may result in a race where LSC is not interpreted yet, but
+     * already gets cleared.
+     *
+     * The easiest fix is to delay LSC events until after they have been
+     * property unmasked, so let's just claim we never saw any here.
+     */
+    if ((ret & E1000_ICS_LSC) && !(s->mac_reg[IMS] & E1000_ICS_LSC)) {
+        ret &= ~E1000_ICS_LSC;
+        new_icr |= E1000_ICS_LSC;
+    }
+
+    set_interrupt_cause(s, 0, new_icr);
      return ret;
  }