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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/11/2023 3:18 pm, Neowutran wrote:
>> 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.
> I was using the qubes-vmm-xen release "4.17.2-5" that use xen version
> "4.17.2" . I don't see custom modification for APIC in the patchs
> applied t 
> o Xen by QubesOS
>
>> 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?
> Once I was logged in, it took me around 10 minutes to type the command
> "sudo dmesg > log"

Ok.  Glacial it is.


>
> There was also graphical instabilities (screen display something, then it is 
> black,
> few seconds later it display things again. 
> Sometime it completly crash and I need to reboot to try to finish the 
> boot+login process),
> and unable to start guests due to the system being too slow. 
>
> Some of the logs gathered from "sudo dmesg" that only appear for case 1 and
> case 3: 
>
> "
>  nvme nvme1: I/O 998 QID 1 timeout, completion polled
>  nvme nvme1: I/O 854 QID 5 timeout, completion polled
>  ...
>  [drm] Fence fallback timer expired on ring sdma0
>  [drm] Fence fallback timer expired on ring sdma0
>  ...
>  [drm] Fence fallback timer expired on ring sdma0
>  [drm] Fence fallback timer ex 
> pired on ring gfx_0.0.0
>  [drm] Fence fallback timer expired on ring gfx_0.0.0
>  [drm] Fence fallback timer expired on ring sdma0
>  ...
> " 
> things like that repeated hundreds of times.

Both the disk and GPU had timeouts.  Both indicative of interrupts not
working correctly.
>> 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.
>>
>> Not that it matters in your case - the bit isn't set in your FADT, hence
>> why case 1 and 3 have the same behaviour.
>>
>>
>> This brings us to case 2, where mixed mode does seem to resolve the per 
> f
>> problem.
>>
>> Since that patch was written, I've learnt how cluster delivery mode
>> works for external interrupts, and Xen should never ever have been using
>> it (Xen appears to be alone in OS software here).  For an external
>> interrupt in Logical cluster mode, it always sends to the lowest ID in
>> the cluster.  If that APIC decides that the local processor is too busy
>> to handle the interrupt now, it forwards the interrupt to the next APIC
>> in the cluster, and this cycle continues until one APIC accepts the message.
>>
>> You get most interrupts hitting the lowest APIC in the cluster, but the
>> interrupt can be forwarded between APICs for an unbounded quantity of
>> time depending on system utilisation.
>>
>>
>> Could you please take case 2 and confirm what happens when booting with
>> x2apic-mode={physical,cluster}?  If the pattern holds, the physical
>> should be fine, and cluster should see the same problems as case 1 and 3.
> I confirm that the pattern holds. "physical" is fine and "cluster"
> have th 
> e same issue as case 1 and case 3.

Ok, so something in your system *really* doesn't like having external
interrupts programmed in Logical cluster mode.

And knowing how logical cluster mode works for external interrupts, we
should never have done that in the first place.

I think this is enough justification to backport mixed mode.  Against a
default upstream Xen, that swaps external interrupts to being programmed
in physical mode, while keeping IPIs in cluster mode.

FWIW, we've already backported mixed mode to XenServer's Xen 4.13 (where
we forced physical mode for other reasons), and that's come with nice
perf improvements from swapping IPIs to using cluster mode.

~Andrew