Enable support of IOAPIC hotplug on x86 platforms

Now all code in processor_core.c is APIC ID related, so rename it as apic_id.c. Later IOAPIC ID related code will be added into apic_id.c.

Signed-off-by Jiang Liu <[email protected]>

---
 drivers/acpi/Makefile         |    2 +-
 drivers/acpi/apic_id.c        |  202 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/processor_core.c |  205 -----------------------------------------

 include/acpi/processor.h      |    3 -
 include/linux/acpi.h          |    3 +
 5 files changed, 206 insertions(+), 209 deletions(-)
 create mode 100644 drivers/acpi/apic_id.c
 delete mode 100644 drivers/acpi/processor_core.c

drivers/acpi/Makefile +1 -1

index 505d4d79fe3e..03ddd03f2bcd 100644
@@ -35,7 +35,7 @@ acpi-y				+= bus.o glue.o
 acpi-y				+= scan.o
 acpi-y				+= resource.o
 acpi-y				+= acpi_processor.o
-acpi-y				+= processor_core.o
+acpi-y				+= apic_id.o
 acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o

drivers/acpi/apic_id.c new file +202 -0

new file mode 100644
index 000000000000..ada5fd48bad4
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2005 Intel Corporation
+ * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
+ *
+ *	Alex Chiang <[email protected]>
+ *	- Unified x86/ia64 implementations
+ */
+#include <linux/export.h>
+#include <linux/acpi.h>
+#include "internal.h"
+
+static int map_lapic_id(struct acpi_subtable_header *entry,
+		 u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_local_apic *lapic =
+		(struct acpi_madt_local_apic *)entry;
+
+	if (!(lapic->lapic_flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	if (lapic->processor_id != acpi_id)
+		return -EINVAL;
+
+	*apic_id = lapic->id;
+	return 0;
+}
+
+static int map_x2apic_id(struct acpi_subtable_header *entry,
+			 int device_declaration, u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_local_x2apic *apic =
+		(struct acpi_madt_local_x2apic *)entry;
+
+	if (!(apic->lapic_flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	if (device_declaration && (apic->uid == acpi_id)) {
+		*apic_id = apic->local_apic_id;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int map_lsapic_id(struct acpi_subtable_header *entry,
+		int device_declaration, u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_local_sapic *lsapic =
+		(struct acpi_madt_local_sapic *)entry;
+
+	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	if (device_declaration) {
+		if ((entry->length < 16) || (lsapic->uid != acpi_id))
+			return -EINVAL;
+	} else if (lsapic->processor_id != acpi_id)
+		return -EINVAL;
+
+	*apic_id = (lsapic->id << 8) | lsapic->eid;
+	return 0;
+}
+
+static int map_madt_entry(int type, u32 acpi_id)
+{
+	unsigned long madt_end, entry;
+	static struct acpi_table_madt *madt;
+	static int read_madt;
+	int apic_id = -1;
+
+	if (!read_madt) {
+		if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0,
+					(struct acpi_table_header **)&madt)))
+			madt = NULL;
+		read_madt++;
+	}
+
+	if (!madt)
+		return apic_id;
+
+	entry = (unsigned long)madt;
+	madt_end = entry + madt->header.length;
+
+	/* Parse all entries looking for a match. */
+
+	entry += sizeof(struct acpi_table_madt);
+	while (entry + sizeof(struct acpi_subtable_header) < madt_end) {
+		struct acpi_subtable_header *header =
+			(struct acpi_subtable_header *)entry;
+		if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
+			if (!map_lapic_id(header, acpi_id, &apic_id))
+				break;
+		} else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC) {
+			if (!map_x2apic_id(header, type, acpi_id, &apic_id))
+				break;
+		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
+			if (!map_lsapic_id(header, type, acpi_id, &apic_id))
+				break;
+		}
+		entry += header->length;
+	}
+	return apic_id;
+}
+
+static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	struct acpi_subtable_header *header;
+	int apic_id = -1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer)))
+		goto exit;
+
+	if (!buffer.length || !buffer.pointer)
+		goto exit;
+
+	obj = buffer.pointer;
+	if (obj->type != ACPI_TYPE_BUFFER ||
+	    obj->buffer.length < sizeof(struct acpi_subtable_header)) {
+		goto exit;
+	}
+
+	header = (struct acpi_subtable_header *)obj->buffer.pointer;
+	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC)
+		map_lapic_id(header, acpi_id, &apic_id);
+	else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC)
+		map_lsapic_id(header, type, acpi_id, &apic_id);
+	else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC)
+		map_x2apic_id(header, type, acpi_id, &apic_id);
+
+exit:
+	kfree(buffer.pointer);
+	return apic_id;
+}
+
+int acpi_get_apicid(acpi_handle handle, int type, u32 acpi_id)
+{
+	int apic_id;
+
+	apic_id = map_mat_entry(handle, type, acpi_id);
+	if (apic_id == -1)
+		apic_id = map_madt_entry(type, acpi_id);
+
+	return apic_id;
+}
+
+int acpi_map_cpuid(int apic_id, u32 acpi_id)
+{
+#ifdef CONFIG_SMP
+	int i;
+#endif
+
+	if (apic_id == -1) {
+		/*
+		 * On UP processor, there is no _MAT or MADT table.
+		 * So above apic_id is always set to -1.
+		 *
+		 * BIOS may define multiple CPU handles even for UP processor.
+		 * For example,
+		 *
+		 * Scope (_PR)
+		 * {
+		 *     Processor (CPU0, 0x00, 0x00000410, 0x06) {}
+		 *     Processor (CPU1, 0x01, 0x00000410, 0x06) {}
+		 *     Processor (CPU2, 0x02, 0x00000410, 0x06) {}
+		 *     Processor (CPU3, 0x03, 0x00000410, 0x06) {}
+		 * }
+		 *
+		 * Ignores apic_id and always returns 0 for the processor
+		 * handle with acpi id 0 if nr_cpu_ids is 1.
+		 * This should be the case if SMP tables are not found.
+		 * Return -1 for other CPU's handle.
+		 */
+		if (nr_cpu_ids <= 1 && acpi_id == 0)
+			return acpi_id;
+		else
+			return apic_id;
+	}
+
+#ifdef CONFIG_SMP
+	for_each_possible_cpu(i) {
+		if (cpu_physical_id(i) == apic_id)
+			return i;
+	}
+#else
+	/* In UP kernel, only processor 0 is valid */
+	if (apic_id == 0)
+		return apic_id;
+#endif
+	return -1;
+}
+
+int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
+{
+	int apic_id;
+
+	apic_id = acpi_get_apicid(handle, type, acpi_id);
+
+	return acpi_map_cpuid(apic_id, acpi_id);
+}
+EXPORT_SYMBOL_GPL(acpi_get_cpuid);

