From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: RFC: ioapic polarity vs. qemu os-x guest Date: Sun, 16 Feb 2014 13:37:48 +0200 Message-ID: <20140216113748.GA30056@redhat.com> References: <20140130204423.GK29329@ERROL.INI.CMU.EDU> <20140211182330.GC29329@ERROL.INI.CMU.EDU> <20140211195444.GB10951@redhat.com> <20140214211311.GH29329@ERROL.INI.CMU.EDU> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, eddie.dong@intel.com, agraf@suse.de To: "Gabriel L. Somlo" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:15260 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751197AbaBPLcw (ORCPT ); Sun, 16 Feb 2014 06:32:52 -0500 Content-Disposition: inline In-Reply-To: <20140214211311.GH29329@ERROL.INI.CMU.EDU> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Feb 14, 2014 at 04:13:11PM -0500, Gabriel L. Somlo wrote: > On Tue, Feb 11, 2014 at 09:54:44PM +0200, Michael S. Tsirkin wrote: > > On Tue, Feb 11, 2014 at 01:23:31PM -0500, Gabriel L. Somlo wrote: > > > 1. Regarding KVM and the polarity xor line in the patch above: Does > > > anyone have experience with any *other* guests which insist on setting > > > level-triggered interrupt polarity to 1/active-low ? Is that xor line > > > actually doing anything useful in practice, for any other guest, on > > > either QEMU or any other platform ? > > > > > > > > > 2. Is there anything in QEMU (besides the ACPI DSDT .dsl files) which > > > has a hardcoded assumption re. "polarity == 0", or active-high, for > > > level-triggered interrupts? I tried to dig through hw/i386/kvm/ioapic.c > > > and a bunch of other files, but couldn't isolate anything that I could > > > "flip" to fix things in userspace. > > > > > > > > > Any ideas or suggestions about the appropriate way to move forward would > > > be much appreciated !!! > > > > > > > > > Thanks much, > > > --Gabriel > > > > I think changing ACPI is the right thing to > > do really. But we'll need to fix some things > > first of course. > > So I followed your advice, and was able to boot OS X just fine (but > booting Linux after this patch still resulted in multiple "no one > cared" complaints on IRQs 17, 18, 19, etc.: > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index d618e9e..9c52f64 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -353,7 +353,7 @@ DefinitionBlock ( > Method(IQCR, 1, Serialized) { > // _CRS method - get current settings > Name(PRR0, ResourceTemplate() { > - Interrupt(, Level, ActiveHigh, Shared) { 0 } > + Interrupt(, Level, ActiveLow, Shared) { 0 } > }) > CreateDWordField(PRR0, 0x05, PRRI) > Store(And(Arg0, 0x0F), PRRI) > @@ -365,7 +365,7 @@ DefinitionBlock ( > Name(_HID, EISAID("PNP0C0F")) \ > Name(_UID, uid) \ > Name(_PRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > 5, 10, 11 \ > } \ > }) \ > @@ -398,12 +398,12 @@ DefinitionBlock ( > Name(_HID, EISAID("PNP0C0F")) \ > Name(_UID, uid) \ > Name(_PRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > gsi \ > } \ > }) \ > Name(_CRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > gsi \ > } \ > }) \ > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 51ce12d..fe1527a 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -206,17 +206,17 @@ static void ich9_lpc_update_pic(ICH9LPCState *lpc, int pic_irq) > int i, pic_level; > > /* The pic level is the logical OR of all the PCI irqs mapped to it */ > - pic_level = 0; > + pic_level = 1; > for (i = 0; i < ICH9_LPC_NB_PIRQS; i++) { > int tmp_irq; > int tmp_dis; > ich9_lpc_pic_irq(lpc, i, &tmp_irq, &tmp_dis); > if (!tmp_dis && pic_irq == tmp_irq) { > - pic_level |= pci_bus_get_irq_level(lpc->d.bus, i); > + pic_level &= !pci_bus_get_irq_level(lpc->d.bus, i); > } > } > if (pic_irq == ich9_lpc_sci_irq(lpc)) { > - pic_level |= lpc->sci_level; > + pic_level &= !lpc->sci_level; > } > > qemu_set_irq(lpc->pic[pic_irq], pic_level); > -- > > However, even on OS X, the Ethernet (e1000) card won't link up at all. > Fixing that requires another patch: > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 58ba93b..c7a2c07 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -301,7 +301,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > s->mac_reg[ICS] = val; > > pending_ints = (s->mac_reg[IMS] & s->mac_reg[ICR]); > - if (!s->mit_irq_level && pending_ints) { > + if (s->mit_irq_level && pending_ints) { > /* > * Here we detect a potential raising edge. We postpone raising the > * interrupt line if we are inside the mitigation delay window > @@ -339,7 +339,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > } > } > > - s->mit_irq_level = (pending_ints != 0); > + s->mit_irq_level = (pending_ints == 0); > pci_set_irq(d, s->mit_irq_level); > } > If we really wanted to change it, we could change pci_set_irq to reverse the polarity. But I think doing this in PIC is cleaner. > @@ -393,7 +393,7 @@ static void e1000_reset(void *opaque) > timer_del(d->autoneg_timer); > timer_del(d->mit_timer); > d->mit_timer_on = 0; > - d->mit_irq_level = 0; > + d->mit_irq_level = 1; > d->mit_ide = 0; > memset(d->phy_reg, 0, sizeof d->phy_reg); > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > @@ -1324,7 +1324,7 @@ static int e1000_post_load(void *opaque, int version_id) > if (!(s->compat_flags & E1000_FLAG_MIT)) { > s->mac_reg[ITR] = s->mac_reg[RDTR] = s->mac_reg[RADV] = > s->mac_reg[TADV] = 0; > - s->mit_irq_level = false; > + s->mit_irq_level = true; > } > s->mit_ide = 0; > s->mit_timer_on = false; > --- > > At this point, I'm beginning to realize that the "ActiveHigh" > assumption is rather pervasively baked in throughout the QEMU > source code... > > And I'm wondering whether a ton of changes to make it either > programatically configurable or change the hard-codded assumption > to "ActiveLow" would be feasible, welcome, etc... I personally > would prefer configurable > (e.g. "-machine foo,kernel_irqchip=on,polarity=high", or some such). > > Thanks much for any ideas, > --Gabriel From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEzxx-0004SS-Gi for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:33:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WEzxr-00010x-H5 for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:32:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:32808) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEzxr-00010r-8N for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:32:51 -0500 Date: Sun, 16 Feb 2014 13:37:48 +0200 From: "Michael S. Tsirkin" Message-ID: <20140216113748.GA30056@redhat.com> References: <20140130204423.GK29329@ERROL.INI.CMU.EDU> <20140211182330.GC29329@ERROL.INI.CMU.EDU> <20140211195444.GB10951@redhat.com> <20140214211311.GH29329@ERROL.INI.CMU.EDU> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140214211311.GH29329@ERROL.INI.CMU.EDU> Subject: Re: [Qemu-devel] RFC: ioapic polarity vs. qemu os-x guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: eddie.dong@intel.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, agraf@suse.de On Fri, Feb 14, 2014 at 04:13:11PM -0500, Gabriel L. Somlo wrote: > On Tue, Feb 11, 2014 at 09:54:44PM +0200, Michael S. Tsirkin wrote: > > On Tue, Feb 11, 2014 at 01:23:31PM -0500, Gabriel L. Somlo wrote: > > > 1. Regarding KVM and the polarity xor line in the patch above: Does > > > anyone have experience with any *other* guests which insist on setting > > > level-triggered interrupt polarity to 1/active-low ? Is that xor line > > > actually doing anything useful in practice, for any other guest, on > > > either QEMU or any other platform ? > > > > > > > > > 2. Is there anything in QEMU (besides the ACPI DSDT .dsl files) which > > > has a hardcoded assumption re. "polarity == 0", or active-high, for > > > level-triggered interrupts? I tried to dig through hw/i386/kvm/ioapic.c > > > and a bunch of other files, but couldn't isolate anything that I could > > > "flip" to fix things in userspace. > > > > > > > > > Any ideas or suggestions about the appropriate way to move forward would > > > be much appreciated !!! > > > > > > > > > Thanks much, > > > --Gabriel > > > > I think changing ACPI is the right thing to > > do really. But we'll need to fix some things > > first of course. > > So I followed your advice, and was able to boot OS X just fine (but > booting Linux after this patch still resulted in multiple "no one > cared" complaints on IRQs 17, 18, 19, etc.: > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index d618e9e..9c52f64 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -353,7 +353,7 @@ DefinitionBlock ( > Method(IQCR, 1, Serialized) { > // _CRS method - get current settings > Name(PRR0, ResourceTemplate() { > - Interrupt(, Level, ActiveHigh, Shared) { 0 } > + Interrupt(, Level, ActiveLow, Shared) { 0 } > }) > CreateDWordField(PRR0, 0x05, PRRI) > Store(And(Arg0, 0x0F), PRRI) > @@ -365,7 +365,7 @@ DefinitionBlock ( > Name(_HID, EISAID("PNP0C0F")) \ > Name(_UID, uid) \ > Name(_PRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > 5, 10, 11 \ > } \ > }) \ > @@ -398,12 +398,12 @@ DefinitionBlock ( > Name(_HID, EISAID("PNP0C0F")) \ > Name(_UID, uid) \ > Name(_PRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > gsi \ > } \ > }) \ > Name(_CRS, ResourceTemplate() { \ > - Interrupt(, Level, ActiveHigh, Shared) { \ > + Interrupt(, Level, ActiveLow, Shared) { \ > gsi \ > } \ > }) \ > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 51ce12d..fe1527a 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -206,17 +206,17 @@ static void ich9_lpc_update_pic(ICH9LPCState *lpc, int pic_irq) > int i, pic_level; > > /* The pic level is the logical OR of all the PCI irqs mapped to it */ > - pic_level = 0; > + pic_level = 1; > for (i = 0; i < ICH9_LPC_NB_PIRQS; i++) { > int tmp_irq; > int tmp_dis; > ich9_lpc_pic_irq(lpc, i, &tmp_irq, &tmp_dis); > if (!tmp_dis && pic_irq == tmp_irq) { > - pic_level |= pci_bus_get_irq_level(lpc->d.bus, i); > + pic_level &= !pci_bus_get_irq_level(lpc->d.bus, i); > } > } > if (pic_irq == ich9_lpc_sci_irq(lpc)) { > - pic_level |= lpc->sci_level; > + pic_level &= !lpc->sci_level; > } > > qemu_set_irq(lpc->pic[pic_irq], pic_level); > -- > > However, even on OS X, the Ethernet (e1000) card won't link up at all. > Fixing that requires another patch: > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 58ba93b..c7a2c07 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -301,7 +301,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > s->mac_reg[ICS] = val; > > pending_ints = (s->mac_reg[IMS] & s->mac_reg[ICR]); > - if (!s->mit_irq_level && pending_ints) { > + if (s->mit_irq_level && pending_ints) { > /* > * Here we detect a potential raising edge. We postpone raising the > * interrupt line if we are inside the mitigation delay window > @@ -339,7 +339,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > } > } > > - s->mit_irq_level = (pending_ints != 0); > + s->mit_irq_level = (pending_ints == 0); > pci_set_irq(d, s->mit_irq_level); > } > If we really wanted to change it, we could change pci_set_irq to reverse the polarity. But I think doing this in PIC is cleaner. > @@ -393,7 +393,7 @@ static void e1000_reset(void *opaque) > timer_del(d->autoneg_timer); > timer_del(d->mit_timer); > d->mit_timer_on = 0; > - d->mit_irq_level = 0; > + d->mit_irq_level = 1; > d->mit_ide = 0; > memset(d->phy_reg, 0, sizeof d->phy_reg); > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > @@ -1324,7 +1324,7 @@ static int e1000_post_load(void *opaque, int version_id) > if (!(s->compat_flags & E1000_FLAG_MIT)) { > s->mac_reg[ITR] = s->mac_reg[RDTR] = s->mac_reg[RADV] = > s->mac_reg[TADV] = 0; > - s->mit_irq_level = false; > + s->mit_irq_level = true; > } > s->mit_ide = 0; > s->mit_timer_on = false; > --- > > At this point, I'm beginning to realize that the "ActiveHigh" > assumption is rather pervasively baked in throughout the QEMU > source code... > > And I'm wondering whether a ton of changes to make it either > programatically configurable or change the hard-codded assumption > to "ActiveLow" would be feasible, welcome, etc... I personally > would prefer configurable > (e.g. "-machine foo,kernel_irqchip=on,polarity=high", or some such). > > Thanks much for any ideas, > --Gabriel