[Qemu-devel] [RFC PATCH v1 0/2] e1000: More link negotiation vs. OS X

[Qemu-devel] [RFC PATCH v1 0/2] e1000: More link negotiation vs. OS X

[Gabriel L. Somlo]

So I accidentally noticed this morning that my previous batch of
e1000 link negotiation patches (specifically, commit 39bb8ee737...)
only fix the problem on q35, but (at least on OS X 10.8 and 10.9)
not also on piix.

After digging around a bit, I came up with the following two issues:

  - 1/2 set_phy_ctrl() seems to act on the value about to be written
        to the phy ctrl. register, but not actually write the value
        to the register! Assuming that's not intentional, once I fix
        it and write the register, I can use the factored-out check
        for autoneg availability from earlier ( have_autoneg() ) if
        I move it before set_phy_ctrl() to avoid a forward declaration.

  - 2/2 the timer delay requested from set_phy_ctrl() is currently 500,
        which, at least on piix + OS X + the built-in e1000 driver, fires
        too early (on piix, OS X takes longer to start paying attention
        to LSC interrupts than it does on q35, for some reason I'm not
        yet clear about). Increasing the timer delay to 5500 (by trial
        and error, possibly specific to my dev machine -- a macpro1,1)
        has it fire, bring the link up, and inject the LSC late enough
        that the OS X driver will be ready for it.

I'm curious what you all think of this. If e.g. patch 1/2 makes sense,
I'd be OK with having it go in without 2/2, which may feel a bit hacky :)

But mainly I'm looking to be educated about what I might have missed :)

Thanks much,
  Gabriel

Gabriel L. Somlo (2):
  e1000: clean up set_phy_ctrl function
  e1000: adjust initial autoneg timing (for piix/osx)

 hw/net/e1000.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

set_phy_ctrl() runs the same autonegotiation availability checks
that were factored out into have_autoneg() in an earlier commit
(d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
on the value about to be written into the phy_ctrl register, and
then does not actually write the register.

This patch moves the have_autoneg() function in front of
set_phy_ctrl(), then updates the latter to actually write
the register with the given value, and use the factored-out
check to determine whether the link should be bounced.

Also, fix indentation/line width when setting up the timer.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

I'm not 100% sure whether "val" in set_phy_ctrl() is not written
to phy_reg[PHY_CTRL] by mistake, or whether there's an ulterior
motive (in which case we should add a comment as to why that is).

Assuming it's a mistake, and we *should* have been writing it (as
per my patch below), we can then use have_autoneg() to also simplify
the code significantly :)

Thanks,
  Gabriel

 hw/net/e1000.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0fc29a0..2376910 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -186,6 +186,14 @@ e1000_link_up(E1000State *s)
     s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
 }
 
+static bool
+have_autoneg(E1000State *s)
+{
+    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
+           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
+           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
+}
+
 static void
 set_phy_ctrl(E1000State *s, int index, uint16_t val)
 {
@@ -194,13 +202,14 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
      * migrate during auto negotiation, after migration the link will be
      * down.
      */
-    if (!(s->compat_flags & E1000_FLAG_AUTONEG)) {
-        return;
-    }
-    if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
+
+    s->phy_reg[PHY_CTRL] = val;
+
+    if (have_autoneg(s)) {
         e1000_link_down(s);
         DBGOUT(PHY, "Start link auto negotiation\n");
-        timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
+        timer_mod(s->autoneg_timer,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
     }
 }
 
@@ -848,14 +857,6 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
     return 0;
 }
 
-static bool
-have_autoneg(E1000State *s)
-{
-    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
-           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
-           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
-}
-
 static void
 e1000_set_link_status(NetClientState *nc)
 {
-- 
1.9.3

[Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Gabriel L. Somlo]

When running on PIIX (as opposed to q35), the stock OS X e1000
driver (AppleIntel8254XEthernet.kext) takes longer to load and
activete, and will "miss" the link status change interrupt
injected when the emulated "hardware" autonegotiation completes
(see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).

This patch extends the delay of the autonetotiation timer set up
during set_phy_ctrl() to a value just large enough to work with
the OS X driver.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

So, the loading OS X driver must take longer between its last
write to the PHY_CTRL register and the time it starts looking
for LSC interrupts, because at delay==500 it obviously misses
the relevant interrupt. Making this 5500 (actually anything
larger than 5300, but there's a bit of variation across OS X
versions, so I rounded up a bit) has the timer fire after
enough time has passed that the driver knows what to do when
the interrupt from the network card fires...

Thanks,
  Gabriel

 hw/net/e1000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 2376910..2300477 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
         e1000_link_down(s);
         DBGOUT(PHY, "Start link auto negotiation\n");
         timer_mod(s->autoneg_timer,
-                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
     }
 }
 
-- 
1.9.3

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Michael S. Tsirkin]

On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
> When running on PIIX (as opposed to q35), the stock OS X e1000
> driver (AppleIntel8254XEthernet.kext) takes longer to load and
> activete, and will "miss" the link status change interrupt
> injected when the emulated "hardware" autonegotiation completes
> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
> 
> This patch extends the delay of the autonetotiation timer set up
> during set_phy_ctrl() to a value just large enough to work with
> the OS X driver.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> So, the loading OS X driver must take longer between its last
> write to the PHY_CTRL register and the time it starts looking
> for LSC interrupts, because at delay==500 it obviously misses
> the relevant interrupt. Making this 5500 (actually anything
> larger than 5300, but there's a bit of variation across OS X
> versions, so I rounded up a bit) has the timer fire after
> enough time has passed that the driver knows what to do when
> the interrupt from the network card fires...
> 
> Thanks,
>   Gabriel
> 
>  hw/net/e1000.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 2376910..2300477 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
>          e1000_link_down(s);
>          DBGOUT(PHY, "Start link auto negotiation\n");
>          timer_mod(s->autoneg_timer,
> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
>      }
>  }


Besides being a bit hacky, it actually has a decent chance
to delay boot for guests. 500ms is probably the max we
can reasonably tolerate, even that is a bit high.

> -- 
> 1.9.3

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Paolo Bonzini]

Il 30/06/2014 19:55, Michael S. Tsirkin ha scritto:
> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
>> When running on PIIX (as opposed to q35), the stock OS X e1000
>> driver (AppleIntel8254XEthernet.kext) takes longer to load and
>> activete, and will "miss" the link status change interrupt
>> injected when the emulated "hardware" autonegotiation completes
>> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
>>
>> This patch extends the delay of the autonetotiation timer set up
>> during set_phy_ctrl() to a value just large enough to work with
>> the OS X driver.
>>
>> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>> ---
>>
>> So, the loading OS X driver must take longer between its last
>> write to the PHY_CTRL register and the time it starts looking
>> for LSC interrupts, because at delay==500 it obviously misses
>> the relevant interrupt. Making this 5500 (actually anything
>> larger than 5300, but there's a bit of variation across OS X
>> versions, so I rounded up a bit) has the timer fire after
>> enough time has passed that the driver knows what to do when
>> the interrupt from the network card fires...
>>
>> Thanks,
>>   Gabriel
>>
>>  hw/net/e1000.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 2376910..2300477 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
>>          e1000_link_down(s);
>>          DBGOUT(PHY, "Start link auto negotiation\n");
>>          timer_mod(s->autoneg_timer,
>> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
>>      }
>>  }
>
>
> Besides being a bit hacky, it actually has a decent chance
> to delay boot for guests. 500ms is probably the max we
> can reasonably tolerate, even that is a bit high.