drivers/acpi/processor_core.c deleted +0 -205

deleted file mode 100644
index b048f3752c2b..000000000000
@@ -1,205 +0,0 @@
-/*
- * Copyright (C) 2005 Intel Corporation
- * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
- *
- *	Alex Chiang <[email protected]>
- *	- Unified x86/ia64 implementations
- */
-#include <linux/export.h>
-#include <linux/acpi.h>
-#include <acpi/processor.h>
-
-#define _COMPONENT		ACPI_PROCESSOR_COMPONENT
-ACPI_MODULE_NAME("processor_core");
-
-static int map_lapic_id(struct acpi_subtable_header *entry,
-		 u32 acpi_id, int *apic_id)
-{
-	struct acpi_madt_local_apic *lapic =
-		(struct acpi_madt_local_apic *)entry;
-
-	if (!(lapic->lapic_flags & ACPI_MADT_ENABLED))
-		return -ENODEV;
-
-	if (lapic->processor_id != acpi_id)
-		return -EINVAL;
-
-	*apic_id = lapic->id;
-	return 0;
-}
-
-static int map_x2apic_id(struct acpi_subtable_header *entry,
-			 int device_declaration, u32 acpi_id, int *apic_id)
-{
-	struct acpi_madt_local_x2apic *apic =
-		(struct acpi_madt_local_x2apic *)entry;
-
-	if (!(apic->lapic_flags & ACPI_MADT_ENABLED))
-		return -ENODEV;
-
-	if (device_declaration && (apic->uid == acpi_id)) {
-		*apic_id = apic->local_apic_id;
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
-static int map_lsapic_id(struct acpi_subtable_header *entry,
-		int device_declaration, u32 acpi_id, int *apic_id)
-{
-	struct acpi_madt_local_sapic *lsapic =
-		(struct acpi_madt_local_sapic *)entry;
-
-	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
-		return -ENODEV;
-
-	if (device_declaration) {
-		if ((entry->length < 16) || (lsapic->uid != acpi_id))
-			return -EINVAL;
-	} else if (lsapic->processor_id != acpi_id)
-		return -EINVAL;
-
-	*apic_id = (lsapic->id << 8) | lsapic->eid;
-	return 0;
-}
-
-static int map_madt_entry(int type, u32 acpi_id)
-{
-	unsigned long madt_end, entry;
-	static struct acpi_table_madt *madt;
-	static int read_madt;
-	int apic_id = -1;
-
-	if (!read_madt) {
-		if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0,
-					(struct acpi_table_header **)&madt)))
-			madt = NULL;
-		read_madt++;
-	}
-
-	if (!madt)
-		return apic_id;
-
-	entry = (unsigned long)madt;
-	madt_end = entry + madt->header.length;
-
-	/* Parse all entries looking for a match. */
-
-	entry += sizeof(struct acpi_table_madt);
-	while (entry + sizeof(struct acpi_subtable_header) < madt_end) {
-		struct acpi_subtable_header *header =
-			(struct acpi_subtable_header *)entry;
-		if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
-			if (!map_lapic_id(header, acpi_id, &apic_id))
-				break;
-		} else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC) {
-			if (!map_x2apic_id(header, type, acpi_id, &apic_id))
-				break;
-		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-			if (!map_lsapic_id(header, type, acpi_id, &apic_id))
-				break;
-		}
-		entry += header->length;
-	}
-	return apic_id;
-}
-
-static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
-{
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-	struct acpi_subtable_header *header;
-	int apic_id = -1;
-
-	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer)))
-		goto exit;
-
-	if (!buffer.length || !buffer.pointer)
-		goto exit;
-
-	obj = buffer.pointer;
-	if (obj->type != ACPI_TYPE_BUFFER ||
-	    obj->buffer.length < sizeof(struct acpi_subtable_header)) {
-		goto exit;
-	}
-
-	header = (struct acpi_subtable_header *)obj->buffer.pointer;
-	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC)
-		map_lapic_id(header, acpi_id, &apic_id);
-	else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC)
-		map_lsapic_id(header, type, acpi_id, &apic_id);
-	else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC)
-		map_x2apic_id(header, type, acpi_id, &apic_id);
-
-exit:
-	kfree(buffer.pointer);
-	return apic_id;
-}
-
-int acpi_get_apicid(acpi_handle handle, int type, u32 acpi_id)
-{
-	int apic_id;
-
-	apic_id = map_mat_entry(handle, type, acpi_id);
-	if (apic_id == -1)
-		apic_id = map_madt_entry(type, acpi_id);
-
-	return apic_id;
-}
-
-int acpi_map_cpuid(int apic_id, u32 acpi_id)
-{
-#ifdef CONFIG_SMP
-	int i;
-#endif
-
-	if (apic_id == -1) {
-		/*
-		 * On UP processor, there is no _MAT or MADT table.
-		 * So above apic_id is always set to -1.
-		 *
-		 * BIOS may define multiple CPU handles even for UP processor.
-		 * For example,
-		 *
-		 * Scope (_PR)
-		 * {
-		 *     Processor (CPU0, 0x00, 0x00000410, 0x06) {}
-		 *     Processor (CPU1, 0x01, 0x00000410, 0x06) {}
-		 *     Processor (CPU2, 0x02, 0x00000410, 0x06) {}
-		 *     Processor (CPU3, 0x03, 0x00000410, 0x06) {}
-		 * }
-		 *
-		 * Ignores apic_id and always returns 0 for the processor
-		 * handle with acpi id 0 if nr_cpu_ids is 1.
-		 * This should be the case if SMP tables are not found.
-		 * Return -1 for other CPU's handle.
-		 */
-		if (nr_cpu_ids <= 1 && acpi_id == 0)
-			return acpi_id;
-		else
-			return apic_id;
-	}
-
-#ifdef CONFIG_SMP
-	for_each_possible_cpu(i) {
-		if (cpu_physical_id(i) == apic_id)
-			return i;
-	}
-#else
-	/* In UP kernel, only processor 0 is valid */
-	if (apic_id == 0)
-		return apic_id;
-#endif
-	return -1;
-}
-
-int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
-{
-	int apic_id;
-
-	apic_id = acpi_get_apicid(handle, type, acpi_id);
-
-	return acpi_map_cpuid(apic_id, acpi_id);
-}
-EXPORT_SYMBOL_GPL(acpi_get_cpuid);

