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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

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)


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445