It can be turned into a property though.

Paolo

Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Michael S. Tsirkin]

On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> set_phy_ctrl() runs the same autonegotiation availability checks
> that were factored out into have_autoneg() in an earlier commit
> (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> on the value about to be written into the phy_ctrl register, and
> then does not actually write the register.
> 
> This patch moves the have_autoneg() function in front of
> set_phy_ctrl(), then updates the latter to actually write
> the register with the given value, and use the factored-out
> check to determine whether the link should be bounced.
> 
> Also, fix indentation/line width when setting up the timer.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>

Weird.
So does this mean have_autoneg is always false then?
How does it work?

Wrt to patch itself, let's do it properly pls:
- bits 0:5 are RO on some cards
- bit 9 is SC

> ---
> 
> I'm not 100% sure whether "val" in set_phy_ctrl() is not written
> to phy_reg[PHY_CTRL] by mistake, or whether there's an ulterior
> motive (in which case we should add a comment as to why that is).
> 
> Assuming it's a mistake, and we *should* have been writing it (as
> per my patch below), we can then use have_autoneg() to also simplify
> the code significantly :)
> 
> Thanks,
>   Gabriel
> 
>  hw/net/e1000.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 0fc29a0..2376910 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -186,6 +186,14 @@ e1000_link_up(E1000State *s)
>      s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
>  }
>  
> +static bool
> +have_autoneg(E1000State *s)
> +{
> +    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> +           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> +           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> +}
> +
>  static void
>  set_phy_ctrl(E1000State *s, int index, uint16_t val)
>  {
> @@ -194,13 +202,14 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
>       * migrate during auto negotiation, after migration the link will be
>       * down.
>       */
> -    if (!(s->compat_flags & E1000_FLAG_AUTONEG)) {
> -        return;
> -    }
> -    if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
> +
> +    s->phy_reg[PHY_CTRL] = val;
> +
> +    if (have_autoneg(s)) {
>          e1000_link_down(s);
>          DBGOUT(PHY, "Start link auto negotiation\n");
> -        timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> +        timer_mod(s->autoneg_timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
>      }
>  }
>  
> @@ -848,14 +857,6 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
>      return 0;
>  }
>  
> -static bool
> -have_autoneg(E1000State *s)
> -{
> -    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> -           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> -           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> -}
> -
>  static void
>  e1000_set_link_status(NetClientState *nc)
>  {
> -- 
> 1.9.3

Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

On Mon, Jun 30, 2014 at 09:04:12PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> > set_phy_ctrl() runs the same autonegotiation availability checks
> > that were factored out into have_autoneg() in an earlier commit
> > (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> > on the value about to be written into the phy_ctrl register, and
> > then does not actually write the register.
> > 
> > This patch moves the have_autoneg() function in front of
> > set_phy_ctrl(), then updates the latter to actually write
> > the register with the given value, and use the factored-out
> > check to determine whether the link should be bounced.
> > 
> > Also, fix indentation/line width when setting up the timer.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> 
> Weird.
> So does this mean have_autoneg is always false then?
> How does it work?

phy_reg[PHY_CTRL] is initialized (along with the rest of phy_reg)
by a memmove from phy_reg_init, during e1000_reset().

So the question would be rather "does it make sense to look at
anything besides (s->compat_flags & E1000_FLAG_AUTONEG), since
we never allow phy_reg[PHY_CTRL] to change after initialization,
so MII_CR_AUTO_NEG_EN and MII_CR_RESTART_AUTO_NEG bits will always
be set?"

> Wrt to patch itself, let's do it properly pls:
> - bits 0:5 are RO on some cards
> - bit 9 is SC

Will do, once we've sorted out the phy_reg[] mystery :)

Thanks,
--Gabriel

> 
> > ---
> > 
> > I'm not 100% sure whether "val" in set_phy_ctrl() is not written
> > to phy_reg[PHY_CTRL] by mistake, or whether there's an ulterior
> > motive (in which case we should add a comment as to why that is).
> > 
> > Assuming it's a mistake, and we *should* have been writing it (as
> > per my patch below), we can then use have_autoneg() to also simplify
> > the code significantly :)
> > 
> > Thanks,
> >   Gabriel
> > 
> >  hw/net/e1000.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 0fc29a0..2376910 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -186,6 +186,14 @@ e1000_link_up(E1000State *s)
> >      s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
> >  }
> >  
> > +static bool
> > +have_autoneg(E1000State *s)
> > +{
> > +    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > +           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > +           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > +}
> > +
> >  static void
> >  set_phy_ctrl(E1000State *s, int index, uint16_t val)
> >  {
> > @@ -194,13 +202,14 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> >       * migrate during auto negotiation, after migration the link will be
> >       * down.
> >       */
> > -    if (!(s->compat_flags & E1000_FLAG_AUTONEG)) {
> > -        return;
> > -    }
> > -    if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
> > +
> > +    s->phy_reg[PHY_CTRL] = val;
> > +
> > +    if (have_autoneg(s)) {
> >          e1000_link_down(s);
> >          DBGOUT(PHY, "Start link auto negotiation\n");
> > -        timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> > +        timer_mod(s->autoneg_timer,
> > +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> >      }
> >  }
> >  
> > @@ -848,14 +857,6 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
> >      return 0;
> >  }
> >  
> > -static bool
> > -have_autoneg(E1000State *s)
> > -{
> > -    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > -           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > -           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > -}
> > -
> >  static void
> >  e1000_set_link_status(NetClientState *nc)
> >  {
> > -- 
> > 1.9.3

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Alexander Graf]

> Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> 
>> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
>> When running on PIIX (as opposed to q35), the stock OS X e1000
>> driver (AppleIntel8254XEthernet.kext) takes longer to load and
>> activete, and will "miss" the link status change interrupt
>> injected when the emulated "hardware" autonegotiation completes
>> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
>> 
>> This patch extends the delay of the autonetotiation timer set up
>> during set_phy_ctrl() to a value just large enough to work with
>> the OS X driver.
>> 
>> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>> ---
>> 
>> So, the loading OS X driver must take longer between its last
>> write to the PHY_CTRL register and the time it starts looking
>> for LSC interrupts, because at delay==500 it obviously misses
>> the relevant interrupt. Making this 5500 (actually anything
>> larger than 5300, but there's a bit of variation across OS X
>> versions, so I rounded up a bit) has the timer fire after
>> enough time has passed that the driver knows what to do when
>> the interrupt from the network card fires...
>> 
>> Thanks,
>>  Gabriel
>> 
>> hw/net/e1000.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 2376910..2300477 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
>>         e1000_link_down(s);
>>         DBGOUT(PHY, "Start link auto negotiation\n");
>>         timer_mod(s->autoneg_timer,
>> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
>>     }
>> }
> 
> 
> Besides being a bit hacky, it actually has a decent chance
> to delay boot for guests. 500ms is probably the max we
> can reasonably tolerate, even that is a bit high.

Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?