include/acpi/processor.h +0 -3

index 9b9b6f29bbf3..99fc22c9e61f 100644
@@ -314,9 +314,6 @@ static inline int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
 
 /* in processor_core.c */
 void acpi_processor_set_pdc(acpi_handle handle);
-int acpi_get_apicid(acpi_handle, int type, u32 acpi_id);
-int acpi_map_cpuid(int apic_id, u32 acpi_id);
-int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id);
 
 /* in processor_throttling.c */
 int acpi_processor_tstate_has_changed(struct acpi_processor *pr);

include/linux/acpi.h +3 -0

index 807cbc46d73e..05ed6886f1f8 100644
@@ -140,6 +140,9 @@ void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa);
 int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
 void acpi_numa_arch_fixup(void);
 
+int acpi_get_apicid(acpi_handle, int type, u32 acpi_id);
+int acpi_map_cpuid(int apic_id, u32 acpi_id);
+int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id);
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 /* Arch dependent functions for cpu hotplug support */
 int acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu);

Quoted 2 lines · Jiang Liu

Now all code in processor_core.c is APIC ID related, so rename it as apic_id.c. Later IOAPIC ID related code will be added into apic_id.c.

Actually, I'm not sure about this one.

Renames like this make it difficult to backport things in general and kind of break "git blame", so do we have to do that?

What's wrong with leaving the name as is and adding a comment about the contents being related to IOAPIC ID?

Hi Gerry, Rafael,

Quoted 8 lines · Rafael J. Wysocki

Quoted 2 lines · Jiang Liu

Now all code in processor_core.c is APIC ID related, so rename it as apic_id.c. Later IOAPIC ID related code will be added into apic_id.c.

Actually, I'm not sure about this one.

Renames like this make it difficult to backport things in general and kind of break "git blame", so do we have to do that?

Quoted 2 lines

What's wrong with leaving the name as is and adding a comment about the contents being related to IOAPIC ID?

It will be thankful for not renaming the file into apic_id.c, because apic id is x86 specific, and platform like ARM will also add some code to get cpu hardware id via GICC structure in MADT table in that file, apic id is not a generic name for both x86 and ARM, I prefer to keep it as it is :)

Quoted 11 lines · Hanjun Guo

Hi Gerry, Rafael,

Quoted 8 lines · Rafael J. Wysocki

Quoted 2 lines · Jiang Liu

Now all code in processor_core.c is APIC ID related, so rename it as apic_id.c. Later IOAPIC ID related code will be added into apic_id.c.

Actually, I'm not sure about this one.

Renames like this make it difficult to backport things in general and kind of break "git blame", so do we have to do that?

Quoted 7 lines

Quoted 2 lines

What's wrong with leaving the name as is and adding a comment about the contents being related to IOAPIC ID?

It will be thankful for not renaming the file into apic_id.c, because apic id is x86 specific, and platform like ARM will also add some code to get cpu hardware id via GICC structure in MADT table in that file, apic id is not a generic name for both x86 and ARM, I prefer to keep it as it is :)

Well, that's a good argument too.

Rafael

