Re: [Qemu-devel] [PATCH v4 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs
On Tue, 18 Oct 2016 10:55:57 -0200
Eduardo Habkost 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 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
> > > > Reviewed-by: Radim Krčmář
> > > > ---
> > > > 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
Re: [Qemu-devel] [PATCH v4 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs
On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 09:27:37 -0200
> Eduardo Habkost 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
> > > Reviewed-by: Radim Krčmář
> > > ---
> > > 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?
--
Eduardo
Re: [Qemu-devel] [PATCH v4 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs
On Tue, 18 Oct 2016 09:27:37 -0200
Eduardo Habkost 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
> > Reviewed-by: Radim Krčmář
> > ---
> > 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");
Re: [Qemu-devel] [PATCH v4 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs
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
> Reviewed-by: Radim Krčmář
> ---
> 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.
--
Eduardo