Alex

> 
>> -- 
>> 1.9.3

Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

On Mon, Jun 30, 2014 at 02:12:51PM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 30, 2014 at 09:04:12PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> > > set_phy_ctrl() runs the same autonegotiation availability checks
> > > that were factored out into have_autoneg() in an earlier commit
> > > (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> > > on the value about to be written into the phy_ctrl register, and
> > > then does not actually write the register.
> > > 
> > > This patch moves the have_autoneg() function in front of
> > > set_phy_ctrl(), then updates the latter to actually write
> > > the register with the given value, and use the factored-out
> > > check to determine whether the link should be bounced.
> > > 
> > > Also, fix indentation/line width when setting up the timer.
> > > 
> > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > 
> > Weird.
> > So does this mean have_autoneg is always false then?
> > How does it work?

Oh, I think I know what's going on now:

In set_mdic() we have:

...
        } else {
            if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
                phyreg_writeops[addr](s, index, data);
            }
            s->phy_reg[addr] = data;
        }
...

Basically, phyreg_writeops[PHY_CTRL] == set_phy_ctrl() is the only
one that applies, but it's like a "pre-write" writeop, the write
happens *after* the write_op completes :) :) Fun stuff...

Leaving it as-is is probably OK, although it does indeed not handle
SC bits properly.

I could rewrite have_autoneg() to accept the *value* of the phy control
register, then we can use it from set_phy_ctrl() before the register
is actually written. Not sure about whether that would be more or less
ugly than the current form :)

Thanks,
--Gabriel

Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

Sorry for the repeated self-replies, I appear to be thinking in very
small increments today :)

On Mon, Jun 30, 2014 at 03:29:57PM -0400, Gabriel L. Somlo wrote:
> > > Weird.
> > > So does this mean have_autoneg is always false then?
> > > How does it work?
> 
> Oh, I think I know what's going on now:
> 
> In set_mdic() we have:
> 
> ...
>         } else {
>             if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
>                 phyreg_writeops[addr](s, index, data);
>             }
>             s->phy_reg[addr] = data;
>         }
> ...

Of course, we could change this to

...
        } else {
            if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
                phyreg_writeops[addr](s, index, data);
            } else {
                s->phy_reg[addr] = data;
            }
        }

and have phy_set_ctrl() be a proper write_op, expected to actually
write the data as well as do other things. That would at least be a
step toward the principle of least WTF, IMHO :)

I'll send out an updated patch shortly...

Thanks,
--Gabriel

> ...
> 
> Basically, phyreg_writeops[PHY_CTRL] == set_phy_ctrl() is the only
> one that applies, but it's like a "pre-write" writeop, the write
> happens *after* the write_op completes :) :) Fun stuff...
> 
> Leaving it as-is is probably OK, although it does indeed not handle
> SC bits properly.
> 
> I could rewrite have_autoneg() to accept the *value* of the phy control
> register, then we can use it from set_phy_ctrl() before the register
> is actually written. Not sure about whether that would be more or less
> ugly than the current form :)
> 
> Thanks,
> --Gabriel

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Gabriel L. Somlo]

On Mon, Jun 30, 2014 at 08:21:43PM +0200, Alexander Graf wrote:
> 
> 
> > Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > 
> >> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
> >> When running on PIIX (as opposed to q35), the stock OS X e1000
> >> driver (AppleIntel8254XEthernet.kext) takes longer to load and
> >> activete, and will "miss" the link status change interrupt
> >> injected when the emulated "hardware" autonegotiation completes
> >> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
> >> 
> >> This patch extends the delay of the autonetotiation timer set up
> >> during set_phy_ctrl() to a value just large enough to work with
> >> the OS X driver.
> >> 
> >> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> >> ---
> >> 
> >> So, the loading OS X driver must take longer between its last
> >> write to the PHY_CTRL register and the time it starts looking
> >> for LSC interrupts, because at delay==500 it obviously misses
> >> the relevant interrupt. Making this 5500 (actually anything
> >> larger than 5300, but there's a bit of variation across OS X
> >> versions, so I rounded up a bit) has the timer fire after
> >> enough time has passed that the driver knows what to do when
> >> the interrupt from the network card fires...
> >> 
> >> Thanks,
> >>  Gabriel
> >> 
> >> hw/net/e1000.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> >> index 2376910..2300477 100644
> >> --- a/hw/net/e1000.c
> >> +++ b/hw/net/e1000.c
> >> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> >>         e1000_link_down(s);
> >>         DBGOUT(PHY, "Start link auto negotiation\n");
> >>         timer_mod(s->autoneg_timer,
> >> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> >> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
> >>     }
> >> }
> > 
> > 
> > Besides being a bit hacky, it actually has a decent chance
> > to delay boot for guests. 500ms is probably the max we
> > can reasonably tolerate, even that is a bit high.
> 
> Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?

I poked around a bit, and the e1000 interrupt mask register is NOT the
problem (the LSC mask bit is clear at all times). If anything, maybe
the PIIX southbridge (or something further up "north") is masking PCI
interrupts (at least from e1000) until roughly 5500 ms into the boot
process ? Any ideas on how I could go about verifying this (without
access to the guest source, obviously :) ) would be very helpful...

Thanks,
--Gabriel

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Alexander Graf]

On 02.07.14 11:02, Gabriel L. Somlo wrote:
> On Mon, Jun 30, 2014 at 08:21:43PM +0200, Alexander Graf wrote:
>>
>>> Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>>>
>>>> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
>>>> When running on PIIX (as opposed to q35), the stock OS X e1000
>>>> driver (AppleIntel8254XEthernet.kext) takes longer to load and
>>>> activete, and will "miss" the link status change interrupt
>>>> injected when the emulated "hardware" autonegotiation completes
>>>> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
>>>>
>>>> This patch extends the delay of the autonetotiation timer set up
>>>> during set_phy_ctrl() to a value just large enough to work with
>>>> the OS X driver.
>>>>
>>>> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>>>> ---
>>>>
>>>> So, the loading OS X driver must take longer between its last
>>>> write to the PHY_CTRL register and the time it starts looking
>>>> for LSC interrupts, because at delay==500 it obviously misses
>>>> the relevant interrupt. Making this 5500 (actually anything
>>>> larger than 5300, but there's a bit of variation across OS X
>>>> versions, so I rounded up a bit) has the timer fire after
>>>> enough time has passed that the driver knows what to do when
>>>> the interrupt from the network card fires...
>>>>
>>>> Thanks,
>>>>   Gabriel
>>>>
>>>> hw/net/e1000.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>>> index 2376910..2300477 100644
>>>> --- a/hw/net/e1000.c
>>>> +++ b/hw/net/e1000.c
>>>> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
>>>>          e1000_link_down(s);
>>>>          DBGOUT(PHY, "Start link auto negotiation\n");
>>>>          timer_mod(s->autoneg_timer,
>>>> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
>>>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
>>>>      }
>>>> }
>>>
>>> Besides being a bit hacky, it actually has a decent chance
>>> to delay boot for guests. 500ms is probably the max we
>>> can reasonably tolerate, even that is a bit high.
>> Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?
> I poked around a bit, and the e1000 interrupt mask register is NOT the
> problem (the LSC mask bit is clear at all times). If anything, maybe
> the PIIX southbridge (or something further up "north") is masking PCI
> interrupts (at least from e1000) until roughly 5500 ms into the boot
> process ? Any ideas on how I could go about verifying this (without
> access to the guest source, obviously :) ) would be very helpful...