static int map_lapic_id(struct acpi_subtable_header *entry,

drivers/acpi/apic_id.c +65 -2

 		 u32 acpi_id, int *apic_id)
 {

@@ -64,8 +71,6 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
 static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
-	static struct acpi_table_madt *madt;
-	static int read_madt;
 	int apic_id = -1;
 
 	if (!read_madt) {

@@ -200,3 +205,90 @@ int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
 	return acpi_map_cpuid(apic_id, acpi_id);
 }
 EXPORT_SYMBOL_GPL(acpi_get_cpuid);
+
+#ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
+static int map_ioapic_id(struct acpi_subtable_header *entry, u32 gsi_base,
+			 u64 *phys_addr, int *ioapic_id)
+{
+	struct acpi_madt_io_apic *ioapic = (struct acpi_madt_io_apic *)entry;
+
+	if (ioapic->global_irq_base != gsi_base)
+		return 0;
+
+	*phys_addr = ioapic->address;
+	*ioapic_id = ioapic->id;
+	return 1;
+}
+
+static int map_madt_ioapic_entry(u32 gsi_base, u64 *phys_addr)
+{
+	struct acpi_subtable_header *hdr;
+	unsigned long madt_end, entry;
+	int apic_id = -1;
+
+	if (!read_madt) {
+		if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0,
+					(struct acpi_table_header **)&madt)))
+			madt = NULL;
+		read_madt++;
+	}
+
+	if (!madt)
+		return apic_id;
+
+	entry = (unsigned long)madt;
+	madt_end = entry + madt->header.length;
+
+	/* Parse all entries looking for a match. */
+	entry += sizeof(struct acpi_table_madt);
+	while (entry + sizeof(struct acpi_subtable_header) < madt_end) {
+		hdr = (struct acpi_subtable_header *)entry;
+		if (hdr->type == ACPI_MADT_TYPE_IO_APIC &&
+		    map_ioapic_id(hdr, gsi_base, phys_addr, &apic_id))
+			break;
+		else
+			entry += hdr->length;
+	}
+
+	return apic_id;
+}
+
+static int map_mat_ioapic_entry(acpi_handle handle, u32 gsi_base,
+				u64 *phys_addr)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	struct acpi_subtable_header *header;
+	int apic_id = -1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer)))
+		goto exit;
+
+	if (!buffer.length || !buffer.pointer)
+		goto exit;
+
+	obj = buffer.pointer;
+	if (obj->type != ACPI_TYPE_BUFFER ||
+	    obj->buffer.length < sizeof(struct acpi_subtable_header))

drivers/acpi/apic_id.c line 17

Quoted 3 lines · Jiang Liu

drivers/acpi/apic_id.c +2 -0

+static struct acpi_table_madt *madt;
+static int read_madt;

Pretty lousy file visible variable names.

So we end up with two copies of the butt ugly

   if (!read_madt) {
      .....
   }

stuff instead of creating a helper function which hides that and do

      madr = get_madt();
      if (!madt)
		return ...;

at the call sites.

Quoted 88 lines

static int map_lapic_id(struct acpi_subtable_header *entry,

drivers/acpi/apic_id.c +71 -2

 		 u32 acpi_id, int *apic_id)
 {

@@ -64,8 +71,6 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
 static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
-	static struct acpi_table_madt *madt;
-	static int read_madt;
 	int apic_id = -1;
 
 	if (!read_madt) {

@@ -200,3 +205,90 @@ int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
 	return acpi_map_cpuid(apic_id, acpi_id);
 }
 EXPORT_SYMBOL_GPL(acpi_get_cpuid);
+
+#ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
+static int map_ioapic_id(struct acpi_subtable_header *entry, u32 gsi_base,
+			 u64 *phys_addr, int *ioapic_id)
+{
+	struct acpi_madt_io_apic *ioapic = (struct acpi_madt_io_apic *)entry;
+
+	if (ioapic->global_irq_base != gsi_base)
+		return 0;
+
+	*phys_addr = ioapic->address;
+	*ioapic_id = ioapic->id;
+	return 1;
+}
+
+static int map_madt_ioapic_entry(u32 gsi_base, u64 *phys_addr)
+{
+	struct acpi_subtable_header *hdr;
+	unsigned long madt_end, entry;
+	int apic_id = -1;
+
+	if (!read_madt) {
+		if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0,
+					(struct acpi_table_header **)&madt)))
+			madt = NULL;
+		read_madt++;
+	}
+
+	if (!madt)
+		return apic_id;
+
+	entry = (unsigned long)madt;
+	madt_end = entry + madt->header.length;
+
+	/* Parse all entries looking for a match. */
+	entry += sizeof(struct acpi_table_madt);
+	while (entry + sizeof(struct acpi_subtable_header) < madt_end) {
+		hdr = (struct acpi_subtable_header *)entry;
+		if (hdr->type == ACPI_MADT_TYPE_IO_APIC &&
+		    map_ioapic_id(hdr, gsi_base, phys_addr, &apic_id))
+			break;
+		else
+			entry += hdr->length;
+	}
+
+	return apic_id;
+}
+
+static int map_mat_ioapic_entry(acpi_handle handle, u32 gsi_base,
+				u64 *phys_addr)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	struct acpi_subtable_header *header;
+	int apic_id = -1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer)))
+		goto exit;
+
+	if (!buffer.length || !buffer.pointer)
+		goto exit;
+
+	obj = buffer.pointer;
+	if (obj->type != ACPI_TYPE_BUFFER ||
+	    obj->buffer.length < sizeof(struct acpi_subtable_header))
+		goto exit;
+
+	header = (struct acpi_subtable_header *)obj->buffer.pointer;
+	if (header->type == ACPI_MADT_TYPE_IO_APIC)
+		map_ioapic_id(header, gsi_base, phys_addr, &apic_id);
+

drivers/acpi/apic_id.c int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id) · line 282

Quoted 4 lines

drivers/acpi/apic_id.c +4 -0

+exit:
+	kfree(buffer.pointer);
+	return apic_id;
+}

This stuff really wants proper documentation. The lack of comments is just annoying.

drivers/acpi/apic_id.c int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id) · line 293

Quoted 11 lines

drivers/acpi/apic_id.c +10 -0

