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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/11/2023 3:04 am, Elliott Mitchell wrote:
> On Fri, Nov 17, 2023 at 11:12:37AM +0100, Neowutran wrote:
>> On 2023-11-07 11:11, Elliott Mitchell wrote:
>>> On Mon, Oct 30, 2023 at 04:27:22PM +01
>>>> On Mon, Oct 30, 2023 at 07:50:27AM -0700, Elliott Mitchell wrote:
>>>>> On Tue, Oct 24, 2023 at 03:51:50PM +0200, Roger Pau Monne wrote:
>>>>>> diff --git a/xen/arch/x86/genapic/x2apic.c 
>>>>>> b/xen/arch/x86/genapic/x2apic.c
>>>>>> index 707deef98c27..15632cc7332e 100644
>>>>>> --- a/xen/arch/x86/genapic/x2apic.c
>>>>>> +++ b/xen/arch/x86/genapic/x2apic.c
>>>>>> @@ -220,38 +239,56 @@ static struct notifier_block x2apic_cpu_nfb = {
>>>>>>  static int8_t __initdata x2apic_phys = -1;
>>>>>>  boolean_param("x2apic_phys", x2apic_phys);
>>>>>>  
>>>>>> +enum {
>>>>>> +   unset, physical, cluster, mixed
>>>>>> +} static __initdata x2apic_mode = unset;
>>>>>> +
>>>>>> +static int __init parse_x2apic_mode(const char *s)
>>>>>> +{
>>>>>> +    if ( !cmdline_strcmp(s, "physical") )
>>>>>> +        x2apic_mode = physical;
>>>>>> +    else if ( !cmdline_strcmp(s, "cluster") )
>>>>>> +        x2apic_mode = cluster;
>>>>>> +    else if ( !cmdline_strcmp(s, "mixed") )
>>>>>> +   
>>>>>> +    else
>>>>>> +        return EINVAL;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +custom_param("x2apic-mode", parse_x2apic_mode);
>>>>>> +
>>>>>>  const struct genapic *__init apic_x2apic_probe(void)
>>>>>>  {
>>>>>> -    if ( x2apic_phys < 0 )
>>>>>> +    /* x2apic-mode option has preference over x2apic_phys. */
>>>>>> +    if ( x2apic_phys >= 0 && x2apic_mode == unset )
>>>>>> +        x2apic_mode = x2apic_phys ? physical : cluster;
>>>>>> +
>>>>>> +    if ( x2apic_mode == unset )
>>>>>>      {
>>>>>> -        /*
>>>>>> -         * Force physical mode if there's no (full) interrupt remapping 
>>>>>> support:
>>>>>> -         * The ID in clustered mode requires a 32 bit destination field 
>>>>>> due to
>>>>>> -         * the usage of the high 16 bits to hold the cluster ID.
>>>>>> -         */
>>>>>> -        x2apic_phys = iommu_intremap != iommu_intremap_full ||
>>>>>> -                      (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) ||
>>>>>> -        
>>>>>> -    }
>>>>>> -    else if ( !x2apic_phys )
>>>>>> -        switch ( iommu_intremap )
>>>>>> +        if ( acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL )
>>>>>>          {
>>>>> Could this explain the issues with recent AMD processors/motherboards?
>>>>>
>>>>> Mainly the firmware had been setting this flag, but Xen was previously
>>>>> ignoring it?
>>>> No, not unless you pass {no-}x2apic_phys={false,0} on the Xen command
>>>> line to force logical (clustered) destination mode.
>>>>
>>>>> As such Xen had been attempting to use cluster mode on an
>>>>> x2APIC where that mode was broken for physical interrupts?
>>>> No, not realy, x2apic_phys was already forced to true if
>>>> acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL is set on the FADT (I
>>>> just delete that line in this same chunk and move it here).
>>> Okay, that was from a quick look at the patch.  Given the symptoms and
>>> workaround with recent AMD motherboards, this looked
>>>
>>> In that case it might be a bug in what AMD is providing to motherboard
>>> manufacturers.  Mainly this bit MUST be set, but AMD's implementation
>>> leaves it unset.
>>>
>>> Could also be if the setup is done correctly the bit can be cleared, but
>>> multiple motherboard manufacturers are breaking this.  Perhaps the steps
>>> are fragile and AMD needed to provide better guidance.
>>>
>>>
>>> Neowutran, are you still setup to and interested in doing
>>> experimentation/testing with Xen on your AMD computer?  Would you be up
>>> for trying the patch here:
>>>
>>> https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger.pau@xxxxxxxxxx/raw
>>>
>>> I have a suspicion this *might* fix the x2APIC issue everyone has been
>>> seeing.
>>>
>>> How plausible would it be to release this as a bugfix/workaround on 4.17?
>>> I'm expecting a "no", but figured I should ask given how widespread the
>>> issue is.
>> I just applied the patch on my setup ( 
>> https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger.pau@xxxxxxxxxx/raw
>>  ) 
>> It seems to fix the x2APIC issue I was having. 
>>
>> I only did some quick tests: 
>>
>> I tried all the differents values in my bios for the X2APIC settings. 
>> Now the system successfully boot in all the cases, without needing
>> manual override of the x2apic_phys/x2apic_mode parameter in boot commandline 
>> .
> In light of this issue effecting a large number of people with recent
> hardware, I formally request the patch
> "x86/x2apic: introduce a mixed physical/cluster mode" be considered for
> backport release on the 4.17 and 4.18 branches.
>
> (I'm unsure whether it is realistic for a 4.17 update, but I figure I
> should ask)

This is an unreasonable ask.

I believe you when you say there is (or at least was) an x2apic bug (or
bugs), but not once did you provide the logging requested, nor engage
usefully with us in debugging.

And despite this, we (Roger, Jan and myself) found, fixed and backported
3 x2apic bugs.

Now you come along guessing alone at x2apic in a patch name that it
fixes your problem, on a patch which is not a bugfix - it's a
performance optimisation.


Neowutran, thankyou for looking into the patch, but I'm afraid that
doesn't confirm that this patch fixed an issue either.  If it really did
make a difference, then you'll see a difference in behaviour using each
of the 3 new x2apic-mode= options.

Please could you take your single up-to-date build of Xen, put the BIOS
settings back to whatever was causing you problems originally, and
describe what happens when booting each of
x2apic-mode={physical,cluster,mixed}?

Thankyou,

~Andrew