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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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).

Thanks, Roger.