Yeah, maybe the interrupt is masked and doesn't get delivered properly? 
See if you can trace when the e1000 emulation starts kicking an 
interrupt and when the guest tries to fetch it (there should be an ack 
register for IRQs somewhere).

If we kick it but the guest doesn't react, the problem is further down - 
check whether the IRQ ever got injected into the guest with trace points.

If we don't kick it, we mask it somewhere in the e1000 emulation and 
need to make sure we do kick once we unmask :). I don't know whether the 
LSC mask is the only one involved.


Alex

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Michael S. Tsirkin]

On Wed, Jul 02, 2014 at 05:02:07AM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 30, 2014 at 08:21:43PM +0200, Alexander Graf wrote:
> > 
> > 
> > > Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > > 
> > >> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
> > >> When running on PIIX (as opposed to q35), the stock OS X e1000
> > >> driver (AppleIntel8254XEthernet.kext) takes longer to load and
> > >> activete, and will "miss" the link status change interrupt
> > >> injected when the emulated "hardware" autonegotiation completes
> > >> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
> > >> 
> > >> This patch extends the delay of the autonetotiation timer set up
> > >> during set_phy_ctrl() to a value just large enough to work with
> > >> the OS X driver.
> > >> 
> > >> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > >> ---
> > >> 
> > >> So, the loading OS X driver must take longer between its last
> > >> write to the PHY_CTRL register and the time it starts looking
> > >> for LSC interrupts, because at delay==500 it obviously misses
> > >> the relevant interrupt. Making this 5500 (actually anything
> > >> larger than 5300, but there's a bit of variation across OS X
> > >> versions, so I rounded up a bit) has the timer fire after
> > >> enough time has passed that the driver knows what to do when
> > >> the interrupt from the network card fires...
> > >> 
> > >> Thanks,
> > >>  Gabriel
> > >> 
> > >> hw/net/e1000.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >> 
> > >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > >> index 2376910..2300477 100644
> > >> --- a/hw/net/e1000.c
> > >> +++ b/hw/net/e1000.c
> > >> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> > >>         e1000_link_down(s);
> > >>         DBGOUT(PHY, "Start link auto negotiation\n");
> > >>         timer_mod(s->autoneg_timer,
> > >> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> > >> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
> > >>     }
> > >> }
> > > 
> > > 
> > > Besides being a bit hacky, it actually has a decent chance
> > > to delay boot for guests. 500ms is probably the max we
> > > can reasonably tolerate, even that is a bit high.
> > 
> > Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?
> 
> I poked around a bit, and the e1000 interrupt mask register is NOT the
> problem (the LSC mask bit is clear at all times). If anything, maybe
> the PIIX southbridge (or something further up "north") is masking PCI
> interrupts (at least from e1000) until roughly 5500 ms into the boot
> process ? Any ideas on how I could go about verifying this (without
> access to the guest source, obviously :) ) would be very helpful...
> 
> Thanks,
> --Gabriel


Just poking around the spec I find more things
we don't implement correctly wrt to auto-negotiation.
For example, MII_SR_AUTONEG_CAPS isn't set, is it?
Maybe that's why your guest doesn't work:
it doesn't expect to get autonegotation at all?

So I have a question: does your patch actually help any guests?
If not, maybe we should defer it to after release,
and try to clean up autonegotiation more thouroughly for 2.2?


-- 
MST

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Gabriel L. Somlo]

On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 02, 2014 at 05:02:07AM -0400, Gabriel L. Somlo wrote:
> > On Mon, Jun 30, 2014 at 08:21:43PM +0200, Alexander Graf wrote:
> > > 
> > > 
> > > > Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > > > 
> > > >> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
> > > >> When running on PIIX (as opposed to q35), the stock OS X e1000
> > > >> driver (AppleIntel8254XEthernet.kext) takes longer to load and
> > > >> activete, and will "miss" the link status change interrupt
> > > >> injected when the emulated "hardware" autonegotiation completes
> > > >> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
> > > >> 
> > > >> This patch extends the delay of the autonetotiation timer set up
> > > >> during set_phy_ctrl() to a value just large enough to work with
> > > >> the OS X driver.
> > > >> 
> > > >> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > >> ---
> > > >> 
> > > >> So, the loading OS X driver must take longer between its last
> > > >> write to the PHY_CTRL register and the time it starts looking
> > > >> for LSC interrupts, because at delay==500 it obviously misses
> > > >> the relevant interrupt. Making this 5500 (actually anything
> > > >> larger than 5300, but there's a bit of variation across OS X
> > > >> versions, so I rounded up a bit) has the timer fire after
> > > >> enough time has passed that the driver knows what to do when
> > > >> the interrupt from the network card fires...
> > > >> 
> > > >> Thanks,
> > > >>  Gabriel
> > > >> 
> > > >> hw/net/e1000.c | 2 +-
> > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > > >> index 2376910..2300477 100644
> > > >> --- a/hw/net/e1000.c
> > > >> +++ b/hw/net/e1000.c
> > > >> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> > > >>         e1000_link_down(s);
> > > >>         DBGOUT(PHY, "Start link auto negotiation\n");
> > > >>         timer_mod(s->autoneg_timer,
> > > >> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> > > >> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
> > > >>     }
> > > >> }
> > > > 
> > > > 
> > > > Besides being a bit hacky, it actually has a decent chance
> > > > to delay boot for guests. 500ms is probably the max we
> > > > can reasonably tolerate, even that is a bit high.
> > > 
> > > Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?
> > 
> > I poked around a bit, and the e1000 interrupt mask register is NOT the
> > problem (the LSC mask bit is clear at all times). If anything, maybe
> > the PIIX southbridge (or something further up "north") is masking PCI
> > interrupts (at least from e1000) until roughly 5500 ms into the boot
> > process ? Any ideas on how I could go about verifying this (without
> > access to the guest source, obviously :) ) would be very helpful...
> > 
> > Thanks,
> > --Gabriel
> 
> 
> Just poking around the spec I find more things
> we don't implement correctly wrt to auto-negotiation.
> For example, MII_SR_AUTONEG_CAPS isn't set, is it?
> Maybe that's why your guest doesn't work:
> it doesn't expect to get autonegotation at all?
> 
> So I have a question: does your patch actually help any guests?
> If not, maybe we should defer it to after release,
> and try to clean up autonegotiation more thouroughly for 2.2?

