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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/11/2023 2:53 pm, Roger Pau Monné wrote:
> 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.

ACPI_FADT_APIC_PHYSICAL is similarly old.

In 2004, xAPIC used strictly in this way distinguished the P4/Xeon APIC
architecture (named xAPIC) from prior generations (simply APIC,
including the original external APIC implementation).

But x2APIC is different still, so properties which were necessary to
state for xAPIC don't necessarily hold for x2APIC.

ACPI_FADT_APIC_CLUSTER is definitely obsolete for x2APIC.  Given the
wording of ACPI_FADT_APIC_PHYSICAL refers to DFR and the 8-APIC
boundary, I'd say its equally obsolete.

~Andrew