+int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr)
+{
+	int apic_id;
+
+	apic_id = map_mat_ioapic_entry(handle, gsi_base, phys_addr);
+	if (apic_id == -1)
+		apic_id = map_madt_ioapic_entry(gsi_base, phys_addr);
+
+	return apic_id;
+}

Thanks,

tglx

From: Yinghai Lu <[email protected]>

Perfer the assigned ID in APIC ID register for x86_64 if it's still available.

Signed-off-by Yinghai Lu <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |   38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

arch/x86/kernel/apic/io_apic.c +0 -0

index 3faf9599ff29..196d9c15fdec 100644

arch/x86/kernel/apic/io_apic.c +19 -3

 {
 	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
 	    !APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
-		return io_apic_get_unique_id(nr_ioapics, id);
+		return io_apic_get_unique_id(idx, id);
 	else
 		return id;
 }
 #else
-static u8 __init io_apic_unique_id(u8 id)
+static u8 io_apic_unique_id(int idx, u8 id)
 {
 	int i;
+	u8 new_id;
+	unsigned long flags;
 	DECLARE_BITMAP(used, 256);
+	union IO_APIC_reg_00 reg_00;
 
 	bitmap_zero(used, 256);
 	for_each_ioapic(i)
 		__set_bit(mpc_ioapic_id(i), used);
 	if (!test_bit(id, used))
 		return id;
-	return find_first_zero_bit(used, 256);
+
+	/* check register at first */
+	raw_spin_lock_irqsave(&ioapic_lock, flags);
+	reg_00.raw = io_apic_read(idx, 0);
+	raw_spin_unlock_irqrestore(&ioapic_lock, flags);
+	new_id = reg_00.bits.ID;
+	if (!test_bit(new_id, used)) {
+		apic_printk(APIC_VERBOSE, KERN_INFO
+			"IOAPIC[%d]: Using reg apic_id %d instead of %d\n",
+			 idx, new_id, id);
+		return new_id;
+	}
+
+	new_id = find_first_zero_bit(used, 256);

Quoted 22 lines · Jiang Liu

From: Yinghai Lu <[email protected]>

Perfer the assigned ID in APIC ID register for x86_64 if it's still available.

Signed-off-by Yinghai Lu <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |   38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

arch/x86/kernel/apic/io_apic.c +0 -0

index 3faf9599ff29..196d9c15fdec 100644
@@ -3575,26 +3575,54 @@ static int __init io_apic_get_unique_id(int ioapic, int apic_id)
 	return apic_id;
 }
 

arch/x86/kernel/apic/io_apic.c static int __init io_apic_get_unique_id(int ioapic, int apic_id) · line 3578

Quoted 2 lines

arch/x86/kernel/apic/io_apic.c +1 -1

-static u8 __init io_apic_unique_id(u8 id)
+static u8 io_apic_unique_id(int idx, u8 id)

How is that change related to the changelog?

Quoted 44 lines

arch/x86/kernel/apic/io_apic.c +24 -3

 {
 	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
 	    !APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
-		return io_apic_get_unique_id(nr_ioapics, id);
+		return io_apic_get_unique_id(idx, id);
 	else
 		return id;
 }
 #else
