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

[Igor Mammedov]

On Mon, 26 Sep 2022 11:32:06 -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.
> 
> One thing to mention is that this patch may break migration compatibility
> of such VM, however that's probably the best thing we can do, because the
> old behavior was simply wrong and not working for >8 vcpus.  For <=8 vcpus,
> there could be a light guest ABI change (by enabling KVM x2apic after this
> patch), but logically it shouldn't affect the migration from working.
> 
> Also, this is not the 1st commit to change x2apic behavior.  Igor provided
> a full history of how this evolved for the past few years:
> 
> 20220922154617.57d1a1fb@redhat.com/">https://lore.kernel.org/qemu-devel/20220922154617.57d1a1fb@redhat.com/
> 
> Relevant commits for reference:
> 
>   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)
> 
> We may want to have this for stable too (mostly for 7.1.0 only).  Adding a
> fixes tag.
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Claudio Fontana <cfontana@suse.de>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Fixes: 77250171bd ("intel_iommu: Fix irqchip / X2APIC configuration checks")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v2:
> - Added some more information into commit message [Igor]
> ---
>  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 */