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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

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?  As such Xen had been attempting to use cluster mode on an
x2APIC where that mode was broken for physical interrupts?

This may warrant a Fixes line.  This may warrant a backport to 4.17 due
to the seriousness of the bug.


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