-static u8 __init io_apic_unique_id(u8 id)
+static u8 io_apic_unique_id(int idx, u8 id)
 {
 	int i;
+	u8 new_id;
+	unsigned long flags;
 	DECLARE_BITMAP(used, 256);
+	union IO_APIC_reg_00 reg_00;
 
 	bitmap_zero(used, 256);
 	for_each_ioapic(i)
 		__set_bit(mpc_ioapic_id(i), used);
 	if (!test_bit(id, used))
 		return id;
-	return find_first_zero_bit(used, 256);
+
+	/* check register at first */
+	raw_spin_lock_irqsave(&ioapic_lock, flags);
+	reg_00.raw = io_apic_read(idx, 0);
+	raw_spin_unlock_irqrestore(&ioapic_lock, flags);
+	new_id = reg_00.bits.ID;
+	if (!test_bit(new_id, used)) {
+		apic_printk(APIC_VERBOSE, KERN_INFO
+			"IOAPIC[%d]: Using reg apic_id %d instead of %d\n",
+			 idx, new_id, id);
+		return new_id;
+	}
+
+	new_id = find_first_zero_bit(used, 256);
+	reg_00.bits.ID = new_id;
+	raw_spin_lock_irqsave(&ioapic_lock, flags);
+	io_apic_write(idx, 0, reg_00.raw);
+	reg_00.raw = io_apic_read(idx, 0);
+	raw_spin_unlock_irqrestore(&ioapic_lock, flags);

arch/x86/kernel/apic/io_apic.c static int __init io_apic_get_unique_id(int ioapic, int apic_id) · line 3623

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +5 -0

+
+	/* Sanity check */
+	if (reg_00.bits.ID != new_id)
+		pr_warn("IOAPIC[%d]: Unable to change apic_id to %d!\n",
+			idx, new_id);

So we detect, that the ID could not be changed and we return it nevertheless?

Thanks,

tglx

EXPORT_SYMBOL(acpi_register_ioapic);

arch/x86/kernel/apic/io_apic.c +7 -2

index 6e67af0c5f99..b286461cabf9 100644
@@ -3851,7 +3851,13 @@ static int bad_ioapic_register(int idx)
 
 static int find_free_ioapic_entry(void)
 {
-	return nr_ioapics;
+	int idx;
+
+	for (idx = 0; idx < MAX_IO_APICS; idx++)
+		if (ioapics[idx].nr_registers == 0)
+			return idx;
+
+	return MAX_IO_APICS;
 }
 
 int mp_register_ioapic(int id, u32 address, u32 gsi_base,

@@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	}
 	for_each_ioapic(ioapic)
 		if (ioapics[ioapic].mp_config.apicaddr == address) {
-			pr_warn("address 0x%x conflicts with IOAPIC%d\n",

Quoted 31 lines · Jiang Liu

EXPORT_SYMBOL(acpi_register_ioapic);

arch/x86/kernel/apic/io_apic.c +12 -3

index 6e67af0c5f99..b286461cabf9 100644
@@ -3851,7 +3851,13 @@ static int bad_ioapic_register(int idx)
 
 static int find_free_ioapic_entry(void)
 {
-	return nr_ioapics;
+	int idx;
+
+	for (idx = 0; idx < MAX_IO_APICS; idx++)
+		if (ioapics[idx].nr_registers == 0)
+			return idx;
+
+	return MAX_IO_APICS;
 }
 
 int mp_register_ioapic(int id, u32 address, u32 gsi_base,

@@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	}
 	for_each_ioapic(ioapic)
 		if (ioapics[ioapic].mp_config.apicaddr == address) {
-			pr_warn("address 0x%x conflicts with IOAPIC%d\n",
-				address, ioapic);
+			/*
+			 * IOAPIC unit may also be visible in PCI scope.
+			 * When ioapic PCI driver's probe() is called,
+			 * the IOAPIC unit may have already been initialized
+			 * at boot time.

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3884

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +4 -0

+			 */
+			if (!ioapic_initialized)
+				pr_warn("address 0x%x conflicts with IOAPIC%d\n",
+					address, ioapic);

Hmm. This smells fishy. Why do we allow multiple initializations of the same IOAPIC in the first place. Either it's done via ACPI or via PCI, but not both.

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +0 -0

 			return -EEXIST;
 		}
 

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3934

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +1 -0

@@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	ioapics[idx].irqdomain = NULL;
 	ioapics[idx].irqdomain_cfg = *cfg;
 
+	if (ioapic_initialized) {

I have a hard time to understand this conditional. Why can't we do that unconditionally?

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3944

Quoted 11 lines

arch/x86/kernel/apic/io_apic.c +7 -0

+		if (mp_irqdomain_create(idx)) {
+			clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
+			return -ENOMEM;
+		}
+		alloc_ioapic_saved_registers(idx);
+	}
+
 	if (gsi_cfg->gsi_end >= gsi_top)
 		gsi_top = gsi_cfg->gsi_end + 1;
 	if (nr_ioapics <= idx)

Thanks,

tglx

Quoted 12 lines

Quoted 12 lines

int mp_register_ioapic(int id, u32 address, u32 gsi_base,

arch/x86/kernel/apic/io_apic.c +5 -2

@@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	}
 	for_each_ioapic(ioapic)
 		if (ioapics[ioapic].mp_config.apicaddr == address) {
-			pr_warn("address 0x%x conflicts with IOAPIC%d\n",
-				address, ioapic);
+			/*
+			 * IOAPIC unit may also be visible in PCI scope.
+			 * When ioapic PCI driver's probe() is called,
+			 * the IOAPIC unit may have already been initialized
+			 * at boot time.

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3884

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +4 -0

+			 */
+			if (!ioapic_initialized)
+				pr_warn("address 0x%x conflicts with IOAPIC%d\n",
+					address, ioapic);

Hmm. This smells fishy. Why do we allow multiple initializations of the same IOAPIC in the first place. Either it's done via ACPI or via PCI, but not both.

The ACPI subsystem will register and initialize all IOAPICs when walking ACPI MADT table during boot, before initializing PCI subsystem. Later when binding ioapic PCI driver to IOAPIC PCI device, it will try to register the IOAPIC device again.

After this patchset is applied, we could remove the !ioapic_initialized check. We check acpi_ioapic_register() before calling acpi_register_ioapic(). So the check becomes redundant.
Or we could remove the temporary code from this patch.

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +0 -0

 			return -EEXIST;
 		}
 

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3934

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +1 -0

@@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	ioapics[idx].irqdomain = NULL;
 	ioapics[idx].irqdomain_cfg = *cfg;
 
+	if (ioapic_initialized) {

I have a hard time to understand this conditional. Why can't we do that unconditionally?

How about following comments?
/*
* If mp_register_ioapic() is called during early boot stage when
* walking ACPI/SFI/DT tables, it's too early to create irqdomain,
* we are still using bootmem allocator. So delay it to setup_IO_APIC(). */

Quoted 25 lines · Jiang Liu

Quoted 12 lines

Quoted 12 lines

int mp_register_ioapic(int id, u32 address, u32 gsi_base,

arch/x86/kernel/apic/io_apic.c +5 -2

@@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	}
 	for_each_ioapic(ioapic)
 		if (ioapics[ioapic].mp_config.apicaddr == address) {
-			pr_warn("address 0x%x conflicts with IOAPIC%d\n",
-				address, ioapic);
+			/*
+			 * IOAPIC unit may also be visible in PCI scope.
+			 * When ioapic PCI driver's probe() is called,
+			 * the IOAPIC unit may have already been initialized
+			 * at boot time.

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3884

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +4 -0

+			 */
+			if (!ioapic_initialized)
+				pr_warn("address 0x%x conflicts with IOAPIC%d\n",
+					address, ioapic);

Hmm. This smells fishy. Why do we allow multiple initializations of the same IOAPIC in the first place. Either it's done via ACPI or via PCI, but not both.

The ACPI subsystem will register and initialize all IOAPICs when walking ACPI MADT table during boot, before initializing PCI subsystem. Later when binding ioapic PCI driver to IOAPIC PCI device, it will try to register the IOAPIC device again.

Quoted 4 lines

After this patchset is applied, we could remove the !ioapic_initialized check. We check acpi_ioapic_register() before calling acpi_register_ioapic(). So the check becomes redundant.
Or we could remove the temporary code from this patch.

How about removing the disfunctional ioapic PCI driver first and then implementing the whole thing cleanly?

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +0 -0

 			return -EEXIST;
 		}
 

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3934

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +1 -0

@@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	ioapics[idx].irqdomain = NULL;
 	ioapics[idx].irqdomain_cfg = *cfg;
 
+	if (ioapic_initialized) {

I have a hard time to understand this conditional. Why can't we do that unconditionally?

Quoted 6 lines

How about following comments?
/*
* If mp_register_ioapic() is called during early boot stage when
* walking ACPI/SFI/DT tables, it's too early to create irqdomain,
* we are still using bootmem allocator. So delay it to setup_IO_APIC(). */

Fine, but then the "if (ioapic_initialized)" conditional still does not make sense. We surely have some global non ioapic specific indicator that bootmem is done and the proper memory allocator is available, right?

Aside of that is there a point to walk those tables before we actually can make any use of their content?

Thanks,

tglx

Quoted 25 lines · Jiang Liu

Quoted 12 lines

Quoted 12 lines

int mp_register_ioapic(int id, u32 address, u32 gsi_base,

arch/x86/kernel/apic/io_apic.c +5 -2

@@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	}
 	for_each_ioapic(ioapic)
 		if (ioapics[ioapic].mp_config.apicaddr == address) {
-			pr_warn("address 0x%x conflicts with IOAPIC%d\n",
-				address, ioapic);
+			/*
+			 * IOAPIC unit may also be visible in PCI scope.
+			 * When ioapic PCI driver's probe() is called,
+			 * the IOAPIC unit may have already been initialized
+			 * at boot time.

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3884

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +4 -0

+			 */
+			if (!ioapic_initialized)
+				pr_warn("address 0x%x conflicts with IOAPIC%d\n",
+					address, ioapic);

Hmm. This smells fishy. Why do we allow multiple initializations of the same IOAPIC in the first place. Either it's done via ACPI or via PCI, but not both.

The ACPI subsystem will register and initialize all IOAPICs when walking ACPI MADT table during boot, before initializing PCI subsystem. Later when binding ioapic PCI driver to IOAPIC PCI device, it will try to register the IOAPIC device again.

Quoted 4 lines

After this patchset is applied, we could remove the !ioapic_initialized check. We check acpi_ioapic_register() before calling acpi_register_ioapic(). So the check becomes redundant.
Or we could remove the temporary code from this patch.

How about removing the disfunctional ioapic PCI driver first and then implementing the whole thing cleanly?

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +0 -0

 			return -EEXIST;
 		}
 

Quoted 8 lines

arch/x86/kernel/apic/io_apic.c int mp_register_ioapic(int id, u32 address, u32 gsi_base, · line 3934

Quoted 5 lines

arch/x86/kernel/apic/io_apic.c +1 -0

@@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	ioapics[idx].irqdomain = NULL;
 	ioapics[idx].irqdomain_cfg = *cfg;
 
+	if (ioapic_initialized) {

I have a hard time to understand this conditional. Why can't we do that unconditionally?

Quoted 6 lines

How about following comments?
/*
* If mp_register_ioapic() is called during early boot stage when
* walking ACPI/SFI/DT tables, it's too early to create irqdomain,
* we are still using bootmem allocator. So delay it to setup_IO_APIC(). */

Fine, but then the "if (ioapic_initialized)" conditional still does not make sense. We surely have some global non ioapic specific indicator that bootmem is done and the proper memory allocator is available, right?

Aside of that is there a point to walk those tables before we actually can make any use of their content?

Thanks,

tglx

Introduce acpi_ioapic_registered() to check whether an IOAPIC has already been registered, it will be used when enabling IOAPIC hotplug.

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/include/asm/io_apic.h |    1 +
 arch/x86/kernel/acpi/boot.c    |   11 +++++++++++
 arch/x86/kernel/apic/io_apic.c |   11 +++++++++++
 include/linux/acpi.h           |    1 +
 4 files changed, 24 insertions(+)

arch/x86/include/asm/io_apic.h +1 -0

index ce63cf34c93c..0db2b7037e4b 100644
@@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq);
 extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 			      struct ioapic_domain_cfg *cfg);
 extern int mp_unregister_ioapic(u32 gsi_base);
+extern int mp_ioapic_registered(u32 gsi_base);
 extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 			    irq_hw_number_t hwirq);
 extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);

arch/x86/kernel/acpi/boot.c +0 -0

index 560a6d1cb0f4..6aa796f77b71 100644

Quoted 31 lines · Jiang Liu

Introduce acpi_ioapic_registered() to check whether an IOAPIC has already been registered, it will be used when enabling IOAPIC hotplug.

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/include/asm/io_apic.h |    1 +
 arch/x86/kernel/acpi/boot.c    |   11 +++++++++++
 arch/x86/kernel/apic/io_apic.c |   11 +++++++++++
 include/linux/acpi.h           |    1 +
 4 files changed, 24 insertions(+)

arch/x86/include/asm/io_apic.h +1 -0

index ce63cf34c93c..0db2b7037e4b 100644
@@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq);
 extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 			      struct ioapic_domain_cfg *cfg);
 extern int mp_unregister_ioapic(u32 gsi_base);
+extern int mp_ioapic_registered(u32 gsi_base);
 extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 			    irq_hw_number_t hwirq);
 extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);

