diff mbox series

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

Message ID 20211209220840.14889-4-dwmw2@infradead.org (mailing list archive)
State New, archived
Headers show
Series [v2,1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement | expand

Commit Message

David Woodhouse Dec. 9, 2021, 10:08 p.m. UTC 
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.

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>
---
 hw/i386/intel_iommu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Peter Xu Dec. 16, 2021, 8:47 a.m. UTC | #1 
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.

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

> ---
>  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;
    }

>              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>              return false;
>          }
> -        if (!kvm_enable_x2apic()) {
> -            error_setg(errp, "eim=on requires support on the KVM side"
> -                             "(X2APIC_API, first shipped in v4.7)");
> -            return false;
> -        }
>      }
>  
>      /* Currently only address widths supported are 39 and 48 bits */
> -- 
> 2.31.1
>
David Woodhouse Dec. 17, 2021, 4:51 p.m. UTC | #2 
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?
Peter Xu Dec. 20, 2021, 10:07 a.m. UTC | #3 
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,
diff mbox series

Patch

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()) {
             error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
             return false;
         }
-        if (!kvm_enable_x2apic()) {
-            error_setg(errp, "eim=on requires support on the KVM side"
-                             "(X2APIC_API, first shipped in v4.7)");
-            return false;
-        }
     }
 
     /* Currently only address widths supported are 39 and 48 bits */