The patch you just accepted (handling self clearing + reserved bits
for phy_ctrl) was just "collateral cleanup" in my attempts to fix
the suspected interrupt masking issue on PIIX. As such, either now
or waiting for 2.2 is fine with me.

Not sure I'll be able to figure out the real/main issue while 2.1
bugfixes are still being accepted, but *that* would actually be really
nice :)

Thanks,
--Gabriel

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Michael S. Tsirkin]

On Wed, Jul 02, 2014 at 08:05:46AM -0400, Gabriel L. Somlo wrote:
> On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 02, 2014 at 05:02:07AM -0400, Gabriel L. Somlo wrote:
> > > On Mon, Jun 30, 2014 at 08:21:43PM +0200, Alexander Graf wrote:
> > > > 
> > > > 
> > > > > Am 30.06.2014 um 19:55 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> > > > > 
> > > > >> On Mon, Jun 30, 2014 at 12:55:50PM -0400, Gabriel L. Somlo wrote:
> > > > >> When running on PIIX (as opposed to q35), the stock OS X e1000
> > > > >> driver (AppleIntel8254XEthernet.kext) takes longer to load and
> > > > >> activete, and will "miss" the link status change interrupt
> > > > >> injected when the emulated "hardware" autonegotiation completes
> > > > >> (see commit 39bb8ee737595e9b264d075dfcd7d86f4d3f1133).
> > > > >> 
> > > > >> This patch extends the delay of the autonetotiation timer set up
> > > > >> during set_phy_ctrl() to a value just large enough to work with
> > > > >> the OS X driver.
> > > > >> 
> > > > >> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > > >> ---
> > > > >> 
> > > > >> So, the loading OS X driver must take longer between its last
> > > > >> write to the PHY_CTRL register and the time it starts looking
> > > > >> for LSC interrupts, because at delay==500 it obviously misses
> > > > >> the relevant interrupt. Making this 5500 (actually anything
> > > > >> larger than 5300, but there's a bit of variation across OS X
> > > > >> versions, so I rounded up a bit) has the timer fire after
> > > > >> enough time has passed that the driver knows what to do when
> > > > >> the interrupt from the network card fires...
> > > > >> 
> > > > >> Thanks,
> > > > >>  Gabriel
> > > > >> 
> > > > >> hw/net/e1000.c | 2 +-
> > > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >> 
> > > > >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > > > >> index 2376910..2300477 100644
> > > > >> --- a/hw/net/e1000.c
> > > > >> +++ b/hw/net/e1000.c
> > > > >> @@ -209,7 +209,7 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> > > > >>         e1000_link_down(s);
> > > > >>         DBGOUT(PHY, "Start link auto negotiation\n");
> > > > >>         timer_mod(s->autoneg_timer,
> > > > >> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> > > > >> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5500);
> > > > >>     }
> > > > >> }
> > > > > 
> > > > > 
> > > > > Besides being a bit hacky, it actually has a decent chance
> > > > > to delay boot for guests. 500ms is probably the max we
> > > > > can reasonably tolerate, even that is a bit high.
> > > > 
> > > > Are you sure there's not just simply some irq unmasking event after 5500ms we don't handle properly?
> > > 
> > > I poked around a bit, and the e1000 interrupt mask register is NOT the
> > > problem (the LSC mask bit is clear at all times). If anything, maybe
> > > the PIIX southbridge (or something further up "north") is masking PCI
> > > interrupts (at least from e1000) until roughly 5500 ms into the boot
> > > process ? Any ideas on how I could go about verifying this (without
> > > access to the guest source, obviously :) ) would be very helpful...
> > > 
> > > Thanks,
> > > --Gabriel
> > 
> > 
> > Just poking around the spec I find more things
> > we don't implement correctly wrt to auto-negotiation.
> > For example, MII_SR_AUTONEG_CAPS isn't set, is it?
> > Maybe that's why your guest doesn't work:
> > it doesn't expect to get autonegotation at all?
> > 
> > So I have a question: does your patch actually help any guests?
> > If not, maybe we should defer it to after release,
> > and try to clean up autonegotiation more thouroughly for 2.2?
> 
> The patch you just accepted (handling self clearing + reserved bits
> for phy_ctrl) was just "collateral cleanup" in my attempts to fix
> the suspected interrupt masking issue on PIIX. As such, either now
> or waiting for 2.2 is fine with me.

OK so better safe than sorry: I'll drop it for now, please repost when
2.2 opens.  Thanks!

> Not sure I'll be able to figure out the real/main issue while 2.1
> bugfixes are still being accepted, but *that* would actually be really
> nice :)
> 
> Thanks,
> --Gabriel

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Gabriel L. Somlo]

On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote:
> Just poking around the spec I find more things
> we don't implement correctly wrt to auto-negotiation.
> For example, MII_SR_AUTONEG_CAPS isn't set, is it?
> Maybe that's why your guest doesn't work:
> it doesn't expect to get autonegotation at all?
> 
> So I have a question: does your patch actually help any guests?
> If not, maybe we should defer it to after release,
> and try to clean up autonegotiation more thouroughly for 2.2?

I'll re-submit after 2.1 is officially out. But, since we're talking
about MII_SR_AUTONEG_CAPS:  PHY_STATUS is initialized to 0x794d, which
includes setting the MII_SR_AUTONEG_CAPS bit (|= 0x8).

Did you mean: we should check for MII_SR_AUTONEG_CAPS in have_autoneg() ?
(i.e., on the chance it gets turned off by a guest-side write to
PHY_STATUS) ?

Thx,
--G

PS. Maybe also spell out the individual bits in phy_reg_init[] ? Like,
instead of:

    [PHY_STATUS] = 0x794d,

do this:

    [PHY_STATUS] = MII_SR_EXTENDED_CAPS |
                   MII_SR_LINK_STATUS |
                   MII_SR_AUTONEG_CAPS | 
                   MII_SR_PREAMBLE_SUPPRESS | 
                   MII_SR_EXTENDED_STATUS | 
                   MII_SR_10T_HD_CAPS | 
                   MII_SR_10T_FD_CAPS | 
                   MII_SR_100X_HD_CAPS | 
                   MII_SR_100X_FD_CAPS,

... for all registers ? Much more verbose, but IMHO that'd be a good
thing :)

Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg timing (for piix/osx)

[Michael S. Tsirkin]

On Wed, Jul 02, 2014 at 10:21:34AM -0400, Gabriel L. Somlo wrote:
> On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote:
> > Just poking around the spec I find more things
> > we don't implement correctly wrt to auto-negotiation.
> > For example, MII_SR_AUTONEG_CAPS isn't set, is it?
> > Maybe that's why your guest doesn't work:
> > it doesn't expect to get autonegotation at all?
> > 
> > So I have a question: does your patch actually help any guests?
> > If not, maybe we should defer it to after release,
> > and try to clean up autonegotiation more thouroughly for 2.2?
> 
> I'll re-submit after 2.1 is officially out. But, since we're talking
> about MII_SR_AUTONEG_CAPS:  PHY_STATUS is initialized to 0x794d, which
> includes setting the MII_SR_AUTONEG_CAPS bit (|= 0x8).
> Did you mean: we should check for MII_SR_AUTONEG_CAPS in have_autoneg() ?
> (i.e., on the chance it gets turned off by a guest-side write to
> PHY_STATUS) ?