arch/x86/kernel/acpi/boot.c +0 -0

index 560a6d1cb0f4..6aa796f77b71 100644
@@ -810,6 +810,17 @@ int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base)
 }
 EXPORT_SYMBOL(acpi_unregister_ioapic);
 

arch/x86/kernel/acpi/boot.c int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) · line 817

Quoted 5 lines

arch/x86/kernel/acpi/boot.c +5 -0

+int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base)
+{
+	int ret;
+
+	down_write(&acpi_ioapic_rwsem);

Why down_write? You are merily checking whether the thing is registered already.

arch/x86/kernel/acpi/boot.c int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) · line 822

Quoted 5 lines

arch/x86/kernel/acpi/boot.c +5 -0

+	ret  = mp_ioapic_registered(gsi_base);
+	up_write(&acpi_ioapic_rwsem);
+
+	return ret;
+}

So I assume that this is for a particular caller in the apci ioapic hotplug code and that call site has its own serialization. Otherwise the return value is not protected at all.

Please add a Docbook comment to that function, and document the locking rules as thats pretty non obvious.

The such is missing in some other patches as well.

Thanks,

tglx

Quoted 10 lines · Thomas Gleixner

arch/x86/kernel/acpi/boot.c int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) · line 822

Quoted 5 lines

arch/x86/kernel/acpi/boot.c +5 -0

