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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Mon, Oct 30, 2023 at 04:27:22PM +0100, Roger Pau Monné wrote:
> 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") )
> > > +        x2apic_mode = 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) ||
> > > -                      IS_ENABLED(CONFIG_X2APIC_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 suspicious.

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.


-- 
(\___(\___(\______          --=> 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