PHY_STATUS isn't writeable is it?
No I just got confused with the binary math.
We really should use symbolic constants there,
we already have them defined.

> 
> Thx,
> --G
> 
> PS. Maybe also spell out the individual bits in phy_reg_init[] ? Like,
> instead of:
> 
>     [PHY_STATUS] = 0x794d,
> 
> do this:
> 
>     [PHY_STATUS] = MII_SR_EXTENDED_CAPS |
>                    MII_SR_LINK_STATUS |
>                    MII_SR_AUTONEG_CAPS | 
>                    MII_SR_PREAMBLE_SUPPRESS | 
>                    MII_SR_EXTENDED_STATUS | 
>                    MII_SR_10T_HD_CAPS | 
>                    MII_SR_10T_FD_CAPS | 
>                    MII_SR_100X_HD_CAPS | 
>                    MII_SR_100X_FD_CAPS,
> 
> ... for all registers ? Much more verbose, but IMHO that'd be a good
> thing :)
> 
> 

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

On Wed, Jul 02, 2014 at 11:16:52AM +0200, Alexander Graf wrote:
>>> Are you sure there's not just simply some irq unmasking event
>>> after 5500ms we don't handle properly?
>> I poked around a bit, and the e1000 interrupt mask register is NOT the
>> problem (the LSC mask bit is clear at all times). If anything, maybe
>> the PIIX southbridge (or something further up "north") is masking PCI
>> interrupts (at least from e1000) until roughly 5500 ms into the boot
>> process ? Any ideas on how I could go about verifying this (without
>> access to the guest source, obviously :) ) would be very helpful...
> 
> Yeah, maybe the interrupt is masked and doesn't get delivered properly? See
> if you can trace when the e1000 emulation starts kicking an interrupt and
> when the guest tries to fetch it (there should be an ack register for IRQs
> somewhere).
> 
> If we kick it but the guest doesn't react, the problem is further down -
> check whether the IRQ ever got injected into the guest with trace points.
> 
> If we don't kick it, we mask it somewhere in the e1000 emulation and need to
> make sure we do kick once we unmask :). I don't know whether the LSC mask is
> the only one involved.

So it turns out everything I thought I knew (which was little indeed)
was more or less wrong. The problem, as far as I'm observing it now,
is that on PIIX, the OS X guest obsessively reads the ICR in a tight
loop. It reads the injected LSC (and probably discards it) before
unmasking the corresponding interrupt bit; later on, when it unmasks
LSC, giving the emulated e1000 hardware a chance to raise the irq
line, the actual LSC event has been flushed from the ICR, and the
driver does not detect the link coming up.

Here's how things work normally on Q35 (with INTERRUPT and PHY debugging
enabled, // my comments on the side):

e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: Start link auto negotiation
e1000: Auto negotiation is completed
e1000: set_ics 4, ICR 0, IMR 0               // autoneg timer injects LSC
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ics 2, ICR 4, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ics 2, ICR 6, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ims df                            // guest unmasks interrupts
e1000: set_ics 0, ICR 6, IMR df
e1000: set_interrupt_cause: mit_irq_level=1  // first raising irq edge
e1000: ICR read: 6                           // guest receives LSC (+more)

... and things work nicely from here on out :)


On PIIX, however, things look like this:

e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0

... <8155 "ICR read" repetitions deleted> ...

e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc ffffffff
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: Start link auto negotiation
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0

... <145 "ICR read" repetitions deleted> ...

e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: Auto negotiation is completed
e1000: set_ics 4, ICR 0, IMR 0               // autoneg timer injects LSC
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 4                           // guest reads (and discards?) LSC
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0

... <23 "ICR read" repetitions deleted> ...

e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 2
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ims df                            // guest unmasks interrupts
e1000: set_ics 0, ICR 2, IMR df
e1000: set_interrupt_cause: mit_irq_level=1  // first raising irq edge
e1000: ICR read: 2                           // too late, LSC is gone :(
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc 2
e1000: set_ics 0, ICR 0, IMR dd
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ims df
e1000: set_ics 0, ICR 0, IMR df
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ics 80, ICR 0, IMR df
e1000: set_interrupt_cause: mit_irq_level=1
e1000: set_ics 3, ICR 80, IMR df
e1000: set_interrupt_cause: mit_irq_level=1
e1000: ICR read: 83
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_imc 82
e1000: set_ics 0, ICR 0, IMR 5d
e1000: set_interrupt_cause: mit_irq_level=0
e1000: set_ims df
e1000: set_ics 0, ICR 0, IMR df
e1000: set_interrupt_cause: mit_irq_level=0
e1000: ICR read: 0
e1000: set_interrupt_cause: mit_irq_level=0

... and so on...


Any clue as to why ICR gets read like that on PIIX, but not Q35 ?

Thanks,
--Gabriel

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

On 02.07.14 22:49, Gabriel L. Somlo wrote:
> On Wed, Jul 02, 2014 at 11:16:52AM +0200, Alexander Graf wrote:
>>>> Are you sure there's not just simply some irq unmasking event
>>>> after 5500ms we don't handle properly?
>>> I poked around a bit, and the e1000 interrupt mask register is NOT the
>>> problem (the LSC mask bit is clear at all times). If anything, maybe
>>> the PIIX southbridge (or something further up "north") is masking PCI
>>> interrupts (at least from e1000) until roughly 5500 ms into the boot
>>> process ? Any ideas on how I could go about verifying this (without
>>> access to the guest source, obviously :) ) would be very helpful...
>> Yeah, maybe the interrupt is masked and doesn't get delivered properly? See
>> if you can trace when the e1000 emulation starts kicking an interrupt and
>> when the guest tries to fetch it (there should be an ack register for IRQs
>> somewhere).
>>
>> If we kick it but the guest doesn't react, the problem is further down -
>> check whether the IRQ ever got injected into the guest with trace points.
>>
>> If we don't kick it, we mask it somewhere in the e1000 emulation and need to
>> make sure we do kick once we unmask :). I don't know whether the LSC mask is
>> the only one involved.
> So it turns out everything I thought I knew (which was little indeed)
> was more or less wrong. The problem, as far as I'm observing it now,
> is that on PIIX, the OS X guest obsessively reads the ICR in a tight
> loop. It reads the injected LSC (and probably discards it) before
> unmasking the corresponding interrupt bit; later on, when it unmasks
> LSC, giving the emulated e1000 hardware a chance to raise the irq
> line, the actual LSC event has been flushed from the ICR, and the
> driver does not detect the link coming up.
>
> Here's how things work normally on Q35 (with INTERRUPT and PHY debugging
> enabled, // my comments on the side):
>
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: Start link auto negotiation
> e1000: Auto negotiation is completed
> e1000: set_ics 4, ICR 0, IMR 0               // autoneg timer injects LSC
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ics 2, ICR 4, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ics 2, ICR 6, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ims df                            // guest unmasks interrupts
> e1000: set_ics 0, ICR 6, IMR df
> e1000: set_interrupt_cause: mit_irq_level=1  // first raising irq edge
> e1000: ICR read: 6                           // guest receives LSC (+more)
>
> ... and things work nicely from here on out :)
>
>
> On PIIX, however, things look like this:
>
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
>
> ... <8155 "ICR read" repetitions deleted> ...
>
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc ffffffff
> e1000: set_ics 0, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: Start link auto negotiation
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
>
> ... <145 "ICR read" repetitions deleted> ...
>
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: Auto negotiation is completed
> e1000: set_ics 4, ICR 0, IMR 0               // autoneg timer injects LSC
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 4                           // guest reads (and discards?) LSC
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
>
> ... <23 "ICR read" repetitions deleted> ...
>
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ics 2, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 2
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ics 2, ICR 0, IMR 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ims df                            // guest unmasks interrupts
> e1000: set_ics 0, ICR 2, IMR df
> e1000: set_interrupt_cause: mit_irq_level=1  // first raising irq edge
> e1000: ICR read: 2                           // too late, LSC is gone :(
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc 2
> e1000: set_ics 0, ICR 0, IMR dd
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ims df
> e1000: set_ics 0, ICR 0, IMR df
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ics 80, ICR 0, IMR df
> e1000: set_interrupt_cause: mit_irq_level=1
> e1000: set_ics 3, ICR 80, IMR df
> e1000: set_interrupt_cause: mit_irq_level=1
> e1000: ICR read: 83
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_imc 82
> e1000: set_ics 0, ICR 0, IMR 5d
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: set_ims df
> e1000: set_ics 0, ICR 0, IMR df
> e1000: set_interrupt_cause: mit_irq_level=0
> e1000: ICR read: 0
> e1000: set_interrupt_cause: mit_irq_level=0
>
> ... and so on...
>
>
> Any clue as to why ICR gets read like that on PIIX, but not Q35 ?