+	ret  = mp_ioapic_registered(gsi_base);
+	up_write(&acpi_ioapic_rwsem);
+
+	return ret;
+}

So I assume that this is for a particular caller in the apci ioapic hotplug code and that call site has its own serialization. Otherwise the return value is not protected at all.

Quoted 2 lines

Please add a Docbook comment to that function, and document the locking rules as thats pretty non obvious.

How about this comments about locking rules?
/*
* Locks related to IOAPIC hotplug
* Hotplug side:
* ->lock_device_hotplug() //device_hotplug_lock
* ->acpi_ioapic_rwsem
* ->ioapic_lock
* Interrupt mapping side:
* ->acpi_ioapic_rwsem
* ->ioapic_mutex
* ->ioapic_lock
*/

Quoted 13 lines · Jiang Liu

Quoted 10 lines · Thomas Gleixner

arch/x86/kernel/acpi/boot.c int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) · line 822

Quoted 5 lines

arch/x86/kernel/acpi/boot.c +5 -0

+	ret  = mp_ioapic_registered(gsi_base);
+	up_write(&acpi_ioapic_rwsem);
+
+	return ret;
+}

So I assume that this is for a particular caller in the apci ioapic hotplug code and that call site has its own serialization. Otherwise the return value is not protected at all.

Quoted 2 lines

Please add a Docbook comment to that function, and document the locking rules as thats pretty non obvious.

Quoted 12 lines

How about this comments about locking rules?
/*
* Locks related to IOAPIC hotplug
* Hotplug side:
* ->lock_device_hotplug() //device_hotplug_lock
* ->acpi_ioapic_rwsem
* ->ioapic_lock
* Interrupt mapping side:
* ->acpi_ioapic_rwsem
* ->ioapic_mutex
* ->ioapic_lock
*/

That looks asymetric. Why is the hotplug side not taking ioapic_mutex?

Thanks,

tglx

This patch set enhances IOAPIC core and ACPI drivers to support IOAPIC hotplug on x86 platforms. It's based on latest mainstream kernel at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

You may pull it from
https://github.com/jiangliu/linux.git ioapic/hotplug_v4

We have pick up several patches from Yinghai's original IOAPIC hotplug patch set and reimplemented IOAPIC driver as an ACPI driver instead of a PCI driver.

It has been tested on a 4-socket Intel SDV with socket hot-addition capability. Any suggestions are welcomed!

Patch 1-5 are bugfixes and enhancements to ACPI subsystem Patch 6-14 enhances IOAPIC core to support IOAPIC hotplug Patch 15 killes PCI IOAPIC driver
Patch 16 reimplements ACPI IOAPIC driver and enables IOAPIC hotplug

V3->V4:
1) Fix a bug in manage IOAPIC reference count
2) Rebase to v3.17-rc2
3) Refine commit messages
V2->V3:
1) Refine ACPI resource walk functions for PCI root bus and IOAPIC
2) Improve commit messages
3) Reorder patch order for better maintenence

Jiang Liu (13):
x86, PCI, ACPI: Kill private function resource_to_addr() in

    arch/x86/pci/acpi.c

ACPI: Correct return value of acpi_dev_resource_address_space()
ACPI: Fix minor syntax issues in processor_core.c
ACPI: Rename processor_core.c as apic_id.c
x86, irq: Remove __init marker for functions will be used by IOAPIC
hotplug
x86, irq: Keep balance of IOAPIC pin reference count x86, irq: Refine mp_register_ioapic() to prepare for IOAPIC hotplug x86, irq, ACPI: Introduce a rwsem to protect IOAPIC operations from
hotplug
x86, irq, ACPI: Implement interface to support ACPI based IOAPIC
hot-addition
x86, irq, ACPI: Implement interfaces to support ACPI based IOAPIC
hot-removal
x86, irq: Introduce helper to check whether an IOAPIC has been
registered
PCI: Remove PCI ioapic driver
x86, irq, ACPI: Implement ACPI driver to support IOAPIC hotplug

Yinghai Lu (3):
ACPI: Add interfaces to parse IOAPIC ID for IOAPIC hotplug x86, irq: Split out alloc_ioapic_save_registers() x86, irq: Prefer assigned ID in APIC ID register for x86_64