Re: [PATCH] x86/x2apic: introduce a mixed physical/cluster mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 27, 2023 at 11:49:03AM +0000, Andrew Cooper wrote:
> On 24/11/2023 7:54 pm, Neowutran wrote:
> > Hi, 
> > I did some more tests and research, indeed this patch improved/solved my 
> > specific case. 
> >
> > Starting point: 
> >
> > I am using Xen version 4.17.2 (exactly this source 
> > https://github.com/QubesOS/qubes-vmm-xen).
> > In the bios (a Asus motherboard), I configured the "local apic" parameter 
> > to "X2APIC".
> > For Xen, I did not set the parameter "x2apic-mode" nor the parameter 
> > "x2apic_phys". 
> >
> > Case 1:
> > I tryied to boot just like that, result: system is unusuably slow
> >
> > Case 2:
> > Then, I applied a backport of the patch  
> > https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger.pau@xxxxxxxxxx/raw
> >  
> > to the original Xen version of QubesOS and I recompiled. 
> > (https://github.com/neowutran/qubes-vmm-xen/blob/x2apic3/X2APIC.patch)
> > Result: it work, the system is usable. 
> >
> > Case 3:
> > Then, I applied the patch 
> > https://github.com/xen-project/xen/commit/26a449ce32cef33f2cb50602be19fcc0c4223ba9
> > to the original Xen version of QubesOS and I recompiled.
> > (https://github.com/neowutran/qubes-vmm-xen/blob/x2apic4/X2APIC.patch)
> > Result: system is  
> > unusuably slow. 
> >
> >
> > In "Case 2", the value returned by the function "apic_x2apic_probe" is 
> > "&apic_x2apic_mixed". 
> > In "Case 3", the value returned by the function "apic_x2apic_probe" is 
> > "&apic_x2apic_cluster". 
> >
> >
> > -------------------
> > If you want / need, details for the function "apic_x2apic_probe":
> >
> > Known "input" value:
> >
> > "CONFIG_X2APIC_PHYSICAL" is not defined
> > "iommu_intremap == iommu_intremap_off" = false
> > "acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL" -> 0
> > "acpi_gbl_FADT.flags" = 247205 (in decimal)
> > "CONFIG_X2APIC_PHYSICAL" is not defined
> > "CONFIG_X2APIC_MIXED" is defined, because it is the default choice
> > "x2apic_mode" = 0
> > "x2apic_phys" = -1
> >
> >
> >
> > Trace log (I did some call "printk" to trace what was going on)
> > Case 2:
> > (XEN) NEOWUTRAN: X2APIC_MODE: 0 
> > (XEN) NEOWUTRAN: X2APIC_PHYS: -1 
> > (XEN) NEOWUTRAN: acpi_gbl_FADT.flags: 247205 
> > (XEN) NEOWUTRAN IOMMU_INTREMAP: different 
> > (XEN) Neowutran: PASSE 2 
> > (XEN) Neowutran: PASSE 4 
> > (XEN) NEOWUTRAN: X2APIC_MODE: 3 
> > (XEN) Neowutran: PASSE 7 
> > (XEN) NEOWUTRAN: X2APIC_MODE: 3 
> >  
> > (XEN) NEOWUTRAN: X2APIC_PHYS: -1 
> > (XEN) NEOWUTRAN: acpi_gbl_FADT.flags: 247205 
> > (XEN) NEOWUTRAN IOMMU_INTREMAP: different 
> >
> > Case 3:
> > (XEN) NEOWUTRAN2: X2APIC_PHYS: -1 
> > (XEN) NEOWUTRAN2: acpi_gbl_FADT.flags: 247205 
> > (XEN) NEOWUTRAN2 IOMMU_INTREMAP: different 
> > (XEN) Neowutran2: Passe 1 
> > (XEN) NEOWUTRAN2: X2APIC_PHYS: 0 
> > (XEN) Neowutran2: Passe 6 
> > (XEN) Neowutran2: Passe 7 
> > (XEN) NEOWUTRAN2: X2APIC_PHYS: 0 
> > (XEN) NEOWUTRAN2: acpi_gbl_FADT.flags: 247205 
> > (XEN) NEOWUTRAN2 IOMMU_INTREMAP: different 
> > (XEN) Neowutran2: Passe 2 
> > (XEN) Neowutran2: Passe 4 
> > (XEN) Neowutran2: Passe 7
> >
> >
> >
> > If you require the full logs, I could publish the full logs somewhere.
> > ----------------------
> >
> > ( However I do not understand if the root issue is a buggy motherboard, a
> > bug in xen, or if the parameter "X2APIC_PHYSICAL" should have been set
> > by the QubesOS project, or something else)
> 
> Hello,
> 
> Thankyou for the analysis.
> 
> For your base version of QubeOS Xen, was that 4.13.2-5 ?   I can't see
> any APIC changes in the patchqueue, and I believe all relevant bugfixes
> are in 4.17.2, but I'd just like to confirm.
> 
> First, by "unusable slow", other than the speed, did everything else
> appear to operate adequately?  Any chance you could guess the slowdown. 
> i.e. was it half the speed, or "seconds per log console line during
> boot" levels of slow?
> 
> 
> Having re-reviewed 26a449ce32, the patch is correct but the reasoning is
> wrong.
> 
> ACPI_FADT_APIC_CLUSTER predates x2APIC by almost a decade (it appeared
> in ACPI 3.0), and is not relevant outside of xAPIC mode.  xAPIC has 2
> different logical destination modes, cluster and flat, and their
> applicability is dependent on whether you have fewer or more than 8
> local APICs, hence that property being called out in the ACPI spec.
> 
> x2APIC does not have this property.  DFR was removed from the
> architecture, and logical mode is strictly cluster.  So the bit should
> never have been interpreted on an x2APIC code path.

FWIW, Jan also pointed out that the ACPI spec mentions xAPIC strictly,
even for ACPI_FADT_APIC_PHYSICAL.  It's possible APIC_PHYSICAL should
also be enforced only in xAPIC mode.  Or it's also possible the ACPI
spec was not updated to mention both xAPIC and x2APIC modes.

Roger.