| Message ID | 20160922210432.18680-6-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 22/09/2016 23:04, Radim Krčmář wrote: > Cluster x2APIC cannot work without KVM's x2apic API when the maximal > APIC ID is > 8. Make the code simpler by completely forbidding EIM > without KVM's x2apic API. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > I think it the dependency would be nicer in the eim setter, but the > other dependency, for interrupt remapping, isn't there and I didn't > venture for reasons. > --- > hw/i386/intel_iommu.c | 7 +++++++ > target-i386/kvm-stub.c | 5 +++++ > target-i386/kvm.c | 13 +++++++++++++ > target-i386/kvm_i386.h | 1 + > 4 files changed, 26 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 269e37e71af4..0304a1bf6f1d 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -32,6 +32,7 @@ > #include "hw/pci-host/q35.h" > #include "sysemu/kvm.h" > #include "hw/i386/apic_internal.h" > +#include "kvm_i386.h" > > /*#define DEBUG_INTEL_IOMMU*/ > #ifdef DEBUG_INTEL_IOMMU > @@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp) > "kernel-irqchip=on, please use 'split|off'."); > exit(1); > } > + > + if (s->eim_supported && kvm_irqchip_in_kernel() && > + !kvm_enable_x2apic()) { > + error_report("EIM requires support from the KVM side (X2APIC_API)."); > + exit(1); > + } > } > > static void vtd_class_init(ObjectClass *klass, void *data) > diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c > index cdf15061091d..bda4dc2f0c57 100644 > --- a/target-i386/kvm-stub.c > +++ b/target-i386/kvm-stub.c > @@ -25,6 +25,11 @@ bool kvm_has_smm(void) > return 1; > } > > +bool kvm_enable_x2apic(void) > +{ > + return false; > +} > + > /* This function is only called inside conditionals which we > * rely on the compiler to optimize out when CONFIG_KVM is not > * defined. > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index f1ad805665ad..4c0a4df5eaea 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void) > return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing(); > } > > +static bool kvm_x2apic_api_set_flags(uint64_t flags) > +{ > + KVMState *s = KVM_STATE(current_machine->accelerator); > + > + return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); > +} > + > +bool kvm_enable_x2apic(void) > +{ > + return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS | > + KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK); > +} > + > static int kvm_get_tsc(CPUState *cs) > { > X86CPU *cpu = X86_CPU(cs); > diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h > index 42b00af1b1c3..559dd8b5acd2 100644 > --- a/target-i386/kvm_i386.h > +++ b/target-i386/kvm_i386.h > @@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector, > int kvm_device_msix_assign(KVMState *s, uint32_t dev_id); > int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id); > > +bool kvm_enable_x2apic(void); > #endif > Since the whole IOMMU feature is new and somewhat experimental, I think it's okay to just make EIM the default for >=2.8 machine types if KVM is on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and false otherwise, and pc-2.7 would set eim=off). It means requiring kernel 4.8 by default, but I don't think it's a big deal. Paolo
On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote: > Since the whole IOMMU feature is new and somewhat experimental, I think > it's okay to just make EIM the default for >=2.8 machine types if KVM is > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and > false otherwise, and pc-2.7 would set eim=off). It means requiring > kernel 4.8 by default, but I don't think it's a big deal. I think the problem is, even we have KVM support for x2apic, we are still losing QEMU part. And guests with cluster x2apic and >8 vcpus will not working properly on device interrupts, which can be very confusing to people (it can boot, but some devices just don't work properly, and they won't see useful information in guest dmesg). Thanks, -- peterx
On 23/09/2016 12:02, Peter Xu wrote: > On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote: >> > Since the whole IOMMU feature is new and somewhat experimental, I think >> > it's okay to just make EIM the default for >=2.8 machine types if KVM is >> > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and >> > false otherwise, and pc-2.7 would set eim=off). It means requiring >> > kernel 4.8 by default, but I don't think it's a big deal. > I think the problem is, even we have KVM support for x2apic, we are > still losing QEMU part. And guests with cluster x2apic and >8 vcpus > will not working properly on device interrupts, which can be very > confusing to people (it can boot, but some devices just don't work > properly, and they won't see useful information in guest dmesg). Yes, that's why I suggested EIM=on by default. Paolo
On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote: > > > On 23/09/2016 12:02, Peter Xu wrote: > > On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote: > >> > Since the whole IOMMU feature is new and somewhat experimental, I think > >> > it's okay to just make EIM the default for >=2.8 machine types if KVM is > >> > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and > >> > false otherwise, and pc-2.7 would set eim=off). It means requiring > >> > kernel 4.8 by default, but I don't think it's a big deal. > > I think the problem is, even we have KVM support for x2apic, we are > > still losing QEMU part. And guests with cluster x2apic and >8 vcpus > > will not working properly on device interrupts, which can be very > > confusing to people (it can boot, but some devices just don't work > > properly, and they won't see useful information in guest dmesg). > > Yes, that's why I suggested EIM=on by default. I am confused. :( Why not we just keep people from that wrong configuration by default, until we have x2apic in QEMU? -- peterx
On 23/09/2016 12:12, Peter Xu wrote: > On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote: >> >> >> On 23/09/2016 12:02, Peter Xu wrote: >>> On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote: >>>>> Since the whole IOMMU feature is new and somewhat experimental, I think >>>>> it's okay to just make EIM the default for >=2.8 machine types if KVM is >>>>> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and >>>>> false otherwise, and pc-2.7 would set eim=off). It means requiring >>>>> kernel 4.8 by default, but I don't think it's a big deal. >>> I think the problem is, even we have KVM support for x2apic, we are >>> still losing QEMU part. And guests with cluster x2apic and >8 vcpus >>> will not working properly on device interrupts, which can be very >>> confusing to people (it can boot, but some devices just don't work >>> properly, and they won't see useful information in guest dmesg). >> >> Yes, that's why I suggested EIM=on by default. > > I am confused. :( > > Why not we just keep people from that wrong configuration by default, > until we have x2apic in QEMU? Do you mean Igor's patches? I expect that they will go in pretty much at the same time as Radim's. Paolo
On Fri, Sep 23, 2016 at 12:39:19PM +0200, Paolo Bonzini wrote: > > > On 23/09/2016 12:12, Peter Xu wrote: > > On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 23/09/2016 12:02, Peter Xu wrote: > >>> On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote: > >>>>> Since the whole IOMMU feature is new and somewhat experimental, I think > >>>>> it's okay to just make EIM the default for >=2.8 machine types if KVM is > >>>>> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and > >>>>> false otherwise, and pc-2.7 would set eim=off). It means requiring > >>>>> kernel 4.8 by default, but I don't think it's a big deal. > >>> I think the problem is, even we have KVM support for x2apic, we are > >>> still losing QEMU part. And guests with cluster x2apic and >8 vcpus > >>> will not working properly on device interrupts, which can be very > >>> confusing to people (it can boot, but some devices just don't work > >>> properly, and they won't see useful information in guest dmesg). > >> > >> Yes, that's why I suggested EIM=on by default. > > > > I am confused. :( > > > > Why not we just keep people from that wrong configuration by default, > > until we have x2apic in QEMU? > > Do you mean Igor's patches? I expect that they will go in pretty much > at the same time as Radim's. Ah! Yes we have x2apic all here... So I totally agree we should set it on as default. (My mistake of not noticing the truth) Thanks, -- peterx
On Thu, 22 Sep 2016 23:04:32 +0200 Radim Krčmář <rkrcmar@redhat.com> wrote: > Cluster x2APIC cannot work without KVM's x2apic API when the maximal > APIC ID is > 8. Make the code simpler by completely forbidding EIM > without KVM's x2apic API. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > I think it the dependency would be nicer in the eim setter, but the > other dependency, for interrupt remapping, isn't there and I didn't > venture for reasons. > --- > hw/i386/intel_iommu.c | 7 +++++++ > target-i386/kvm-stub.c | 5 +++++ > target-i386/kvm.c | 13 +++++++++++++ > target-i386/kvm_i386.h | 1 + > 4 files changed, 26 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 269e37e71af4..0304a1bf6f1d 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -32,6 +32,7 @@ > #include "hw/pci-host/q35.h" > #include "sysemu/kvm.h" > #include "hw/i386/apic_internal.h" > +#include "kvm_i386.h" > > /*#define DEBUG_INTEL_IOMMU*/ > #ifdef DEBUG_INTEL_IOMMU > @@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp) > "kernel-irqchip=on, please use 'split|off'."); > exit(1); > } > + > + if (s->eim_supported && kvm_irqchip_in_kernel() && > + !kvm_enable_x2apic()) { > + error_report("EIM requires support from the KVM side (X2APIC_API)."); > + exit(1); > + } > } > > static void vtd_class_init(ObjectClass *klass, void *data) > diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c > index cdf15061091d..bda4dc2f0c57 100644 > --- a/target-i386/kvm-stub.c > +++ b/target-i386/kvm-stub.c > @@ -25,6 +25,11 @@ bool kvm_has_smm(void) > return 1; > } > > +bool kvm_enable_x2apic(void) > +{ > + return false; > +} > + > /* This function is only called inside conditionals which we > * rely on the compiler to optimize out when CONFIG_KVM is not > * defined. > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index f1ad805665ad..4c0a4df5eaea 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void) > return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing(); > } > > +static bool kvm_x2apic_api_set_flags(uint64_t flags) > +{ > + KVMState *s = KVM_STATE(current_machine->accelerator); > + > + return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); > +} > + > +bool kvm_enable_x2apic(void) > +{ maybe it should be: static bool enabled_x2apic; if (!enabled_x2apic && kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS | KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)) { enabled_x2apic = true; } return enabled_x2apic; so that it could be called from multiple sites. > + return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS | > + KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK); > +} > + > static int kvm_get_tsc(CPUState *cs) > { > X86CPU *cpu = X86_CPU(cs); > diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h > index 42b00af1b1c3..559dd8b5acd2 100644 > --- a/target-i386/kvm_i386.h > +++ b/target-i386/kvm_i386.h > @@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector, > int kvm_device_msix_assign(KVMState *s, uint32_t dev_id); > int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id); > > +bool kvm_enable_x2apic(void); > #endif
2016-09-23 11:27+0200, Paolo Bonzini: > Since the whole IOMMU feature is new and somewhat experimental, I think > it's okay to just make EIM the default for >=2.8 machine types if KVM is > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and Sounds good. > false otherwise, and pc-2.7 would set eim=off). What about eim=on in pc-2.7, to avoid breaking migration?
----- Original Message ----- > From: "Radim Krčmář" <rkrcmar@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: qemu-devel@nongnu.org, "Peter Xu" <peterx@redhat.com>, "Igor Mammedov" <imammedo@redhat.com>, "Richard Henderson" > <rth@twiddle.net>, "Eduardo Habkost" <ehabkost@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com> > Sent: Tuesday, September 27, 2016 4:01:39 PM > Subject: Re: [PATCH 5/5] intel_iommu: do not allow EIM without KVM support > > 2016-09-23 11:27+0200, Paolo Bonzini: > > Since the whole IOMMU feature is new and somewhat experimental, I think > > it's okay to just make EIM the default for >=2.8 machine types if KVM is > > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and > > Sounds good. BTW this also means KVM+vIOMMU requires 4.8. Let's remember to document it in the release notes. > > false otherwise, and pc-2.7 would set eim=off). > > What about eim=on in pc-2.7, to avoid breaking migration? Yup, just a thinko. Paolo
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 269e37e71af4..0304a1bf6f1d 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -32,6 +32,7 @@ #include "hw/pci-host/q35.h" #include "sysemu/kvm.h" #include "hw/i386/apic_internal.h" +#include "kvm_i386.h" /*#define DEBUG_INTEL_IOMMU*/ #ifdef DEBUG_INTEL_IOMMU @@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp) "kernel-irqchip=on, please use 'split|off'."); exit(1); } + + if (s->eim_supported && kvm_irqchip_in_kernel() && + !kvm_enable_x2apic()) { + error_report("EIM requires support from the KVM side (X2APIC_API)."); + exit(1); + } } static void vtd_class_init(ObjectClass *klass, void *data) diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c index cdf15061091d..bda4dc2f0c57 100644 --- a/target-i386/kvm-stub.c +++ b/target-i386/kvm-stub.c @@ -25,6 +25,11 @@ bool kvm_has_smm(void) return 1; } +bool kvm_enable_x2apic(void) +{ + return false; +} + /* This function is only called inside conditionals which we * rely on the compiler to optimize out when CONFIG_KVM is not * defined. diff --git a/target-i386/kvm.c b/target-i386/kvm.c index f1ad805665ad..4c0a4df5eaea 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void) return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing(); } +static bool kvm_x2apic_api_set_flags(uint64_t flags) +{ + KVMState *s = KVM_STATE(current_machine->accelerator); + + return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); +} + +bool kvm_enable_x2apic(void) +{ + return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS | + KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK); +} + static int kvm_get_tsc(CPUState *cs) { X86CPU *cpu = X86_CPU(cs); diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h index 42b00af1b1c3..559dd8b5acd2 100644 --- a/target-i386/kvm_i386.h +++ b/target-i386/kvm_i386.h @@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector, int kvm_device_msix_assign(KVMState *s, uint32_t dev_id); int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id); +bool kvm_enable_x2apic(void); #endif
Cluster x2APIC cannot work without KVM's x2apic API when the maximal APIC ID is > 8. Make the code simpler by completely forbidding EIM without KVM's x2apic API. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- I think it the dependency would be nicer in the eim setter, but the other dependency, for interrupt remapping, isn't there and I didn't venture for reasons. --- hw/i386/intel_iommu.c | 7 +++++++ target-i386/kvm-stub.c | 5 +++++ target-i386/kvm.c | 13 +++++++++++++ target-i386/kvm_i386.h | 1 + 4 files changed, 26 insertions(+)