From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [RFC PATCH v2] kvm: x86: ignore ioapic polarity Date: Sun, 2 Mar 2014 16:55:37 +0200 Message-ID: <20140302145537.GF30372@redhat.com> References: <2CEB9F8C-E983-4182-A514-44EC568E18D8@suse.de> <20140216114151.GB30056@redhat.com> <1392562020.15608.437.camel@ul30vt.home> <20140216162300.GI30056@redhat.com> <20140227170549.GA23037@redhat.com> <20140227214102.GG17184@ERROL.INI.CMU.EDU> <530FBC9F.8080800@redhat.com> <20140227231312.GH17184@ERROL.INI.CMU.EDU> <530FCACF.40100@redhat.com> <20140228040617.GA22103@crash.ini.cmu.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, pbonzini@redhat.com, alex.williamson@redhat.com, eddie.dong@intel.com, agraf@suse.de, qemu-devel@nongnu.org To: "Gabriel L. Somlo" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39129 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbaCBOuU (ORCPT ); Sun, 2 Mar 2014 09:50:20 -0500 Content-Disposition: inline In-Reply-To: <20140228040617.GA22103@crash.ini.cmu.edu> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Feb 27, 2014 at 11:06:17PM -0500, Gabriel L. Somlo wrote: > Both QEMU and KVM have already accumulated a significant number of > optimizations based on the hard-coded assumption that ioapic polarity > will always use the ActiveHigh convention, where the logical and > physical states of level-triggered irq lines always match (i.e., > active(asserted) == high == 1, inactive == low == 0). QEMU guests > are expected to follow directions given via ACPI and configure the > ioapic with polarity 0 (ActiveHigh). However, even when misbehaving > guests (e.g. OS X <= 10.9) set the ioapic polarity to 1 (ActiveLow), > QEMU will still use the ActiveHigh signaling convention when > interfacing with KVM. > > This patch modifies KVM to completely ignore ioapic polarity as set by > the guest OS, enabling misbehaving guests to work alongside those which > comply with the ActiveHigh polarity specified by QEMU's ACPI tables. > > Signed-off-by: Michael S. Tsirkin > Signed-off-by: Gabriel L. Somlo > --- > > On Fri, Feb 28, 2014 at 12:31:27AM +0100, Paolo Bonzini wrote: > >>>This is a much better description. Can you turn it into a patch to > >>>Documentation/virtual/kvm/api.txt and a more complete commit > >>>message? > > OK, let me know what you all think of this version. Looks good to me. Acked-by: Michael S. Tsirkin > > If you change ACPI tables to ActiveLow, and leave the polarity > > inversion in hw/intc/ioapic.c, then it will matter. > > I only changed ACPI to ActiveLow to cause Linux to misbehave in the > same way OS X does, it was a temporary/test hack. ACPI should stay > ActiveHigh to reflect the rest of QEMU/KVM, and misbehaving guest OSs > should ignore it at their own peril :) I actually think now that you've fixed intc.c as well, we should change it as the next step. > > >(Hmmm, maybe this one of the reasons I never got OS X to boot without > > >-enable-kvm... I should look at the QEMU hw/intc/ioapic*.c, and see > > >if *it* cares about guest-configured polarity, and maybe get it to > > >*stop* caring :) > > > Exactly my point. :) > > So this patch gets the accelerated path to work regardless of how > well-behaved a guest OS may or may not be w.r.t. ACPI and polarity. > I'll try to find out whether it's possible to be *extra* nice to these > types of guests by also allowing them to ignore ACPI polarity when not > using acceleration :) > > Thanks again, > Gabriel > > Documentation/virtual/kvm/api.txt | 18 ++++++++++++++++++ > include/uapi/linux/kvm.h | 1 + > virt/kvm/ioapic.c | 1 - > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 6cd63a9..0492a94 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -1365,6 +1365,24 @@ struct kvm_irq_routing_msi { > __u32 pad; > }; > > +NOTE: For each level-triggered interrupt managed by a virtual ioapic, > +the guest OS may set a polarity value (bit 13 of each corresponding I/O > +redirection table register). The polarity bit defines the relationship > +between an irq line's logical state (active/asserted = 1, inactive = 0) > +and its physical state (high = 1, low = 0). When the polarity bit is 0 > +(ActiveHigh), logical and physical states are matched (active == high == 1, > +inactive == low == 0). When polarity is set to 1 (ActiveLow), logical and > +physical state are opposites (logical == !physical). Typically, guests are > +expected to use the same polarity across all level-triggered interrupts, > +as directed by ACPI. Historically, both KVM and QEMU have accumulated a > +significant number of optimizations based on the hard-coded assumption > +that polarity will always be set to ActiveHigh. When interfacing with KVM, > +QEMU will use the ActiveHigh convention for all level-triggered irqs, > +regardless of how the guest OS has configured the polarity bits in the > +ioapic registers. As such, KVM must also ignore these bits, and always act > +as if the logical and physical states of an irq line exactly match each > +other (i.e., follow the ActiveHigh convention regardless of polarity). > + > > 4.53 KVM_ASSIGN_SET_MSIX_NR > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 932d7f2..5bfc0e6 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -675,6 +675,7 @@ struct kvm_ppc_smmu_info { > #define KVM_CAP_SPAPR_MULTITCE 94 > #define KVM_CAP_EXT_EMUL_CPUID 95 > #define KVM_CAP_HYPERV_TIME 96 > +#define KVM_CAP_X86_IOAPIC_POLARITY_IGNORED 97 > > #ifdef KVM_CAP_IRQ_ROUTING > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > index ce9ed99..1539d37 100644 > --- a/virt/kvm/ioapic.c > +++ b/virt/kvm/ioapic.c > @@ -328,7 +328,6 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id, > irq_level = __kvm_irq_line_state(&ioapic->irq_states[irq], > irq_source_id, level); > entry = ioapic->redirtbl[irq]; > - irq_level ^= entry.fields.polarity; > if (!irq_level) { > ioapic->irr &= ~mask; > ret = 1; > -- > 1.8.1.4 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WK7ij-0006y0-Hi for qemu-devel@nongnu.org; Sun, 02 Mar 2014 09:50:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WK7ic-0008UG-VX for qemu-devel@nongnu.org; Sun, 02 Mar 2014 09:50:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46731) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WK7ic-0008U4-NG for qemu-devel@nongnu.org; Sun, 02 Mar 2014 09:50:18 -0500 Date: Sun, 2 Mar 2014 16:55:37 +0200 From: "Michael S. Tsirkin" Message-ID: <20140302145537.GF30372@redhat.com> References: <2CEB9F8C-E983-4182-A514-44EC568E18D8@suse.de> <20140216114151.GB30056@redhat.com> <1392562020.15608.437.camel@ul30vt.home> <20140216162300.GI30056@redhat.com> <20140227170549.GA23037@redhat.com> <20140227214102.GG17184@ERROL.INI.CMU.EDU> <530FBC9F.8080800@redhat.com> <20140227231312.GH17184@ERROL.INI.CMU.EDU> <530FCACF.40100@redhat.com> <20140228040617.GA22103@crash.ini.cmu.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140228040617.GA22103@crash.ini.cmu.edu> Subject: Re: [Qemu-devel] [RFC PATCH v2] kvm: x86: ignore ioapic polarity List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: kvm@vger.kernel.org, eddie.dong@intel.com, agraf@suse.de, qemu-devel@nongnu.org, alex.williamson@redhat.com, pbonzini@redhat.com On Thu, Feb 27, 2014 at 11:06:17PM -0500, Gabriel L. Somlo wrote: > Both QEMU and KVM have already accumulated a significant number of > optimizations based on the hard-coded assumption that ioapic polarity > will always use the ActiveHigh convention, where the logical and > physical states of level-triggered irq lines always match (i.e., > active(asserted) == high == 1, inactive == low == 0). QEMU guests > are expected to follow directions given via ACPI and configure the > ioapic with polarity 0 (ActiveHigh). However, even when misbehaving > guests (e.g. OS X <= 10.9) set the ioapic polarity to 1 (ActiveLow), > QEMU will still use the ActiveHigh signaling convention when > interfacing with KVM. > > This patch modifies KVM to completely ignore ioapic polarity as set by > the guest OS, enabling misbehaving guests to work alongside those which > comply with the ActiveHigh polarity specified by QEMU's ACPI tables. > > Signed-off-by: Michael S. Tsirkin > Signed-off-by: Gabriel L. Somlo > --- > > On Fri, Feb 28, 2014 at 12:31:27AM +0100, Paolo Bonzini wrote: > >>>This is a much better description. Can you turn it into a patch to > >>>Documentation/virtual/kvm/api.txt and a more complete commit > >>>message? > > OK, let me know what you all think of this version. Looks good to me. Acked-by: Michael S. Tsirkin > > If you change ACPI tables to ActiveLow, and leave the polarity > > inversion in hw/intc/ioapic.c, then it will matter. > > I only changed ACPI to ActiveLow to cause Linux to misbehave in the > same way OS X does, it was a temporary/test hack. ACPI should stay > ActiveHigh to reflect the rest of QEMU/KVM, and misbehaving guest OSs > should ignore it at their own peril :) I actually think now that you've fixed intc.c as well, we should change it as the next step. > > >(Hmmm, maybe this one of the reasons I never got OS X to boot without > > >-enable-kvm... I should look at the QEMU hw/intc/ioapic*.c, and see > > >if *it* cares about guest-configured polarity, and maybe get it to > > >*stop* caring :) > > > Exactly my point. :) > > So this patch gets the accelerated path to work regardless of how > well-behaved a guest OS may or may not be w.r.t. ACPI and polarity. > I'll try to find out whether it's possible to be *extra* nice to these > types of guests by also allowing them to ignore ACPI polarity when not > using acceleration :) > > Thanks again, > Gabriel > > Documentation/virtual/kvm/api.txt | 18 ++++++++++++++++++ > include/uapi/linux/kvm.h | 1 + > virt/kvm/ioapic.c | 1 - > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 6cd63a9..0492a94 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -1365,6 +1365,24 @@ struct kvm_irq_routing_msi { > __u32 pad; > }; > > +NOTE: For each level-triggered interrupt managed by a virtual ioapic, > +the guest OS may set a polarity value (bit 13 of each corresponding I/O > +redirection table register). The polarity bit defines the relationship > +between an irq line's logical state (active/asserted = 1, inactive = 0) > +and its physical state (high = 1, low = 0). When the polarity bit is 0 > +(ActiveHigh), logical and physical states are matched (active == high == 1, > +inactive == low == 0). When polarity is set to 1 (ActiveLow), logical and > +physical state are opposites (logical == !physical). Typically, guests are > +expected to use the same polarity across all level-triggered interrupts, > +as directed by ACPI. Historically, both KVM and QEMU have accumulated a > +significant number of optimizations based on the hard-coded assumption > +that polarity will always be set to ActiveHigh. When interfacing with KVM, > +QEMU will use the ActiveHigh convention for all level-triggered irqs, > +regardless of how the guest OS has configured the polarity bits in the > +ioapic registers. As such, KVM must also ignore these bits, and always act > +as if the logical and physical states of an irq line exactly match each > +other (i.e., follow the ActiveHigh convention regardless of polarity). > + > > 4.53 KVM_ASSIGN_SET_MSIX_NR > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 932d7f2..5bfc0e6 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -675,6 +675,7 @@ struct kvm_ppc_smmu_info { > #define KVM_CAP_SPAPR_MULTITCE 94 > #define KVM_CAP_EXT_EMUL_CPUID 95 > #define KVM_CAP_HYPERV_TIME 96 > +#define KVM_CAP_X86_IOAPIC_POLARITY_IGNORED 97 > > #ifdef KVM_CAP_IRQ_ROUTING > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > index ce9ed99..1539d37 100644 > --- a/virt/kvm/ioapic.c > +++ b/virt/kvm/ioapic.c > @@ -328,7 +328,6 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id, > irq_level = __kvm_irq_line_state(&ioapic->irq_states[irq], > irq_source_id, level); > entry = ioapic->redirtbl[irq]; > - irq_level ^= entry.fields.polarity; > if (!irq_level) { > ioapic->irr &= ~mask; > ret = 1; > -- > 1.8.1.4