Re: [PATCH] x86/apic: Remove logical destination mode for 64-bit

[Rob <rob@durendal.co.uk>]

* Thomas Gleixner (tglx@linutronix.de) wrote:
>Logical destination mode of the local APIC is used for systems with up to
>8 CPUs. It has an advantage over physical destination mode as it allows to
>target multiple CPUs at once with IPIs.
>
>That advantage was definitely worth it when systems with up to 8 CPUs
>were state of the art for servers and workstations, but that's history.
>
>Aside of that there are systems which fail to work with logical destination
>mode as the ACPI/DMI quirks show and there are AMD Zen1 systems out there
>which fail when interrupt remapping is enabled. The latter can be cured by
>firmware updates, but not all OEMs distribute the required changes.
>
>Physical destination mode is guaranteed to work because it is the only way
>to get a CPU up and running via the INIT/INIT/STARTUP sequence.
>
>As the number of CPUs keeps increasing, logical destination mode becomes a
>less used code path so there is no real good reason to keep it around.
>
>Therefore remove logical destination mode support for 64-bit and default to
>physical destination mode.

Thanks Chris for applying the patch for me.

Thomas - The patched kernel boots successfully.  I held off updating the 
BIOS so there can be no ambiguity.

Thanks,

Rob

>
>Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>---
> arch/x86/include/asm/apic.h         |    8 --
> arch/x86/kernel/apic/apic_flat_64.c |  119 ++----------------------------------
> 2 files changed, 7 insertions(+), 120 deletions(-)
>
>--- a/arch/x86/include/asm/apic.h
>+++ b/arch/x86/include/asm/apic.h
>@@ -345,20 +345,12 @@ extern struct apic *apic;
>  * APIC drivers are probed based on how they are listed in the .apicdrivers
>  * section. So the order is important and enforced by the ordering
>  * of different apic driver files in the Makefile.
>- *
>- * For the files having two apic drivers, we use apic_drivers()
>- * to enforce the order with in them.
>  */
> #define apic_driver(sym)					\
> 	static const struct apic *__apicdrivers_##sym __used		\
> 	__aligned(sizeof(struct apic *))			\
> 	__section(".apicdrivers") = { &sym }
>
>-#define apic_drivers(sym1, sym2)					\
>-	static struct apic *__apicdrivers_##sym1##sym2[2] __used	\
>-	__aligned(sizeof(struct apic *))				\
>-	__section(".apicdrivers") = { &sym1, &sym2 }
>-
> extern struct apic *__apicdrivers[], *__apicdrivers_end[];
>
> /*
>--- a/arch/x86/kernel/apic/apic_flat_64.c
>+++ b/arch/x86/kernel/apic/apic_flat_64.c
>@@ -8,129 +8,25 @@
>  * Martin Bligh, Andi Kleen, James Bottomley, John Stultz, and
>  * James Cleverdon.
>  */
>-#include <linux/cpumask.h>
> #include <linux/export.h>
>-#include <linux/acpi.h>
>
>-#include <asm/jailhouse_para.h>
> #include <asm/apic.h>
>
> #include "local.h"
>
>-static struct apic apic_physflat;
>-static struct apic apic_flat;
>-
>-struct apic *apic __ro_after_init = &apic_flat;
>-EXPORT_SYMBOL_GPL(apic);
>-
>-static int flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>-{
>-	return 1;
>-}
>-
>-static void _flat_send_IPI_mask(unsigned long mask, int vector)
>-{
>-	unsigned long flags;
>-
>-	local_irq_save(flags);
>-	__default_send_IPI_dest_field(mask, vector, APIC_DEST_LOGICAL);
>-	local_irq_restore(flags);
>-}
>-
>-static void flat_send_IPI_mask(const struct cpumask *cpumask, int vector)
>-{
>-	unsigned long mask = cpumask_bits(cpumask)[0];
>-
>-	_flat_send_IPI_mask(mask, vector);
>-}
>-
>-static void
>-flat_send_IPI_mask_allbutself(const struct cpumask *cpumask, int vector)
>-{
>-	unsigned long mask = cpumask_bits(cpumask)[0];
>-	int cpu = smp_processor_id();
>-
>-	if (cpu < BITS_PER_LONG)
>-		__clear_bit(cpu, &mask);
>-
>-	_flat_send_IPI_mask(mask, vector);
>-}
>-
>-static u32 flat_get_apic_id(u32 x)
>+static u32 physflat_get_apic_id(u32 x)
> {
> 	return (x >> 24) & 0xFF;
> }
>
>-static int flat_probe(void)
>+static int physflat_probe(void)
> {
> 	return 1;
> }
>
>-static struct apic apic_flat __ro_after_init = {
>-	.name				= "flat",
>-	.probe				= flat_probe,
>-	.acpi_madt_oem_check		= flat_acpi_madt_oem_check,
>-
>-	.dest_mode_logical		= true,
>-
>-	.disable_esr			= 0,
>-
>-	.init_apic_ldr			= default_init_apic_ldr,
>-	.cpu_present_to_apicid		= default_cpu_present_to_apicid,
>-
>-	.max_apic_id			= 0xFE,
>-	.get_apic_id			= flat_get_apic_id,
>-
>-	.calc_dest_apicid		= apic_flat_calc_apicid,
>-
>-	.send_IPI			= default_send_IPI_single,
>-	.send_IPI_mask			= flat_send_IPI_mask,
>-	.send_IPI_mask_allbutself	= flat_send_IPI_mask_allbutself,
>-	.send_IPI_allbutself		= default_send_IPI_allbutself,
>-	.send_IPI_all			= default_send_IPI_all,
>-	.send_IPI_self			= default_send_IPI_self,
>-	.nmi_to_offline_cpu		= true,
>-
>-	.read				= native_apic_mem_read,
>-	.write				= native_apic_mem_write,
>-	.eoi				= native_apic_mem_eoi,
>-	.icr_read			= native_apic_icr_read,
>-	.icr_write			= native_apic_icr_write,
>-	.wait_icr_idle			= apic_mem_wait_icr_idle,
>-	.safe_wait_icr_idle		= apic_mem_wait_icr_idle_timeout,
>-};
>-
>-/*
>- * Physflat mode is used when there are more than 8 CPUs on a system.
>- * We cannot use logical delivery in this case because the mask
>- * overflows, so use physical mode.
>- */
> static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> {
>-#ifdef CONFIG_ACPI
>-	/*
>-	 * Quirk: some x86_64 machines can only use physical APIC mode
>-	 * regardless of how many processors are present (x86_64 ES7000
>-	 * is an example).
>-	 */
>-	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
>-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
>-		printk(KERN_DEBUG "system APIC only can use physical flat");
>-		return 1;
>-	}
>-
>-	if (!strncmp(oem_id, "IBM", 3) && !strncmp(oem_table_id, "EXA", 3)) {
>-		printk(KERN_DEBUG "IBM Summit detected, will use apic physical");
>-		return 1;
>-	}
>-#endif
>-
>-	return 0;
>-}
>-
>-static int physflat_probe(void)
>-{
>-	return apic == &apic_physflat || num_possible_cpus() > 8 || jailhouse_paravirt();
>+	return 1;
> }
>
> static struct apic apic_physflat __ro_after_init = {
>@@ -146,7 +42,7 @@ static struct apic apic_physflat __ro_af
> 	.cpu_present_to_apicid		= default_cpu_present_to_apicid,
>
> 	.max_apic_id			= 0xFE,
>-	.get_apic_id			= flat_get_apic_id,
>+	.get_apic_id			= physflat_get_apic_id,
>
> 	.calc_dest_apicid		= apic_default_calc_apicid,
>
>@@ -166,8 +62,7 @@ static struct apic apic_physflat __ro_af
> 	.wait_icr_idle			= apic_mem_wait_icr_idle,
> 	.safe_wait_icr_idle		= apic_mem_wait_icr_idle_timeout,
> };
>+apic_driver(apic_physflat);
>
>-/*
>- * We need to check for physflat first, so this order is important.
>- */
>-apic_drivers(apic_physflat, apic_flat);
>+struct apic *apic __ro_after_init = &apic_physflat;
>+EXPORT_SYMBOL_GPL(apic);

-- 
Rob Newcater