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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2637 bytes --]

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.

> > 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?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]