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:34:44 +0200 Message-ID: <20140216113444.GD28991@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]:49274 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751169AbaBPL3u (ORCPT ); Sun, 16 Feb 2014 06:29:50 -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); > } > > @@ -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; Hmm no this is all wrong, from API point of view, devices shoud not care about value of interrupt. They just assert/deassert interrupts. It so happens that 1 means assert 0 means deassert. It's the PCI host or the PIC that needs to flip it. It can't be host ATM because PIC does the logical OR so it must be encoded as 1 == assert. So how about simply - qemu_set_irq(lpc->pic[pic_irq], pic_level); + qemu_set_irq(lpc->pic[pic_irq], !pic_level); plus the ACPI tweaks ... ? > --- > > 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 I don't think we need to make this configurable unless there's real hardware that makes PCI active low. Don't give up yet :) -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEzv1-00035k-D9 for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:30:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WEzuv-00008P-77 for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:29:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30640) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEzuu-00008A-VY for qemu-devel@nongnu.org; Sun, 16 Feb 2014 06:29:49 -0500 Date: Sun, 16 Feb 2014 13:34:44 +0200 From: "Michael S. Tsirkin" Message-ID: <20140216113444.GD28991@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); > } > > @@ -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; Hmm no this is all wrong, from API point of view, devices shoud not care about value of interrupt. They just assert/deassert interrupts. It so happens that 1 means assert 0 means deassert. It's the PCI host or the PIC that needs to flip it. It can't be host ATM because PIC does the logical OR so it must be encoded as 1 == assert. So how about simply - qemu_set_irq(lpc->pic[pic_irq], pic_level); + qemu_set_irq(lpc->pic[pic_irq], !pic_level); plus the ACPI tweaks ... ? > --- > > 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 I don't think we need to make this configurable unless there's real hardware that makes PCI active low. Don't give up yet :) -- MST