| Message ID | 1404409177-3226-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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?
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
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; }
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(-)