[PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

It's true that when vcpus<=255 we don't require the length of 32bit APIC
IDs.  However here since we already have EIM=ON it means the hypervisor
will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
that wants to boot a VM with >=9 but <=255 vcpus with:

  -device intel-iommu,intremap=on

For anyone who does not want to enable x2apic, we can use eim=off in the
intel-iommu parameters to skip enabling KVM x2apic.

This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
keeping the valid bit on checking split irqchip, but revert the other change.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Claudio Fontana <cfontana@suse.de>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 05d53a1aa9..6524c2ee32 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
             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.32.0

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Jason Wang]

On Thu, Sep 22, 2022 at 12:12 AM Peter Xu <peterx@redhat.com> wrote:
>
> It's true that when vcpus<=255 we don't require the length of 32bit APIC
> IDs.  However here since we already have EIM=ON it means the hypervisor
> will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
> that wants to boot a VM with >=9 but <=255 vcpus with:
>
>   -device intel-iommu,intremap=on
>
> For anyone who does not want to enable x2apic, we can use eim=off in the
> intel-iommu parameters to skip enabling KVM x2apic.
>
> This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> keeping the valid bit on checking split irqchip, but revert the other change.
>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Claudio Fontana <cfontana@suse.de>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05d53a1aa9..6524c2ee32 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>              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;
> +        }

I wonder if we need some work on the migration compatibility here
(though it could be tricky).

Thanks

>      }
>
>      /* Currently only address widths supported are 39 and 48 bits */
> --
> 2.32.0
>

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

On Thu, Sep 22, 2022 at 09:32:54AM +0800, Jason Wang wrote:
> > +        if (!kvm_enable_x2apic()) {
> > +            error_setg(errp, "eim=on requires support on the KVM side"
> > +                             "(X2APIC_API, first shipped in v4.7)");
> > +            return false;
> > +        }
> 
> I wonder if we need some work on the migration compatibility here
> (though it could be tricky).

Right, as I replied to Igor, it's just challenging and debateable whether
keeping the old behavior helps in any cases..  Thanks,

-- 
Peter Xu

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Igor Mammedov]

On Wed, 21 Sep 2022 12:12:27 -0400
Peter Xu <peterx@redhat.com> wrote:

> It's true that when vcpus<=255 we don't require the length of 32bit APIC
> IDs.  However here since we already have EIM=ON it means the hypervisor
> will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
> that wants to boot a VM with >=9 but <=255 vcpus with:
> 
>   -device intel-iommu,intremap=on
> 
> For anyone who does not want to enable x2apic, we can use eim=off in the
> intel-iommu parameters to skip enabling KVM x2apic.
> 
> This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> keeping the valid bit on checking split irqchip, but revert the other change.
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Claudio Fontana <cfontana@suse.de>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05d53a1aa9..6524c2ee32 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>              return false;
>          }
> +        if (!kvm_enable_x2apic()) {

above 'check' has side-effects
if it's supposed to be a check it would be better to use kvm_has_x2apic_api()
instead.

Also 77250171bdc says:
"
    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.
"

Looking for that elsewhere, it looks like commit dc89f32d92b was supposed
to take care of removed chunk, but that is not reachable because of > 255 vCPUs"

Likely 77250171bdc just exposed a bug in dc89f32d92b, where
the later removed kvm_enable_x2apic() always called (with split irqchip)
and made it called only when > 255 vCPUs.

So migration wise it looks like all version with it and less than 255 cpus
are broken.

Wait earlier c1bb5418e3, introduced that
   kvm_irqchip_is_split() && kvm_enable_x2apic()
'condition', also without any compat machinery to keep old behavior.

And before that kvm_enable_x2apic() was affecting only configuration
with intel_iommu (fb506e701e9b).

I'm not sure if anything could be salvaged here migration wise

PS:
I'd keep kvm_enable_x2apic() only in corrected x86_cpus_init()
and use kvm_has_x2apic_api() elsewhere for checks and bailing out.


> +            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 */

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

On Thu, Sep 22, 2022 at 03:46:17PM +0200, Igor Mammedov wrote:
> On Wed, 21 Sep 2022 12:12:27 -0400
> Peter Xu <peterx@redhat.com> wrote:
> 
> > It's true that when vcpus<=255 we don't require the length of 32bit APIC
> > IDs.  However here since we already have EIM=ON it means the hypervisor
> > will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> > EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> > even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
> > that wants to boot a VM with >=9 but <=255 vcpus with:
> > 
> >   -device intel-iommu,intremap=on
> > 
> > For anyone who does not want to enable x2apic, we can use eim=off in the
> > intel-iommu parameters to skip enabling KVM x2apic.
> > 
> > This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> > keeping the valid bit on checking split irqchip, but revert the other change.
> > 
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Claudio Fontana <cfontana@suse.de>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 05d53a1aa9..6524c2ee32 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
> >              return false;
> >          }
> > +        if (!kvm_enable_x2apic()) {
> 
> above 'check' has side-effects
> if it's supposed to be a check it would be better to use kvm_has_x2apic_api()
> instead.

A check is not what I wanted.

As stated in the commit message, since for some reason EIM is enabled on
the VT-d device already, we need to enable x2apic for the whole guest
(including KVM) to match with EIM=on being declared even if vcpus<255.

> 
> Also 77250171bdc says:
> "
>     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.
> "
> 
> Looking for that elsewhere, it looks like commit dc89f32d92b was supposed
> to take care of removed chunk, but that is not reachable because of > 255 vCPUs"
> 
> Likely 77250171bdc just exposed a bug in dc89f32d92b, where
> the later removed kvm_enable_x2apic() always called (with split irqchip)
> and made it called only when > 255 vCPUs.
> 
> So migration wise it looks like all version with it and less than 255 cpus
> are broken.
> 
> Wait earlier c1bb5418e3, introduced that
>    kvm_irqchip_is_split() && kvm_enable_x2apic()
> 'condition', also without any compat machinery to keep old behavior.

There's actually some attempt to be compatible, with:

 GlobalProperty pc_compat_5_1[] = {
     { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
+    { TYPE_X86_CPU, "kvm-msi-ext-dest-id", "off" },
 };

But I don't think that's strictly correct, because afaict that only
controls whether guest enables it or not (I can only see Linux does it that
way; no idea whether that's detected to other OSes from the PV interfaces).
The KVM x2apic will still be enabled even on old machines I think, as you
mentioned.

> 
> And before that kvm_enable_x2apic() was affecting only configuration
> with intel_iommu (fb506e701e9b).

Right, afaict that's what we "officially" support.

> 
> I'm not sure if anything could be salvaged here migration wise

This whole thing is indeed very unfortunate.  For easier reference of
future, here are the list of commits that are relevant in time order:

fb506e701e ("intel_iommu: reject broken EIM", 2016-10-17)
c1bb5418e3 ("target/i386: Support up to 32768 CPUs without IRQ remapping", 2020-12-10)
77250171bd ("intel_iommu: Fix irqchip / X2APIC configuration checks", 2022-05-16)
dc89f32d92 ("target/i386: Fix sanity check on max APIC ID / X2APIC enablement", 2022-05-16)

So regarding compatibility I'm wondering whether we should loose it for
this case, depending on whether vendors (like RH, or QEMU community in
general) should support "allowing 32K vcpu without vIOMMU" as an "official"
feature, or treat it as "experimental only".

IMO it's more important to always make the officially supported bits
compatible and work as expected.  Here IIUC the (only) official way to
support >255 vcpus should still be using vIOMMU with EIM enabled so far.
But I'm happy to be corrected..

If so, I would still suggest having such a patch because it should fix one
of the basic use cases with vIOMMU and currently upstream is broken on it.
This patch will definitely change the behavior again, but the old was
simply broken and we don't really have much choice out of it, IMHO.

Thanks,

> 
> PS:
> I'd keep kvm_enable_x2apic() only in corrected x86_cpus_init()
> and use kvm_has_x2apic_api() elsewhere for checks and bailing out.
> 
> 
> > +            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 */
> 

-- 
Peter Xu

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Igor Mammedov]

On Thu, 22 Sep 2022 12:40:01 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Thu, Sep 22, 2022 at 03:46:17PM +0200, Igor Mammedov wrote:
> > On Wed, 21 Sep 2022 12:12:27 -0400
> > Peter Xu <peterx@redhat.com> wrote:
> >   
> > > It's true that when vcpus<=255 we don't require the length of 32bit APIC
> > > IDs.  However here since we already have EIM=ON it means the hypervisor
> > > will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> > > EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> > > even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
> > > that wants to boot a VM with >=9 but <=255 vcpus with:
> > > 
> > >   -device intel-iommu,intremap=on
> > > 
> > > For anyone who does not want to enable x2apic, we can use eim=off in the
> > > intel-iommu parameters to skip enabling KVM x2apic.
> > > 
> > > This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> > > keeping the valid bit on checking split irqchip, but revert the other change.
> > > 
> > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > Cc: Claudio Fontana <cfontana@suse.de>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 05d53a1aa9..6524c2ee32 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> > >              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
> > >              return false;
> > >          }
> > > +        if (!kvm_enable_x2apic()) {  
> > 
> > above 'check' has side-effects
> > if it's supposed to be a check it would be better to use kvm_has_x2apic_api()
> > instead.  
> 
> A check is not what I wanted.
> 
> As stated in the commit message, since for some reason EIM is enabled on
> the VT-d device already, we need to enable x2apic for the whole guest
> (including KVM) to match with EIM=on being declared even if vcpus<255.
> 
> > 
> > Also 77250171bdc says:
> > "
> >     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.
> > "
> > 
> > Looking for that elsewhere, it looks like commit dc89f32d92b was supposed
> > to take care of removed chunk, but that is not reachable because of > 255 vCPUs"
> > 
> > Likely 77250171bdc just exposed a bug in dc89f32d92b, where
> > the later removed kvm_enable_x2apic() always called (with split irqchip)
> > and made it called only when > 255 vCPUs.
> > 
> > So migration wise it looks like all version with it and less than 255 cpus
> > are broken.
> > 
> > Wait earlier c1bb5418e3, introduced that
> >    kvm_irqchip_is_split() && kvm_enable_x2apic()
> > 'condition', also without any compat machinery to keep old behavior.  
> 
> There's actually some attempt to be compatible, with:
> 
>  GlobalProperty pc_compat_5_1[] = {
>      { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> +    { TYPE_X86_CPU, "kvm-msi-ext-dest-id", "off" },
>  };
> 
> But I don't think that's strictly correct, because afaict that only
> controls whether guest enables it or not (I can only see Linux does it that
> way; no idea whether that's detected to other OSes from the PV interfaces).
> The KVM x2apic will still be enabled even on old machines I think, as you
> mentioned.

yep, that compat affects only kvm-msi-ext-dest-id, the kvm_enable_x2apic()
was still called regardless of that.

> > And before that kvm_enable_x2apic() was affecting only configuration
> > with intel_iommu (fb506e701e9b).  
> 
> Right, afaict that's what we "officially" support.
> 
> > 
> > I'm not sure if anything could be salvaged here migration wise  
> 
> This whole thing is indeed very unfortunate.  For easier reference of
> future, here are the list of commits that are relevant in time order:
> 
> fb506e701e ("intel_iommu: reject broken EIM", 2016-10-17)
> c1bb5418e3 ("target/i386: Support up to 32768 CPUs without IRQ remapping", 2020-12-10)
> 77250171bd ("intel_iommu: Fix irqchip / X2APIC configuration checks", 2022-05-16)
> dc89f32d92 ("target/i386: Fix sanity check on max APIC ID / X2APIC enablement", 2022-05-16)
> 
> So regarding compatibility I'm wondering whether we should loose it for
> this case, depending on whether vendors (like RH, or QEMU community in
> general) should support "allowing 32K vcpu without vIOMMU" as an "official"
> feature, or treat it as "experimental only".

If I recall correctly that's PV only feature that also requires special
tailored guest. i.e. it's possible to deliver IPIs in such configuration
but devices could interact only with a fraction of CPUs (irq-wise) or
something else should take care of IRQ remapping.
I don't think mainstream distributions care about this case much.

> IMO it's more important to always make the officially supported bits
> compatible and work as expected.  Here IIUC the (only) official way to
> support >255 vcpus should still be using vIOMMU with EIM enabled so far.
> But I'm happy to be corrected..
> 
> If so, I would still suggest having such a patch because it should fix one
> of the basic use cases with vIOMMU and currently upstream is broken on it.
> This patch will definitely change the behavior again, but the old was
> simply broken and we don't really have much choice out of it, IMHO.

The thing I worry about is calling kvm_enable_x2apic() from multiple places.
It would be better to fix x86_cpus_init() part of dc89f32d92 and have a check
in IOMMU. But if that's impossible/too ugly, I won't object to this patch.


> Thanks,
> 
> > 
> > PS:
> > I'd keep kvm_enable_x2apic() only in corrected x86_cpus_init()
> > and use kvm_has_x2apic_api() elsewhere for checks and bailing out.
> > 
> >   
> > > +            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 */  
> >   
> 

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Igor Mammedov]

On Fri, 23 Sep 2022 10:20:34 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 22 Sep 2022 12:40:01 -0400
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Thu, Sep 22, 2022 at 03:46:17PM +0200, Igor Mammedov wrote:  
> > > On Wed, 21 Sep 2022 12:12:27 -0400
> > > Peter Xu <peterx@redhat.com> wrote:
> > >     
> > > > It's true that when vcpus<=255 we don't require the length of 32bit APIC
> > > > IDs.  However here since we already have EIM=ON it means the hypervisor
> > > > will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> > > > EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> > > > even if vcpus<=255.  In short, commit 77250171bdc breaks any simple cmdline
> > > > that wants to boot a VM with >=9 but <=255 vcpus with:
> > > > 
> > > >   -device intel-iommu,intremap=on
> > > > 
> > > > For anyone who does not want to enable x2apic, we can use eim=off in the
> > > > intel-iommu parameters to skip enabling KVM x2apic.
> > > > 
> > > > This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> > > > keeping the valid bit on checking split irqchip, but revert the other change.
> > > > 
> > > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > > Cc: Claudio Fontana <cfontana@suse.de>
> > > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  hw/i386/intel_iommu.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index 05d53a1aa9..6524c2ee32 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> > > >              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
> > > >              return false;
> > > >          }
> > > > +        if (!kvm_enable_x2apic()) {    
> > > 
> > > above 'check' has side-effects
> > > if it's supposed to be a check it would be better to use kvm_has_x2apic_api()
> > > instead.    
> > 
> > A check is not what I wanted.
> > 
> > As stated in the commit message, since for some reason EIM is enabled on
> > the VT-d device already, we need to enable x2apic for the whole guest
> > (including KVM) to match with EIM=on being declared even if vcpus<255.
> >   
> > > 
> > > Also 77250171bdc says:
> > > "
> > >     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.
> > > "
> > > 
> > > Looking for that elsewhere, it looks like commit dc89f32d92b was supposed
> > > to take care of removed chunk, but that is not reachable because of > 255 vCPUs"
> > > 
> > > Likely 77250171bdc just exposed a bug in dc89f32d92b, where
> > > the later removed kvm_enable_x2apic() always called (with split irqchip)
> > > and made it called only when > 255 vCPUs.
> > > 
> > > So migration wise it looks like all version with it and less than 255 cpus
> > > are broken.
> > > 
> > > Wait earlier c1bb5418e3, introduced that
> > >    kvm_irqchip_is_split() && kvm_enable_x2apic()
> > > 'condition', also without any compat machinery to keep old behavior.    
> > 
> > There's actually some attempt to be compatible, with:
> > 
> >  GlobalProperty pc_compat_5_1[] = {
> >      { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> > +    { TYPE_X86_CPU, "kvm-msi-ext-dest-id", "off" },
> >  };
> > 
> > But I don't think that's strictly correct, because afaict that only
> > controls whether guest enables it or not (I can only see Linux does it that
> > way; no idea whether that's detected to other OSes from the PV interfaces).
> > The KVM x2apic will still be enabled even on old machines I think, as you
> > mentioned.  
> 
> yep, that compat affects only kvm-msi-ext-dest-id, the kvm_enable_x2apic()
> was still called regardless of that.
> 
> > > And before that kvm_enable_x2apic() was affecting only configuration
> > > with intel_iommu (fb506e701e9b).    
> > 
> > Right, afaict that's what we "officially" support.
> >   
> > > 
> > > I'm not sure if anything could be salvaged here migration wise    
> > 
> > This whole thing is indeed very unfortunate.  For easier reference of
> > future, here are the list of commits that are relevant in time order:
> > 
> > fb506e701e ("intel_iommu: reject broken EIM", 2016-10-17)
> > c1bb5418e3 ("target/i386: Support up to 32768 CPUs without IRQ remapping", 2020-12-10)
> > 77250171bd ("intel_iommu: Fix irqchip / X2APIC configuration checks", 2022-05-16)
> > dc89f32d92 ("target/i386: Fix sanity check on max APIC ID / X2APIC enablement", 2022-05-16)
> > 
> > So regarding compatibility I'm wondering whether we should loose it for
> > this case, depending on whether vendors (like RH, or QEMU community in
> > general) should support "allowing 32K vcpu without vIOMMU" as an "official"
> > feature, or treat it as "experimental only".  
> 
> If I recall correctly that's PV only feature that also requires special
> tailored guest. i.e. it's possible to deliver IPIs in such configuration
> but devices could interact only with a fraction of CPUs (irq-wise) or
> something else should take care of IRQ remapping.
> I don't think mainstream distributions care about this case much.
> 
> > IMO it's more important to always make the officially supported bits
> > compatible and work as expected.  Here IIUC the (only) official way to
> > support >255 vcpus should still be using vIOMMU with EIM enabled so far.
> > But I'm happy to be corrected..
> > 
> > If so, I would still suggest having such a patch because it should fix one
> > of the basic use cases with vIOMMU and currently upstream is broken on it.
> > This patch will definitely change the behavior again, but the old was
> > simply broken and we don't really have much choice out of it, IMHO.  

It's worth putting history excavation with explanation what is broken and why
compat stuff is being ignored in the patch.

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

On Fri, Sep 23, 2022 at 10:41:59AM +0200, Igor Mammedov wrote:
> It's worth putting history excavation with explanation what is broken and why
> compat stuff is being ignored in the patch.

Makes sense, I'll amend the commit message and repost.  Thanks,

-- 
Peter Xu

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

On Fri, Sep 23, 2022 at 06:03:44PM -0400, Peter Xu wrote:
> On Fri, Sep 23, 2022 at 10:41:59AM +0200, Igor Mammedov wrote:
> > It's worth putting history excavation with explanation what is broken and why
> > compat stuff is being ignored in the patch.
> 
> Makes sense, I'll amend the commit message and repost.  Thanks,

There's actually one way to slightly remedy this single case, mostly for
any QEMU 7.1.0 user with -smp <=8 and the intel iommu (as 77250171bdc02 is
merged only in 7.1.0).

We can have one compact parameter x-eim-enable-kvm-x2apic, setting it "on"
by default, "off" for 7.1, and "on" for 7.0-.

I'm not very sure whether that'll worth it.  Any thoughts?

-- 
Peter Xu

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Igor Mammedov]

On Fri, 23 Sep 2022 21:27:08 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Fri, Sep 23, 2022 at 06:03:44PM -0400, Peter Xu wrote:
> > On Fri, Sep 23, 2022 at 10:41:59AM +0200, Igor Mammedov wrote:  
> > > It's worth putting history excavation with explanation what is broken and why
> > > compat stuff is being ignored in the patch.  
> > 
> > Makes sense, I'll amend the commit message and repost.  Thanks,  
> 
> There's actually one way to slightly remedy this single case, mostly for
> any QEMU 7.1.0 user with -smp <=8 and the intel iommu (as 77250171bdc02 is
> merged only in 7.1.0).
> 
> We can have one compact parameter x-eim-enable-kvm-x2apic, setting it "on"
> by default, "off" for 7.1, and "on" for 7.0-.
> 
> I'm not very sure whether that'll worth it.  Any thoughts?

How it (enabling x2apic API) would affect kvm/guests running with 8 or less CPUs
and with intel iommu + remapping?

> 

Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"

[Peter Xu]

On Mon, Sep 26, 2022 at 11:33:11AM +0200, Igor Mammedov wrote:
> On Fri, 23 Sep 2022 21:27:08 -0400
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Fri, Sep 23, 2022 at 06:03:44PM -0400, Peter Xu wrote:
> > > On Fri, Sep 23, 2022 at 10:41:59AM +0200, Igor Mammedov wrote:  
> > > > It's worth putting history excavation with explanation what is broken and why
> > > > compat stuff is being ignored in the patch.  
> > > 
> > > Makes sense, I'll amend the commit message and repost.  Thanks,  
> > 
> > There's actually one way to slightly remedy this single case, mostly for
> > any QEMU 7.1.0 user with -smp <=8 and the intel iommu (as 77250171bdc02 is
> > merged only in 7.1.0).
> > 
> > We can have one compact parameter x-eim-enable-kvm-x2apic, setting it "on"
> > by default, "off" for 7.1, and "on" for 7.0-.
> > 
> > I'm not very sure whether that'll worth it.  Any thoughts?
> 
> How it (enabling x2apic API) would affect kvm/guests running with 8 or less CPUs
> and with intel iommu + remapping?

It's more from the guest ABI level, but indeed at least from what I see so
far <=8 CPUs are fine.

I'll keep the patch short, thanks.

-- 
Peter Xu