Maybe it's using MSI on q35? :)
Maybe we also share the same IRQ line with another device on PIIX that 
gets polled all the time? IDE maybe?

Either way, why does the bit get cleared even though it hasn't been 
raised? What does real hardware do with interrupts that have been masked?


Alex

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

On Wed, Jul 02, 2014 at 11:02:30PM +0200, Alexander Graf wrote:
> 
> On 02.07.14 22:49, Gabriel L. Somlo wrote:
> >So it turns out everything I thought I knew (which was little indeed)
> >was more or less wrong. The problem, as far as I'm observing it now,
> >is that on PIIX, the OS X guest obsessively reads the ICR in a tight
> >loop. It reads the injected LSC (and probably discards it) before
> >unmasking the corresponding interrupt bit; later on, when it unmasks
> >LSC, giving the emulated e1000 hardware a chance to raise the irq
> >line, the actual LSC event has been flushed from the ICR, and the
> >driver does not detect the link coming up.
> >
> > [...]
> >
> >Any clue as to why ICR gets read like that on PIIX, but not Q35 ?
> 
> Either way, why does the bit get cleared even though it hasn't been raised?
> What does real hardware do with interrupts that have been masked?

The e1000 manual says ICR bits are cleared on read. It also says PCI
interrupts are only generated if the corresponding bit in *both* ICR
and IMS registers is 1. ICR bits are still cleared if read, even if
masked and no actual interrupt is raised.

> Maybe it's using MSI on q35? :)
> Maybe we also share the same IRQ line with another device on PIIX that gets
> polled all the time? IDE maybe?

Even if that were the case, how come it's reading precisely our
device's ICR register ? Isn't that too much of a coincidence ? 

/me goes back to reading about PIIX and ICH10 (840 pages FTW!) :)

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

> Am 02.07.2014 um 23:14 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> 
>> On Wed, Jul 02, 2014 at 11:02:30PM +0200, Alexander Graf wrote:
>> 
>>> On 02.07.14 22:49, Gabriel L. Somlo wrote:
>>> So it turns out everything I thought I knew (which was little indeed)
>>> was more or less wrong. The problem, as far as I'm observing it now,
>>> is that on PIIX, the OS X guest obsessively reads the ICR in a tight
>>> loop. It reads the injected LSC (and probably discards it) before
>>> unmasking the corresponding interrupt bit; later on, when it unmasks
>>> LSC, giving the emulated e1000 hardware a chance to raise the irq
>>> line, the actual LSC event has been flushed from the ICR, and the
>>> driver does not detect the link coming up.
>>> 
>>> [...]
>>> 
>>> Any clue as to why ICR gets read like that on PIIX, but not Q35 ?
>> 
>> Either way, why does the bit get cleared even though it hasn't been raised?
>> What does real hardware do with interrupts that have been masked?
> 
> The e1000 manual says ICR bits are cleared on read. It also says PCI
> interrupts are only generated if the corresponding bit in *both* ICR
> and IMS registers is 1. ICR bits are still cleared if read, even if
> masked and no actual interrupt is raised.
> 
>> Maybe it's using MSI on q35? :)
>> Maybe we also share the same IRQ line with another device on PIIX that gets
>> polled all the time? IDE maybe?
> 
> Even if that were the case, how come it's reading precisely our
> device's ICR register ? Isn't that too much of a coincidence ? 

When PCI devices share an irq line, the OS needs to poke all devices on that line to figure out which device the IRQ came from. Maybe it's reading ICR for that purpose?

Alex

> 
> /me goes back to reading about PIIX and ICH10 (840 pages FTW!) :)
> 

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

On Wed, Jul 02, 2014 at 05:14:26PM -0400, Gabriel L. Somlo wrote:
> On Wed, Jul 02, 2014 at 11:02:30PM +0200, Alexander Graf wrote:
> > 
> > On 02.07.14 22:49, Gabriel L. Somlo wrote:
> > >So it turns out everything I thought I knew (which was little indeed)
> > >was more or less wrong. The problem, as far as I'm observing it now,
> > >is that on PIIX, the OS X guest obsessively reads the ICR in a tight
> > >loop. It reads the injected LSC (and probably discards it) before
> > >unmasking the corresponding interrupt bit; later on, when it unmasks
> > >LSC, giving the emulated e1000 hardware a chance to raise the irq
> > >line, the actual LSC event has been flushed from the ICR, and the
> > >driver does not detect the link coming up.
> > >
> > > [...]
> > >
> > >Any clue as to why ICR gets read like that on PIIX, but not Q35 ?
> > 
> > Either way, why does the bit get cleared even though it hasn't been raised?
> > What does real hardware do with interrupts that have been masked?
> 
> The e1000 manual says ICR bits are cleared on read. It also says PCI
> interrupts are only generated if the corresponding bit in *both* ICR
> and IMS registers is 1. ICR bits are still cleared if read, even if
> masked and no actual interrupt is raised.
> 
> > Maybe it's using MSI on q35? :)
> > Maybe we also share the same IRQ line with another device on PIIX that gets
> > polled all the time? IDE maybe?
> 
> Even if that were the case, how come it's reading precisely our
> device's ICR register ? Isn't that too much of a coincidence ? 

