| Message ID | 1476444335-206575-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 14, 2016 at 01:25:35PM +0200, Igor Mammedov wrote: > it would prevent starting guest with incorrect configs > where interrupts couldn't be delivered to CPUs with > APIC IDs > 255. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v4: > - s/254/255/ in commit message (Radim) > --- > hw/i386/pc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 40eb43b..f7070e0 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -68,6 +68,7 @@ > #include "qapi-visit.h" > #include "qom/cpu.h" > #include "hw/nmi.h" > +#include "hw/i386/intel_iommu.h" > > /* debug PC/ISA interrupts */ > //#define DEBUG_IRQ > @@ -1264,6 +1265,18 @@ void pc_machine_done(Notifier *notifier, void *data) > sizeof(pcms->boot_cpus_le)); > } > } > + > + if (pcms->apic_id_limit > 255) { > + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); > + > + if (!iommu || !iommu->x86_iommu.intr_supported || > + iommu->intr_eim != ON_OFF_AUTO_ON) { > + error_report("current -smp configuration requires " > + "Extended Interrupt Mode enabled. " > + "IOMMU should have eim=on option set"); Suggestion for a follow-up patch: * Error message explaining how to set eim=on if the iommu is available * Error message explaining how to make sure the iommu is created, in case it was not even created.
On Tue, 18 Oct 2016 09:27:37 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Oct 14, 2016 at 01:25:35PM +0200, Igor Mammedov wrote: > > it would prevent starting guest with incorrect configs > > where interrupts couldn't be delivered to CPUs with > > APIC IDs > 255. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > > --- > > v4: > > - s/254/255/ in commit message (Radim) > > --- > > hw/i386/pc.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 40eb43b..f7070e0 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -68,6 +68,7 @@ > > #include "qapi-visit.h" > > #include "qom/cpu.h" > > #include "hw/nmi.h" > > +#include "hw/i386/intel_iommu.h" > > > > /* debug PC/ISA interrupts */ > > //#define DEBUG_IRQ > > @@ -1264,6 +1265,18 @@ void pc_machine_done(Notifier *notifier, void *data) > > sizeof(pcms->boot_cpus_le)); > > } > > } > > + > > + if (pcms->apic_id_limit > 255) { > > + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); > > + > > + if (!iommu || !iommu->x86_iommu.intr_supported || > > + iommu->intr_eim != ON_OFF_AUTO_ON) { > > + error_report("current -smp configuration requires " > > + "Extended Interrupt Mode enabled. " > > + "IOMMU should have eim=on option set"); > > Suggestion for a follow-up patch: > > * Error message explaining how to set eim=on if the iommu is > available > * Error message explaining how to make sure the iommu is created, > in case it was not even created. Reason I didn't include how to create iommu/CLI example is that it could be some other iommu in future so that message could bit rot over time. But I can add description if you'd prefer it. How about something like this: + error_report("current -smp configuration requires " + "Intel IOMMU with Extended Interrupt Mode enabled. " + "To enable IOMMU add to command line: " + "-device intel-iommu,intremap=on,eim=on");
On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote: > On Tue, 18 Oct 2016 09:27:37 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Oct 14, 2016 at 01:25:35PM +0200, Igor Mammedov wrote: > > > it would prevent starting guest with incorrect configs > > > where interrupts couldn't be delivered to CPUs with > > > APIC IDs > 255. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > > > --- > > > v4: > > > - s/254/255/ in commit message (Radim) > > > --- > > > hw/i386/pc.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 40eb43b..f7070e0 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -68,6 +68,7 @@ > > > #include "qapi-visit.h" > > > #include "qom/cpu.h" > > > #include "hw/nmi.h" > > > +#include "hw/i386/intel_iommu.h" > > > > > > /* debug PC/ISA interrupts */ > > > //#define DEBUG_IRQ > > > @@ -1264,6 +1265,18 @@ void pc_machine_done(Notifier *notifier, void *data) > > > sizeof(pcms->boot_cpus_le)); > > > } > > > } > > > + > > > + if (pcms->apic_id_limit > 255) { > > > + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); > > > + > > > + if (!iommu || !iommu->x86_iommu.intr_supported || > > > + iommu->intr_eim != ON_OFF_AUTO_ON) { > > > + error_report("current -smp configuration requires " > > > + "Extended Interrupt Mode enabled. " > > > + "IOMMU should have eim=on option set"); > > > > Suggestion for a follow-up patch: > > > > * Error message explaining how to set eim=on if the iommu is > > available > > * Error message explaining how to make sure the iommu is created, > > in case it was not even created. > Reason I didn't include how to create iommu/CLI example is that > it could be some other iommu in future so that message could > bit rot over time. I see. > > But I can add description if you'd prefer it. > How about something like this: > + error_report("current -smp configuration requires " > + "Intel IOMMU with Extended Interrupt Mode enabled. " > + "To enable IOMMU add to command line: " > + "-device intel-iommu,intremap=on,eim=on"); If there could be other iommu devices in the future, we can show it as an example, but not an instruction to be followed as-is. Maybe we can just say "You can add an IOMMU using: -device intel-iommu,intremap=on,eim=on". It would mean it is one way to add an IOMMU, but not necessarily the only way. BTW, are there any plans to make machine code create an IOMMU automatically if the VM config requires it?
On Tue, 18 Oct 2016 10:55:57 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote: > > On Tue, 18 Oct 2016 09:27:37 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Fri, Oct 14, 2016 at 01:25:35PM +0200, Igor Mammedov wrote: > > > > it would prevent starting guest with incorrect configs > > > > where interrupts couldn't be delivered to CPUs with > > > > APIC IDs > 255. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > > > > --- > > > > v4: > > > > - s/254/255/ in commit message (Radim) > > > > --- > > > > hw/i386/pc.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index 40eb43b..f7070e0 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -68,6 +68,7 @@ > > > > #include "qapi-visit.h" > > > > #include "qom/cpu.h" > > > > #include "hw/nmi.h" > > > > +#include "hw/i386/intel_iommu.h" > > > > > > > > /* debug PC/ISA interrupts */ > > > > //#define DEBUG_IRQ > > > > @@ -1264,6 +1265,18 @@ void pc_machine_done(Notifier *notifier, void *data) > > > > sizeof(pcms->boot_cpus_le)); > > > > } > > > > } > > > > + > > > > + if (pcms->apic_id_limit > 255) { > > > > + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); > > > > + > > > > + if (!iommu || !iommu->x86_iommu.intr_supported || > > > > + iommu->intr_eim != ON_OFF_AUTO_ON) { > > > > + error_report("current -smp configuration requires " > > > > + "Extended Interrupt Mode enabled. " > > > > + "IOMMU should have eim=on option set"); > > > > > > Suggestion for a follow-up patch: > > > > > > * Error message explaining how to set eim=on if the iommu is > > > available > > > * Error message explaining how to make sure the iommu is created, > > > in case it was not even created. > > Reason I didn't include how to create iommu/CLI example is that > > it could be some other iommu in future so that message could > > bit rot over time. > > I see. > > > > > But I can add description if you'd prefer it. > > How about something like this: > > + error_report("current -smp configuration requires " > > + "Intel IOMMU with Extended Interrupt Mode enabled. " > > + "To enable IOMMU add to command line: " > > + "-device intel-iommu,intremap=on,eim=on"); > > If there could be other iommu devices in the future, we can show > it as an example, but not an instruction to be followed as-is. > > Maybe we can just say "You can add an IOMMU using: > -device intel-iommu,intremap=on,eim=on". It would mean it is one > way to add an IOMMU, but not necessarily the only way. ok, I'll amend error message. > > BTW, are there any plans to make machine code create an IOMMU > automatically if the VM config requires it? not that I know of
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 40eb43b..f7070e0 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -68,6 +68,7 @@ #include "qapi-visit.h" #include "qom/cpu.h" #include "hw/nmi.h" +#include "hw/i386/intel_iommu.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ @@ -1264,6 +1265,18 @@ void pc_machine_done(Notifier *notifier, void *data) sizeof(pcms->boot_cpus_le)); } } + + if (pcms->apic_id_limit > 255) { + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default()); + + if (!iommu || !iommu->x86_iommu.intr_supported || + iommu->intr_eim != ON_OFF_AUTO_ON) { + error_report("current -smp configuration requires " + "Extended Interrupt Mode enabled. " + "IOMMU should have eim=on option set"); + exit(EXIT_FAILURE); + } + } } void pc_guest_info_init(PCMachineState *pcms)