Re: [Qemu-devel] [PATCH v4 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 09:27:37 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > > Reviewed-by: Radim Krčmář <address@hidden>
> > > ---
> > > 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