Unless some other device on a shared IRQ line generated the interrupt,
and our e1000 driver then checked ICR. Although if that happened, our
driver should happily acknowledge the link-up event (I checked, and it
does NOT read IMS, so it has no way of knowing it's masked). Except it
does -- it knows the last thing it did was set_imc(0xFFFFFFFF), and
that it hasn't yet set_ims(anything) yet... 

And I think you're right:

(qemu) info pci
  Bus  0, device   0, function 0:
    Host bridge: PCI device 8086:1237
      id ""
  Bus  0, device   1, function 0:
    ISA bridge: PCI device 8086:7000
      id ""
  Bus  0, device   1, function 1:
    IDE controller: PCI device 8086:7010
      BAR4: I/O at 0xc080 [0xc08f].
      id ""
  Bus  0, device   1, function 2:
    USB controller: PCI device 8086:7020
      IRQ 11.
      BAR4: I/O at 0xc040 [0xc05f].
      id ""
  Bus  0, device   1, function 3:
    Bridge: PCI device 8086:7113
      IRQ 9.
      id ""
  Bus  0, device   2, function 0:
    VGA controller: PCI device 1013:00b8
      BAR0: 32 bit prefetchable memory at 0xfc000000 [0xfdffffff].
      BAR1: 32 bit memory at 0xfebf0000 [0xfebf0fff].
      BAR6: 32 bit memory at 0xffffffffffffffff [0x0000fffe].
      id ""
  Bus  0, device   3, function 0:
    SATA controller: PCI device 8086:2922
      IRQ 11.
      BAR4: I/O at 0xc060 [0xc07f].
      BAR5: 32 bit memory at 0xfebf1000 [0xfebf1fff].
      id "ide"
  Bus  0, device   4, function 0:
    Ethernet controller: PCI device 8086:100f
      IRQ 11.
      BAR0: 32 bit memory at 0xfebc0000 [0xfebdffff].
      BAR1: I/O at 0xffffffffffffffff [0x003e].
      BAR6: 32 bit memory at 0xffffffffffffffff [0x0003fffe].
      id "vnet0"

so Ethernet, SATA, and USB, all sharing IRQ 11. Is there an easy way
to force one of those to use a different IRQ ?

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

> Am 03.07.2014 um 00:02 schrieb "Gabriel L. Somlo" <gsomlo@gmail.com>:
> 
>> On Wed, Jul 02, 2014 at 05:14:26PM -0400, Gabriel L. Somlo wrote:
>>> On Wed, Jul 02, 2014 at 11:02:30PM +0200, Alexander Graf wrote:
>>> 
>>>> On 02.07.14 22:49, Gabriel L. Somlo wrote:
>>>> So it turns out everything I thought I knew (which was little indeed)
>>>> was more or less wrong. The problem, as far as I'm observing it now,
>>>> is that on PIIX, the OS X guest obsessively reads the ICR in a tight
>>>> loop. It reads the injected LSC (and probably discards it) before
>>>> unmasking the corresponding interrupt bit; later on, when it unmasks
>>>> LSC, giving the emulated e1000 hardware a chance to raise the irq
>>>> line, the actual LSC event has been flushed from the ICR, and the
>>>> driver does not detect the link coming up.
>>>> 
>>>> [...]
>>>> 
>>>> Any clue as to why ICR gets read like that on PIIX, but not Q35 ?
>>> 
>>> Either way, why does the bit get cleared even though it hasn't been raised?
>>> What does real hardware do with interrupts that have been masked?
>> 
>> The e1000 manual says ICR bits are cleared on read. It also says PCI
>> interrupts are only generated if the corresponding bit in *both* ICR
>> and IMS registers is 1. ICR bits are still cleared if read, even if
>> masked and no actual interrupt is raised.
>> 
>>> Maybe it's using MSI on q35? :)
>>> Maybe we also share the same IRQ line with another device on PIIX that gets
>>> polled all the time? IDE maybe?
>> 
>> Even if that were the case, how come it's reading precisely our
>> device's ICR register ? Isn't that too much of a coincidence ?
> 
> Unless some other device on a shared IRQ line generated the interrupt,
> and our e1000 driver then checked ICR. Although if that happened, our
> driver should happily acknowledge the link-up event (I checked, and it
> does NOT read IMS, so it has no way of knowing it's masked). Except it
> does -- it knows the last thing it did was set_imc(0xFFFFFFFF), and
> that it hasn't yet set_ims(anything) yet... 
> 
> And I think you're right:
> 
> (qemu) info pci
>  Bus  0, device   0, function 0:
>    Host bridge: PCI device 8086:1237
>      id ""
>  Bus  0, device   1, function 0:
>    ISA bridge: PCI device 8086:7000
>      id ""
>  Bus  0, device   1, function 1:
>    IDE controller: PCI device 8086:7010
>      BAR4: I/O at 0xc080 [0xc08f].
>      id ""
>  Bus  0, device   1, function 2:
>    USB controller: PCI device 8086:7020
>      IRQ 11.
>      BAR4: I/O at 0xc040 [0xc05f].
>      id ""
>  Bus  0, device   1, function 3:
>    Bridge: PCI device 8086:7113
>      IRQ 9.
>      id ""
>  Bus  0, device   2, function 0:
>    VGA controller: PCI device 1013:00b8
>      BAR0: 32 bit prefetchable memory at 0xfc000000 [0xfdffffff].
>      BAR1: 32 bit memory at 0xfebf0000 [0xfebf0fff].
>      BAR6: 32 bit memory at 0xffffffffffffffff [0x0000fffe].
>      id ""
>  Bus  0, device   3, function 0:
>    SATA controller: PCI device 8086:2922
>      IRQ 11.
>      BAR4: I/O at 0xc060 [0xc07f].
>      BAR5: 32 bit memory at 0xfebf1000 [0xfebf1fff].
>      id "ide"
>  Bus  0, device   4, function 0:
>    Ethernet controller: PCI device 8086:100f
>      IRQ 11.
>      BAR0: 32 bit memory at 0xfebc0000 [0xfebdffff].
>      BAR1: I/O at 0xffffffffffffffff [0x003e].
>      BAR6: 32 bit memory at 0xffffffffffffffff [0x0003fffe].
>      id "vnet0"
> 
> so Ethernet, SATA, and USB, all sharing IRQ 11. Is there an easy way
> to force one of those to use a different IRQ ?

IIRC if you plug the device in a different slot, the irq distribution should be different :).

Alex

> 
> 

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

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...

Gabriel

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

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.


Alex

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

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 :)

Thanks much,
--Gabriel

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

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?


Alex

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

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.

Thanks,
--Gabriel

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

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.

Hrm. So could we just delay notification of the LSC change until IMS.LCR 
= 1? In other words, when the guest writes IMS.LCR = 1, it was 0 before 
and the link is up, send an interrupt?


Alex

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

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

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;
  }

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Paolo Bonzini]

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

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Gabriel L. Somlo]

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;
>  }
> 

Re: [Qemu-devel] e1000 autoneg timing, piix/osx

[Alexander Graf]

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