Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[Peter Xu]

On Fri, Dec 17, 2021 at 04:51:20PM +0000, David Woodhouse wrote:
> On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote:
> > Hi, David,
> > 
> > On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> > > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> > > interrupt remapping even if we can't address CPUs above 254. Kind of
> > > pointless, but still functional.
> > 
> > We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
> > still enable IR without x2apic even with current code?
> > 
> > Could you elaborate what's the use scenario for this patch?  Thanks in 
> > advance.
> 
> You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You
> just can't have any CPUs with an APIC ID > 254.
> 
> But qemu is going to bail out *anyway* if you attempt to have CPUs with
> APIC IDs above 254 without the corresponding kernel-side support, so
> there's no need to check it here.

Ah OK.

> 
> > > The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> > > allow CPUs above 254 even without an IOMMU, so allow that to happen
> > > elsewhere.
> > > 
> > > However, we do require the *split* irqchip in order to rewrite I/OAPIC
> > > destinations. So fix that check while we're here.
> > > 
> > > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > I think the r-b and a-b should be for patch 2 not this one? :)
> > 
> 
> Yes, I think I must have swapped those round. Thanks.
> 
> > > ---
> > >  hw/i386/intel_iommu.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index bd288d45bb..0d1c72f08e 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, 
> > > Error **errp)
> > >                                                ON_OFF_AUTO_ON : 
> > > ON_OFF_AUTO_OFF;
> > >      }
> > >      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> > > -        if (!kvm_irqchip_in_kernel()) {
> > > +        if (!kvm_irqchip_is_split()) {
> > 
> > I think this is okay, but note that we'll already fail if !split in
> > x86_iommu_realize():
> > 
> >     bool irq_all_kernel = kvm_irqchip_in_kernel() && 
> > !kvm_irqchip_is_split();
> > 
> >     /* Both Intel and AMD IOMMU IR only support 
> > "kernel-irqchip={off|split}" */
> >     if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
> >         error_setg(errp, "Interrupt Remapping cannot work with "
> >                          "kernel-irqchip=on, please use 'split|off'.");
> >         return;
> >     }
> 
> OK, then perhaps the entire check is redundant?

Yes, maybe.

It also reminded me that this is the only place that we used the "buggy_eim"
variable.  If we drop this chunk, that flag will become meaningless.

If we look back, it seems to decides whether we should call kvm_enable_x2apic()
at all, so as to be compatible with old qemus.  Please see commit fb506e701e
("intel_iommu: reject broken EIM", 2016-10-17).

hw_compat_2_7 has:

    { "intel-iommu", "x-buggy-eim", "true" },

It means kvm_enable_x2apic() (at least before commit c1bb5418e3 of yours)
should be skipped for 2.7 or older version of QEMU binaries.

Now after commit c1bb5418e3 we'll unconditionally call kvm_enable_x2apic() in
x86_cpu_load_model() anyway, even if x-buggy-eim=on.  IIUC it violates with the
original purpose of commit fb506e701e.

However maybe it's not necessary to maintain that awkward/buggy compatibility
at all for those old qemu binaries.  I can hardly imagine someone uses vIOMMU
2.7- versions for production purposes, and for relying on that buggy behavior
to work.

To summarize: I'm wondering whether we should also drop buggy-eim as a whole..

Thanks,

-- 
Peter Xu