[Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Alexander Graf]

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 guest already has a way of telling us that it can interpret LSC events
though and that's via the interrupt mask register (IMS).

So if we just leave the LSC interrupt bit pending, but invisible to the guest
as long as it's not ready to receive LSC interrupts, we basically defer the
interrupt to the earliest point in time when the guest would know how to
handle it.

This patch fixes occasional e1000 link detection hickups with Mac OS X.

Reported-by: Gabriel Somlo <somlo@cmu.edu>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/net/e1000.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0fc29a0..595e034 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
+     * properly 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;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Peter Maydell]

On 3 July 2014 18:39, Alexander Graf <agraf@suse.de> wrote:
> 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 guest already has a way of telling us that it can interpret LSC events
> though and that's via the interrupt mask register (IMS).
>
> So if we just leave the LSC interrupt bit pending, but invisible to the guest
> as long as it's not ready to receive LSC interrupts, we basically defer the
> interrupt to the earliest point in time when the guest would know how to
> handle it.

This would break any guests dealing with this in a polling
mode (ie "permanently leave interrupts masked and read
ICR periodically to find out whether anything interesting
has happened"), right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Alexander Graf]

On 03.07.14 19:57, Peter Maydell wrote:
> On 3 July 2014 18:39, Alexander Graf <agraf@suse.de> wrote:
>> 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 guest already has a way of telling us that it can interpret LSC events
>> though and that's via the interrupt mask register (IMS).
>>
>> So if we just leave the LSC interrupt bit pending, but invisible to the guest
>> as long as it's not ready to receive LSC interrupts, we basically defer the
>> interrupt to the earliest point in time when the guest would know how to
>> handle it.
> This would break any guests dealing with this in a polling
> mode (ie "permanently leave interrupts masked and read
> ICR periodically to find out whether anything interesting
> has happened"), right?

If those guests would wait for a link detect event that way, yes.

Considering all the hackery we already have about link negotiation 
(delay it until a random amount of ms passed) I'd say the breakage this 
patch fixes is a lot more likely than a polling guest that waits for a 
link based on ICR.LSC :).


Alex

Re: [Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 08:00:04PM +0200, Alexander Graf wrote:
> 
> On 03.07.14 19:57, Peter Maydell wrote:
> >On 3 July 2014 18:39, Alexander Graf <agraf@suse.de> wrote:
> >>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 guest already has a way of telling us that it can interpret LSC events
> >>though and that's via the interrupt mask register (IMS).
> >>
> >>So if we just leave the LSC interrupt bit pending, but invisible to the guest
> >>as long as it's not ready to receive LSC interrupts, we basically defer the
> >>interrupt to the earliest point in time when the guest would know how to
> >>handle it.
> >This would break any guests dealing with this in a polling
> >mode (ie "permanently leave interrupts masked and read
> >ICR periodically to find out whether anything interesting
> >has happened"), right?
> 
> If those guests would wait for a link detect event that way, yes.
> 
> Considering all the hackery we already have about link negotiation (delay it
> until a random amount of ms passed) I'd say the breakage this patch fixes is
> a lot more likely than a polling guest that waits for a link based on
> ICR.LSC :).
> 
> 
> Alex

Well that hackery was justified by the claim that real hardware behaves
this way: it has a random delay since it needs a bit of time to bring
the link up.

What's the justification here? How come this driver works with
real hardware?

-- 
MST

Re: [Qemu-devel] [PATCH] e1000: Delay LSC until mask is active

[Alexander Graf]

On 03.07.14 22:18, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 08:00:04PM +0200, Alexander Graf wrote:
>> On 03.07.14 19:57, Peter Maydell wrote:
>>> On 3 July 2014 18:39, Alexander Graf <agraf@suse.de> wrote:
>>>> 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 guest already has a way of telling us that it can interpret LSC events
>>>> though and that's via the interrupt mask register (IMS).
>>>>
>>>> So if we just leave the LSC interrupt bit pending, but invisible to the guest
>>>> as long as it's not ready to receive LSC interrupts, we basically defer the
>>>> interrupt to the earliest point in time when the guest would know how to
>>>> handle it.
>>> This would break any guests dealing with this in a polling
>>> mode (ie "permanently leave interrupts masked and read
>>> ICR periodically to find out whether anything interesting
>>> has happened"), right?
>> If those guests would wait for a link detect event that way, yes.
>>
>> Considering all the hackery we already have about link negotiation (delay it
>> until a random amount of ms passed) I'd say the breakage this patch fixes is
>> a lot more likely than a polling guest that waits for a link based on
>> ICR.LSC :).
>>
>>
>> Alex
> Well that hackery was justified by the claim that real hardware behaves
> this way: it has a random delay since it needs a bit of time to bring
> the link up.
>
> What's the justification here? How come this driver works with
> real hardware?

Real hardware never shares interrupts? ;)


Alex