[PATCH v2 0/6] x86: ACPI / APIC / IOMMU interaction

[PATCH v2 0/6] x86: ACPI / APIC / IOMMU interaction

[Jan Beulich]

While reworking patch 1 (the only one here that I consider a candidate
for 4.16; see there) I stumbled over quite a few things that have long
been ripe for cleaning up. Hence the tail of 5 further patches ...

1: x2APIC: defer probe until after IOMMU ACPI table parsing
2: APIC: drop clustered_apic_check() hook
3: APIC: drop {acpi_madt,mps}_oem_check() hooks
4: APIC: drop probe_default()
5: APIC: rename cmdline_apic
6: ACPI: drop dead interpreter-related code

Jan

[PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
when ACPI tables are missing") deals with apic_x2apic_probe() as called
from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
affected: The call needs to occur after acpi_iommu_init(), such that
iommu_intremap getting disabled there can be properly taken into account
by apic_x2apic_probe().

Note that, for the time being (further cleanup patches following),
reversing the order of the calls to generic_apic_probe() and
acpi_boot_init() is not an option:
- acpi_process_madt() calls clustered_apic_check() and hence relies on
  genapic to have got filled before,
- generic_bigsmp_probe() (called from acpi_process_madt()) needs to
  occur after generic_apic_probe(),
- acpi_parse_madt() (called from acpi_process_madt()) calls
  acpi_madt_oem_check(), which wants to be after generic_apic_probe().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Based on code inspection only - I have no affected system and hence no
way to actually test the case.
---
v2: Don't move generic_apic_probe() invocation, instead pull out
    acpi_iommu_init() from acpi_boot_init().
---
4.16: While investigating the issue addressed by the referenced commit,
      a variant of that problem was identified when firmware pre-enables
      x2APIC mode. Whether that's something sane firmware would do when
      at the same time IOMMU(s) is/are disabled is unclear, so this may
      be a purely academical consideration. Working around the problem
      also ought to be as simple as passing "iommu=no-intremap" on the
      command line. Considering the fragility of the code (as further
      demonstrated by v1 having been completely wrong), it may therefore
      be advisable to defer this change until after branching.
      Nevertheless it will then be a backporting candidate, so
      considering to take it right away can't simply be put off.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
 
 	acpi_mmcfg_init();
 
-	acpi_iommu_init();
-
 	erst_init();
 
 	acpi_hest_init();
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
 
     dmi_scan_machine();
 
+    /*
+     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
+     * check_x2apic_preenabled() to be able to observe respective findings, in
+     * particular iommu_intremap having got turned off.
+     */
+    acpi_iommu_init();
+
     generic_apic_probe();
 
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",

[PATCH v2 2/6] x86/APIC: drop clustered_apic_check() hook

[Jan Beulich]

The hook functions have been empty forever (x2APIC) or issuing merely a
printk() for a long time (xAPIC). Since that printk() is (a) generally
useful (i.e. also in the x2APIC case) and (b) would better only be
issued once the final APIC driver to use was determined, move (and
generalize) it into connect_bsp_APIC().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -674,9 +674,7 @@ static void __init acpi_process_madt(voi
 			error = acpi_parse_madt_ioapic_entries();
 			if (!error) {
 				acpi_ioapic = true;
-
 				smp_found_config = true;
-				clustered_apic_check();
 			}
 		}
 		if (error == -EINVAL) {
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -243,6 +243,12 @@ void __init connect_bsp_APIC(void)
         outb(0x70, 0x22);
         outb(0x01, 0x23);
     }
+
+    printk("Enabling APIC mode:  %s.  Using %d I/O APICs\n",
+           !INT_DEST_MODE ? "Physical"
+                          : init_apic_ldr == init_apic_ldr_flat ? "Flat"
+                                                                : "Clustered",
+           nr_ioapics);
     enable_apic_mode();
 }
 
--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -19,11 +19,6 @@ void init_apic_ldr_flat(void)
 	apic_write(APIC_LDR, val);
 }
 
-void __init clustered_apic_check_flat(void)
-{
-	printk("Enabling APIC mode:  Flat.  Using %d I/O APICs\n", nr_ioapics);
-}
-
 const cpumask_t *vector_allocation_cpumask_flat(int cpu)
 {
 	return &cpu_online_map;
@@ -43,11 +38,6 @@ void init_apic_ldr_phys(void)
 	/* We only deliver in phys mode - no setup needed. */
 }
 
-void __init clustered_apic_check_phys(void)
-{
-	printk("Enabling APIC mode:  Phys.  Using %d I/O APICs\n", nr_ioapics);
-}
-
 const cpumask_t *vector_allocation_cpumask_phys(int cpu)
 {
 	return cpumask_of(cpu);
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -75,10 +75,6 @@ static void init_apic_ldr_x2apic_cluster
     cpumask_set_cpu(this_cpu, per_cpu(cluster_cpus, this_cpu));
 }
 
-static void __init clustered_apic_check_x2apic(void)
-{
-}
-
 static const cpumask_t *vector_allocation_cpumask_x2apic_cluster(int cpu)
 {
     return per_cpu(cluster_cpus, cpu);
@@ -175,7 +171,6 @@ static const struct genapic __initconstr
     .int_delivery_mode = dest_Fixed,
     .int_dest_mode = 0 /* physical delivery */,
     .init_apic_ldr = init_apic_ldr_phys,
-    .clustered_apic_check = clustered_apic_check_x2apic,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
     .send_IPI_mask = send_IPI_mask_x2apic_phys,
@@ -187,7 +182,6 @@ static const struct genapic __initconstr
     .int_delivery_mode = dest_LowestPrio,
     .int_dest_mode = 1 /* logical delivery */,
     .init_apic_ldr = init_apic_ldr_x2apic_cluster,
-    .clustered_apic_check = clustered_apic_check_x2apic,
     .vector_allocation_cpumask = vector_allocation_cpumask_x2apic_cluster,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_x2apic_cluster,
     .send_IPI_mask = send_IPI_mask_x2apic_cluster,
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -410,7 +410,6 @@ static int __init smp_read_mpc(struct mp
 			}
 		}
 	}
-	clustered_apic_check();
 	if (!num_processors)
 		printk(KERN_ERR "SMP mptable: no processors registered!\n");
 	return num_processors;
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -32,7 +32,6 @@ struct genapic {
 	int int_delivery_mode;
 	int int_dest_mode;
 	void (*init_apic_ldr)(void);
-	void (*clustered_apic_check)(void);
 	const cpumask_t *(*vector_allocation_cpumask)(int cpu);
 	unsigned int (*cpu_mask_to_apicid)(const cpumask_t *cpumask);
 	void (*send_IPI_mask)(const cpumask_t *mask, int vector);
@@ -54,7 +53,6 @@ extern const struct genapic apic_bigsmp;
 void send_IPI_self_legacy(uint8_t vector);
 
 void init_apic_ldr_flat(void);
-void clustered_apic_check_flat(void);
 unsigned int cpu_mask_to_apicid_flat(const cpumask_t *cpumask);
 void send_IPI_mask_flat(const cpumask_t *mask, int vector);
 const cpumask_t *vector_allocation_cpumask_flat(int cpu);
@@ -62,14 +60,12 @@ const cpumask_t *vector_allocation_cpuma
 	.int_delivery_mode = dest_LowestPrio, \
 	.int_dest_mode = 1 /* logical delivery */, \
 	.init_apic_ldr = init_apic_ldr_flat, \
-	.clustered_apic_check = clustered_apic_check_flat, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_flat, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \
 	.send_IPI_mask = send_IPI_mask_flat, \
 	.send_IPI_self = send_IPI_self_legacy
 
 void init_apic_ldr_phys(void);
-void clustered_apic_check_phys(void);
 unsigned int cpu_mask_to_apicid_phys(const cpumask_t *cpumask);
 void send_IPI_mask_phys(const cpumask_t *mask, int vector);
 const cpumask_t *vector_allocation_cpumask_phys(int cpu);
@@ -77,7 +73,6 @@ const cpumask_t *vector_allocation_cpuma
 	.int_delivery_mode = dest_Fixed, \
 	.int_dest_mode = 0 /* physical delivery */, \
 	.init_apic_ldr = init_apic_ldr_phys, \
-	.clustered_apic_check = clustered_apic_check_phys, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_phys, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys, \
 	.send_IPI_mask = send_IPI_mask_phys, \
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -14,7 +14,6 @@
 #define INT_DEST_MODE (genapic.int_dest_mode)
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr (genapic.init_apic_ldr)
-#define clustered_apic_check (genapic.clustered_apic_check)
 #define cpu_mask_to_apicid(mask) ({ \
 	/* \
 	 * There are a number of places where the address of a local variable \

[PATCH v2 3/6] x86/APIC: drop {acpi_madt,mps}_oem_check() hooks

[Jan Beulich]

The hook functions have been empty for a very long time, if not
(according to git history) forever. Ditch them alongside the then empty
mach_mpparse.h instances and the then unused APICFUNC() macro.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -39,7 +39,6 @@
 #include <asm/processor.h>
 #include <asm/hpet.h> /* for hpet_address */
 #include <mach_apic.h>
-#include <mach_mpparse.h>
 
 #define PREFIX			"ACPI: "
 
@@ -75,8 +74,6 @@ static int __init acpi_parse_madt(struct
 		       madt->address);
 	}
 
-	acpi_madt_oem_check(madt->header.oem_id, madt->header.oem_table_id);
-
 	return 0;
 }
 
--- a/xen/arch/x86/genapic/bigsmp.c
+++ b/xen/arch/x86/genapic/bigsmp.c
@@ -8,7 +8,6 @@
 #include <xen/smp.h>
 #include <xen/init.h>
 #include <xen/dmi.h>
-#include <asm/mach-default/mach_mpparse.h>
 #include <asm/io_apic.h>
 
 static __init int force_bigsmp(const struct dmi_system_id *d)
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -12,7 +12,6 @@
 #include <xen/smp.h>
 #include <xen/init.h>
 #include <asm/io_apic.h>
-#include <asm/mach-default/mach_mpparse.h>
 
 /* should be called last. */
 static __init int probe_default(void)
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -78,39 +78,3 @@ void __init generic_apic_probe(void)
 
 	printk(KERN_INFO "Using APIC driver %s\n", genapic.name);
 } 
-
-/* These functions can switch the APIC even after the initial ->probe() */
-
-int __init mps_oem_check(struct mp_config_table *mpc, char *oem, char *productid)
-{ 
-	int i;
-	for (i = 0; apic_probe[i]; ++i) { 
-		if (apic_probe[i]->mps_oem_check(mpc,oem,productid)) { 
-			if (!cmdline_apic &&
-			     genapic.name != apic_probe[i]->name) {
-				genapic = *apic_probe[i];
-				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic.name);
-			}
-			return 1;
-		} 
-	} 
-	return 0;
-} 
-
-int __init acpi_madt_oem_check(char *oem_id, char *oem_table_id)
-{
-	int i;
-	for (i = 0; apic_probe[i]; ++i) { 
-		if (apic_probe[i]->acpi_madt_oem_check(oem_id, oem_table_id)) { 
-			if (!cmdline_apic &&
-			     genapic.name != apic_probe[i]->name) {
-				genapic = *apic_probe[i];
-				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic.name);
-			}
-			return 1;
-		} 
-	} 
-	return 0;	
-}
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -27,7 +27,6 @@
 #include <asm/msr.h>
 #include <asm/processor.h>
 #include <xen/smp.h>
-#include <asm/mach-default/mach_mpparse.h>
 
 static DEFINE_PER_CPU_READ_MOSTLY(u32, cpu_2_logical_apicid);
 static DEFINE_PER_CPU_READ_MOSTLY(cpumask_t *, cluster_cpus);
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -30,7 +30,6 @@
 #include <asm/setup.h>
 
 #include <mach_apic.h>
-#include <mach_mpparse.h>
 #include <bios_ebda.h>
 
 /* Have we found an MP table */
@@ -326,8 +325,6 @@ static int __init smp_read_mpc(struct mp
 	str[12]=0;
 	printk("Product ID: %s ",str);
 
-	mps_oem_check(mpc, oem, str);
-
 	printk("APIC at: %#x\n", mpc->mpc_lapic);
 
 	/* 
--- a/xen/arch/x86/x86_64/acpi_mmcfg.c
+++ b/xen/arch/x86/x86_64/acpi_mmcfg.c
@@ -38,7 +38,6 @@
 #include <asm/mpspec.h>
 #include <asm/processor.h>
 #include <mach_apic.h>
-#include <mach_mpparse.h>
 
 #include "mmconfig.h"
 
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -21,13 +21,6 @@ struct genapic {
 	const char *name;
 	int (*probe)(void);
 
-	/* When one of the next two hooks returns 1 the genapic
-	   is switched to this. Essentially they are additional probe 
-	   functions. */
-	int (*mps_oem_check)(struct mp_config_table *mpc, char *oem, 
-			      char *productid);
-	int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
-
 	/* Interrupt delivery parameters ('physical' vs. 'logical flat'). */
 	int int_delivery_mode;
 	int int_dest_mode;
@@ -38,13 +31,9 @@ struct genapic {
     void (*send_IPI_self)(uint8_t vector);
 };
 
-#define APICFUNC(x) .x = x
-
 #define APIC_INIT(aname, aprobe) \
 	.name = aname, \
-	.probe = aprobe, \
-	APICFUNC(mps_oem_check), \
-	APICFUNC(acpi_madt_oem_check)
+	.probe = aprobe
 
 extern struct genapic genapic;
 extern const struct genapic apic_default;
--- a/xen/include/asm-x86/mach-default/mach_mpparse.h
+++ /dev/null
@@ -1,17 +0,0 @@
-#ifndef __ASM_MACH_MPPARSE_H
-#define __ASM_MACH_MPPARSE_H
-
-static inline int __init mps_oem_check(struct mp_config_table *mpc, char *oem,
-				       char *productid)
-{
-	return 0;
-}
-
-/* Hook from generic ACPI tables.c */
-static inline int __init acpi_madt_oem_check(char *oem_id, char *oem_table_id)
-{
-	return 0;
-}
-
-
-#endif /* __ASM_MACH_MPPARSE_H */
--- a/xen/include/asm-x86/mach-generic/mach_mpparse.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef _MACH_MPPARSE_H
-#define _MACH_MPPARSE_H 1
-
-int mps_oem_check(struct mp_config_table *mpc, char *oem, char *productid);
-int acpi_madt_oem_check(char *oem_id, char *oem_table_id);
-
-#endif

[PATCH v2 4/6] x86/APIC: drop probe_default()

[Jan Beulich]

The function does nothing but return success. Simply treat absence of a
probe hook to mean just this. This then eliminates the (purely
theoretical at this point) risk of trying to call through
apic_x2apic_{cluster,phys}'s respective NULL pointers.

While doing this also eliminate generic_apic_probe()'s "changed"
variable: apic_probe[]'s default entry will now be used unconditionally
in yet more obvious a way, such that separately setting genapic from
apic_default is (hopefully) no longer justified. Yet that was the main
purpose of the variable.

To help prove that apic_default's probe() hook doesn't get used
elsewhere, further make apic_probe[] static at this occasion.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -14,12 +14,7 @@
 #include <asm/io_apic.h>
 
 /* should be called last. */
-static __init int probe_default(void)
-{ 
-	return 1;
-} 
-
 const struct genapic __initconstrel apic_default = {
-	APIC_INIT("default", probe_default),
+	APIC_INIT("default", NULL),
 	GENAPIC_FLAT
 };
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -18,7 +18,7 @@
 
 struct genapic __read_mostly genapic;
 
-const struct genapic *const __initconstrel apic_probe[] = {
+static const struct genapic *const __initconstrel apic_probe[] = {
 	&apic_bigsmp, 
 	&apic_default,	/* must be last */
 	NULL,
@@ -59,22 +59,20 @@ custom_param("apic", genapic_apic_force)
 
 void __init generic_apic_probe(void) 
 { 
-	bool changed;
 	int i;
 
 	record_boot_APIC_mode();
 
 	check_x2apic_preenabled();
-	cmdline_apic = changed = !!genapic.name;
 
-	for (i = 0; !changed && apic_probe[i]; i++) { 
-		if (apic_probe[i]->probe()) {
-			changed = 1;
+	cmdline_apic = genapic.name;
+
+	for (i = 0; !genapic.name && apic_probe[i]; i++) {
+		if (!apic_probe[i]->probe || apic_probe[i]->probe())
 			genapic = *apic_probe[i];
-		} 
 	}
-	if (!changed) 
-		genapic = apic_default;
+
+	BUG_ON(!genapic.name);
 
 	printk(KERN_INFO "Using APIC driver %s\n", genapic.name);
 } 

[PATCH v2 5/6] x86/APIC: rename cmdline_apic

[Jan Beulich]

The name hasn't been appropriate for a long time: It covers not only
command line overrides, but also x2APIC pre-enabled state.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -24,7 +24,7 @@ static const struct genapic *const __ini
 	NULL,
 };
 
-static bool_t __initdata cmdline_apic;
+static bool __initdata forced_apic;
 
 void __init generic_bigsmp_probe(void)
 {
@@ -35,7 +35,7 @@ void __init generic_bigsmp_probe(void)
 	 * - we find more than 8 CPUs in acpi LAPIC listing with xAPIC support
 	 */
 
-	if (!cmdline_apic && genapic.name == apic_default.name)
+	if (!forced_apic && genapic.name == apic_default.name)
 		if (apic_bigsmp.probe()) {
 			genapic = apic_bigsmp;
 			printk(KERN_INFO "Overriding APIC driver with %s\n",
@@ -65,7 +65,7 @@ void __init generic_apic_probe(void)
 
 	check_x2apic_preenabled();
 
-	cmdline_apic = genapic.name;
+	forced_apic = genapic.name;
 
 	for (i = 0; !genapic.name && apic_probe[i]; i++) {
 		if (!apic_probe[i]->probe || apic_probe[i]->probe())

[PATCH v2 6/6] x86/ACPI: drop dead interpreter-related code

[Jan Beulich]

CONFIG_ACPI_INTERPRETER does not get defined anywhere, the enclosed code
wouldn't build, and the default-to-phys logic works differently anyway
(see genapic/bigsmp.c:probe_bigsmp()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -477,16 +477,6 @@ static int __init acpi_parse_fadt(struct
 	const struct acpi_table_fadt *fadt =
 		container_of(table, const struct acpi_table_fadt, header);
 
-#ifdef	CONFIG_ACPI_INTERPRETER
-	/* initialize sci_int early for INT_SRC_OVR MADT parsing */
-	acpi_fadt.sci_int = fadt->sci_int;
-
-	/* initialize rev and apic_phys_dest_mode for x86_64 genapic */
-	acpi_fadt.revision = fadt->revision;
-	acpi_fadt.force_apic_physical_destination_mode =
-	    fadt->force_apic_physical_destination_mode;
-#endif
-
 	/* detect the location of the ACPI PM Timer */
 	if (fadt->header.revision >= FADT2_REVISION_ID &&
 	    fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné]

On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> affected: The call needs to occur after acpi_iommu_init(), such that
> iommu_intremap getting disabled there can be properly taken into account
> by apic_x2apic_probe().
> 
> Note that, for the time being (further cleanup patches following),
> reversing the order of the calls to generic_apic_probe() and
> acpi_boot_init() is not an option:
> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>   genapic to have got filled before,
> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>   occur after generic_apic_probe(),
> - acpi_parse_madt() (called from acpi_process_madt()) calls
>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.
> ---
> v2: Don't move generic_apic_probe() invocation, instead pull out
>     acpi_iommu_init() from acpi_boot_init().
> ---
> 4.16: While investigating the issue addressed by the referenced commit,
>       a variant of that problem was identified when firmware pre-enables
>       x2APIC mode. Whether that's something sane firmware would do when
>       at the same time IOMMU(s) is/are disabled is unclear, so this may
>       be a purely academical consideration. Working around the problem
>       also ought to be as simple as passing "iommu=no-intremap" on the
>       command line. Considering the fragility of the code (as further
>       demonstrated by v1 having been completely wrong), it may therefore
>       be advisable to defer this change until after branching.
>       Nevertheless it will then be a backporting candidate, so
>       considering to take it right away can't simply be put off.

The main issue here would be missing a dependency between
acpi_iommu_init and the rest of acpi_boot_init, apart from the
existing dependencies between acpi_iommu_init and generic_apic_probe.

> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
>  
>  	acpi_mmcfg_init();
>  
> -	acpi_iommu_init();
> -
>  	erst_init();
>  
>  	acpi_hest_init();
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>  
>      dmi_scan_machine();
>  
> +    /*
> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
> +     * check_x2apic_preenabled() to be able to observe respective findings, in
> +     * particular iommu_intremap having got turned off.
> +     */
> +    acpi_iommu_init();

If we pull this out I think we should add a check for acpi_disabled
and if set turn off iommu_intremap and iommu_enable?

Thanks, Roger.

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> > 4.16: While investigating the issue addressed by the referenced commit,
> >       a variant of that problem was identified when firmware pre-enables
> >       x2APIC mode. Whether that's something sane firmware would do when
> >       at the same time IOMMU(s) is/are disabled is unclear, so this may
> >       be a purely academical consideration. Working around the problem
> >       also ought to be as simple as passing "iommu=no-intremap" on the
> >       command line. Considering the fragility of the code (as further
> >       demonstrated by v1 having been completely wrong), it may therefore
> >       be advisable to defer this change until after branching.

Thanks for the frank analysis.

> The main issue here would be missing a dependency between
> acpi_iommu_init and the rest of acpi_boot_init, apart from the
> existing dependencies between acpi_iommu_init and generic_apic_probe.

I have been thinking about this.  I'm still not sure.

> >       Nevertheless it will then be a backporting candidate, so
> >       considering to take it right away can't simply be put off.

This part confused me.  Under what circumstances would we backport
this ?  AIUI it would be backporting a potentially-fragile and
not-readily-testable bugfix, for a theoretical scenario with a
straightforward workaround.

Ian.

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 05.11.2021 16:38, Roger Pau Monné wrote:
> On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>>  
>>      dmi_scan_machine();
>>  
>> +    /*
>> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
>> +     * check_x2apic_preenabled() to be able to observe respective findings, in
>> +     * particular iommu_intremap having got turned off.
>> +     */
>> +    acpi_iommu_init();
> 
> If we pull this out I think we should add a check for acpi_disabled
> and if set turn off iommu_intremap and iommu_enable?

Hmm, I should have added a note regarding this. If we want to exactly
retain prior behavior, acpi_ht would also need checking. Yet that has
gone wrong long ago: We parse way too many tables when acpi_disabled
&& acpi_ht, and hence while correct wrt to prior behavior I'd consider
it wrong to (re)add a "!acpi_ht" check.

As a result I'm of the opinion that checking acpi_disabled here also
isn't necessarily appropriate, and instead IOMMU disabling would
better be solely under the control of "iommu=".

Additionally iirc Andrew has been suggesting to drop all this "ACPI
disabled / HT-only" machinery (I'm somewhat hesitant with that, but
as a result I'm also not very eager to actually correct to accumulated
bad behavior). The change here simply would be a tiny first step in
that direction.

Jan

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 05.11.2021 16:47, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
>>> 4.16: While investigating the issue addressed by the referenced commit,
>>>       a variant of that problem was identified when firmware pre-enables
>>>       x2APIC mode. Whether that's something sane firmware would do when
>>>       at the same time IOMMU(s) is/are disabled is unclear, so this may
>>>       be a purely academical consideration. Working around the problem
>>>       also ought to be as simple as passing "iommu=no-intremap" on the
>>>       command line. Considering the fragility of the code (as further
>>>       demonstrated by v1 having been completely wrong), it may therefore
>>>       be advisable to defer this change until after branching.
> 
> Thanks for the frank analysis.
> 
>> The main issue here would be missing a dependency between
>> acpi_iommu_init and the rest of acpi_boot_init, apart from the
>> existing dependencies between acpi_iommu_init and generic_apic_probe.
> 
> I have been thinking about this.  I'm still not sure.
> 
>>>       Nevertheless it will then be a backporting candidate, so
>>>       considering to take it right away can't simply be put off.
> 
> This part confused me.  Under what circumstances would we backport
> this ?  AIUI it would be backporting a potentially-fragile and
> not-readily-testable bugfix, for a theoretical scenario with a
> straightforward workaround.

Well, I've said "candidate" for this very reason: To me, every bug
fix is a candidate. Whether risks outweigh the potential benefits is
then influencing whether to _actually_ take the change. A reason to
take it despite the available workaround might be that
"straightforward" doesn't also mean "obvious" here. IOW once you
know what to do, doing so is easy. But one first needs to arrive
there.

Jan

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné]

On Mon, Nov 08, 2021 at 08:40:59AM +0100, Jan Beulich wrote:
> On 05.11.2021 16:38, Roger Pau Monné wrote:
> > On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
> >>  
> >>      dmi_scan_machine();
> >>  
> >> +    /*
> >> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
> >> +     * check_x2apic_preenabled() to be able to observe respective findings, in
> >> +     * particular iommu_intremap having got turned off.
> >> +     */
> >> +    acpi_iommu_init();
> > 
> > If we pull this out I think we should add a check for acpi_disabled
> > and if set turn off iommu_intremap and iommu_enable?
> 
> Hmm, I should have added a note regarding this. If we want to exactly
> retain prior behavior, acpi_ht would also need checking. Yet that has
> gone wrong long ago: We parse way too many tables when acpi_disabled
> && acpi_ht, and hence while correct wrt to prior behavior I'd consider
> it wrong to (re)add a "!acpi_ht" check.
> 
> As a result I'm of the opinion that checking acpi_disabled here also
> isn't necessarily appropriate, and instead IOMMU disabling would
> better be solely under the control of "iommu=".

I haven't looked very deeply, but will the acpi helpers work correctly
in that case? As acpi_boot_table_init will be short-circuited if
 `acpi_disabled && !acpi_ht`.

Thanks, Roger.

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 08.11.2021 10:36, Roger Pau Monné wrote:
> On Mon, Nov 08, 2021 at 08:40:59AM +0100, Jan Beulich wrote:
>> On 05.11.2021 16:38, Roger Pau Monné wrote:
>>> On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>>>>  
>>>>      dmi_scan_machine();
>>>>  
>>>> +    /*
>>>> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
>>>> +     * check_x2apic_preenabled() to be able to observe respective findings, in
>>>> +     * particular iommu_intremap having got turned off.
>>>> +     */
>>>> +    acpi_iommu_init();
>>>
>>> If we pull this out I think we should add a check for acpi_disabled
>>> and if set turn off iommu_intremap and iommu_enable?
>>
>> Hmm, I should have added a note regarding this. If we want to exactly
>> retain prior behavior, acpi_ht would also need checking. Yet that has
>> gone wrong long ago: We parse way too many tables when acpi_disabled
>> && acpi_ht, and hence while correct wrt to prior behavior I'd consider
>> it wrong to (re)add a "!acpi_ht" check.
>>
>> As a result I'm of the opinion that checking acpi_disabled here also
>> isn't necessarily appropriate, and instead IOMMU disabling would
>> better be solely under the control of "iommu=".
> 
> I haven't looked very deeply, but will the acpi helpers work correctly
> in that case? As acpi_boot_table_init will be short-circuited if
>  `acpi_disabled && !acpi_ht`.

Oh, that's a good point you make.

Jan

Re: [PATCH v2 2/6] x86/APIC: drop clustered_apic_check() hook

[Roger Pau Monné]

On Fri, Nov 05, 2021 at 01:34:12PM +0100, Jan Beulich wrote:
> The hook functions have been empty forever (x2APIC) or issuing merely a
> printk() for a long time (xAPIC). Since that printk() is (a) generally
> useful (i.e. also in the x2APIC case) and (b) would better only be
> issued once the final APIC driver to use was determined, move (and
> generalize) it into connect_bsp_APIC().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v2: New.
> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -674,9 +674,7 @@ static void __init acpi_process_madt(voi
>  			error = acpi_parse_madt_ioapic_entries();
>  			if (!error) {
>  				acpi_ioapic = true;
> -
>  				smp_found_config = true;
> -				clustered_apic_check();
>  			}
>  		}
>  		if (error == -EINVAL) {
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -243,6 +243,12 @@ void __init connect_bsp_APIC(void)
>          outb(0x70, 0x22);
>          outb(0x01, 0x23);
>      }
> +
> +    printk("Enabling APIC mode:  %s.  Using %d I/O APICs\n",

I don't think it makes sense to prefix APIC with 'x' or 'x2' here, as
we already print the APIC mode elsewhere?

> +           !INT_DEST_MODE ? "Physical"
> +                          : init_apic_ldr == init_apic_ldr_flat ? "Flat"
> +                                                                : "Clustered",
> +           nr_ioapics);
>      enable_apic_mode();

This also seem to be completely unneeded? I guess it would be cleaned
in a further patch.

Thanks, Roger.

Re: [PATCH v2 2/6] x86/APIC: drop clustered_apic_check() hook

[Jan Beulich]

On 08.11.2021 12:02, Roger Pau Monné wrote:
> On Fri, Nov 05, 2021 at 01:34:12PM +0100, Jan Beulich wrote:
>> The hook functions have been empty forever (x2APIC) or issuing merely a
>> printk() for a long time (xAPIC). Since that printk() is (a) generally
>> useful (i.e. also in the x2APIC case) and (b) would better only be
>> issued once the final APIC driver to use was determined, move (and
>> generalize) it into connect_bsp_APIC().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -243,6 +243,12 @@ void __init connect_bsp_APIC(void)
>>          outb(0x70, 0x22);
>>          outb(0x01, 0x23);
>>      }
>> +
>> +    printk("Enabling APIC mode:  %s.  Using %d I/O APICs\n",
> 
> I don't think it makes sense to prefix APIC with 'x' or 'x2' here, as
> we already print the APIC mode elsewhere?

I was indeed pondering that, and decided that the extra yet redundant
information wouldn't be worth the extra logic here (the more that
there's no good way to optionally print a single character, as sadly
%c does not print nothing when passed '\0', and I find single-char
string literals kind of ugly / wasteful). But I have no strong
opinion here, so if you think it would be better to add the extra
bits, I'll happily do so.

>> +           !INT_DEST_MODE ? "Physical"
>> +                          : init_apic_ldr == init_apic_ldr_flat ? "Flat"
>> +                                                                : "Clustered",
>> +           nr_ioapics);
>>      enable_apic_mode();
> 
> This also seem to be completely unneeded? I guess it would be cleaned
> in a further patch.

I have to admit I didn't even check. There's so much more cleanup to
do here ...

Jan

[PATCH v2.1 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
when ACPI tables are missing") deals with apic_x2apic_probe() as called
from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
affected: The call needs to occur after acpi_iommu_init(), such that
iommu_intremap getting disabled there can be properly taken into account
by apic_x2apic_probe().

Note that, for the time being (further cleanup patches following),
reversing the order of the calls to generic_apic_probe() and
acpi_boot_init() is not an option:
- acpi_process_madt() calls clustered_apic_check() and hence relies on
  genapic to have got filled before,
- generic_bigsmp_probe() (called from acpi_process_madt()) needs to
  occur after generic_apic_probe(),
- acpi_parse_madt() (called from acpi_process_madt()) calls
  acpi_madt_oem_check(), which wants to be after generic_apic_probe().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Based on code inspection only - I have no affected system and hence no
way to actually test the case.
---
v2.1: Respect acpi_disabled in acpi_iommu_init().
v2: Don't move generic_apic_probe() invocation, instead pull out
    acpi_iommu_init() from acpi_boot_init().
---
4.16: While investigating the issue addressed by the referenced commit,
      a variant of that problem was identified when firmware pre-enables
      x2APIC mode. Whether that's something sane firmware would do when
      at the same time IOMMU(s) is/are disabled is unclear, so this may
      be a purely academical consideration. Working around the problem
      also ought to be as simple as passing "iommu=no-intremap" on the
      command line. Considering the fragility of the code (as further
      demonstrated by v1 having been completely wrong), it may therefore
      be advisable to defer this change until after branching.
      Nevertheless it will then be a backporting candidate, so
      considering to take it right away can't simply be put off.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -759,8 +759,6 @@ int __init acpi_boot_init(void)
 
 	acpi_mmcfg_init();
 
-	acpi_iommu_init();
-
 	erst_init();
 
 	acpi_hest_init();
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1666,6 +1666,13 @@ void __init noreturn __start_xen(unsigne
 
     dmi_scan_machine();
 
+    /*
+     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
+     * check_x2apic_preenabled() to be able to observe respective findings, in
+     * particular iommu_intremap having got turned off.
+     */
+    acpi_iommu_init();
+
     generic_apic_probe();
 
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -44,14 +44,17 @@ bool __read_mostly iommu_intpost;
 
 void __init acpi_iommu_init(void)
 {
-    int ret;
+    int ret = -ENODEV;
 
     if ( !iommu_enable && !iommu_intremap )
         return;
 
-    ret = acpi_dmar_init();
-    if ( ret == -ENODEV )
-        ret = acpi_ivrs_init();
+    if ( !acpi_disabled )
+    {
+        ret = acpi_dmar_init();
+        if ( ret == -ENODEV )
+            ret = acpi_ivrs_init();
+    }
 
     if ( ret )
     {

Re: [PATCH v2.1 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné]

On Mon, Nov 08, 2021 at 12:40:59PM +0100, Jan Beulich wrote:
> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> affected: The call needs to occur after acpi_iommu_init(), such that
> iommu_intremap getting disabled there can be properly taken into account
> by apic_x2apic_probe().
> 
> Note that, for the time being (further cleanup patches following),
> reversing the order of the calls to generic_apic_probe() and
> acpi_boot_init() is not an option:
> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>   genapic to have got filled before,
> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>   occur after generic_apic_probe(),
> - acpi_parse_madt() (called from acpi_process_madt()) calls
>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> On 05.11.2021 16:47, Ian Jackson wrote:
> > This part confused me.  Under what circumstances would we backport
> > this ?  AIUI it would be backporting a potentially-fragile and
> > not-readily-testable bugfix, for a theoretical scenario with a
> > straightforward workaround.
> 
> Well, I've said "candidate" for this very reason: To me, every bug
> fix is a candidate. Whether risks outweigh the potential benefits is
> then influencing whether to _actually_ take the change. A reason to
> take it despite the available workaround might be that
> "straightforward" doesn't also mean "obvious" here. IOW once you
> know what to do, doing so is easy. But one first needs to arrive
> there.

Could we not do a smaller fix that would print something in the boot
output, mabye ?  That would be a lower risk change.

So far, I think the tradeoff here isn't looking very good: a risk of
unclear magnitude for many users, vs a hard crash at boot for a set of
users we believe to be empty.

As ever, feel free to contradict me if I have the wrong end of one of
the many sticks here...

Ian.

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 08.11.2021 16:04, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
>> On 05.11.2021 16:47, Ian Jackson wrote:
>>> This part confused me.  Under what circumstances would we backport
>>> this ?  AIUI it would be backporting a potentially-fragile and
>>> not-readily-testable bugfix, for a theoretical scenario with a
>>> straightforward workaround.
>>
>> Well, I've said "candidate" for this very reason: To me, every bug
>> fix is a candidate. Whether risks outweigh the potential benefits is
>> then influencing whether to _actually_ take the change. A reason to
>> take it despite the available workaround might be that
>> "straightforward" doesn't also mean "obvious" here. IOW once you
>> know what to do, doing so is easy. But one first needs to arrive
>> there.
> 
> Could we not do a smaller fix that would print something in the boot
> output, mabye ?  That would be a lower risk change.

Hmm, maybe something could be done, but at the risk of getting the
conditions there wrong (and hence having false positives and/or
false negatives, confusing users at best) and with likely a clumsy
log message ("abc ran before xyz"), suggesting that we actually
know how to do better. IOW - I'd rather not go this route, and it
would feel better to me to simply defer this change to post-4.16
if we deem it too risky to put it in now.

> So far, I think the tradeoff here isn't looking very good: a risk of
> unclear magnitude for many users, vs a hard crash at boot for a set of
> users we believe to be empty.
> 
> As ever, feel free to contradict me if I have the wrong end of one of
> the many sticks here...

I think you've got it quite right. I did put the question mark in the
tag specifically to make clear that while I'd like this to be
considered, I'm myself not convinced the risks outweigh the benefits.

Jan

Re: [PATCH v2.1 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 08.11.2021 12:54, Roger Pau Monné wrote:
> On Mon, Nov 08, 2021 at 12:40:59PM +0100, Jan Beulich wrote:
>> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
>> when ACPI tables are missing") deals with apic_x2apic_probe() as called
>> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
>> affected: The call needs to occur after acpi_iommu_init(), such that
>> iommu_intremap getting disabled there can be properly taken into account
>> by apic_x2apic_probe().
>>
>> Note that, for the time being (further cleanup patches following),
>> reversing the order of the calls to generic_apic_probe() and
>> acpi_boot_init() is not an option:
>> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>>   genapic to have got filled before,
>> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>>   occur after generic_apic_probe(),
>> - acpi_parse_madt() (called from acpi_process_madt()) calls
>>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, but testing again revealed a problem, this time on AMD. The
dependencies are yet more complicated, so it'll need to be a mix of
v1 and v2: acpi_iommu_init() needs to be split out, but
generic_apic_probe() also needs to move. That's because
_pci_hide_device() relies on dom_xen to have got set up before.
Expect v2.2 after I have actually tested the outlined change.

Jan

[PATCH v2.2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
when ACPI tables are missing") deals with apic_x2apic_probe() as called
from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
affected: The call needs to occur after acpi_iommu_init(), such that
iommu_intremap getting disabled there can be properly taken into account
by apic_x2apic_probe().

Note that, for the time being (further cleanup patches following),
reversing the order of the calls to generic_apic_probe() and
acpi_boot_init() is not an option:
- acpi_process_madt() calls clustered_apic_check() and hence relies on
  genapic to have got filled before,
- generic_bigsmp_probe() (called from acpi_process_madt()) needs to
  occur after generic_apic_probe(),
- acpi_parse_madt() (called from acpi_process_madt()) calls
  acpi_madt_oem_check(), which wants to be after generic_apic_probe().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Based on code inspection only - I have no affected system and hence no
way to actually test the case.
---
v2.2: Move generic_apic_probe() again, but only past acpi_iommu_init().
v2.1: Respect acpi_disabled in acpi_iommu_init().
v2: Don't move generic_apic_probe() invocation, instead pull out
    acpi_iommu_init() from acpi_boot_init().
---
4.16: While investigating the issue addressed by the referenced commit,
      a variant of that problem was identified when firmware pre-enables
      x2APIC mode. Whether that's something sane firmware would do when
      at the same time IOMMU(s) is/are disabled is unclear, so this may
      be a purely academical consideration. Working around the problem
      also ought to be as simple as passing "iommu=no-intremap" on the
      command line. Considering the fragility of the code (as further
      demonstrated by v1 having been completely wrong), it may therefore
      be advisable to defer this change until after branching.
      Nevertheless it will then be a backporting candidate, so
      considering to take it right away can't simply be put off.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
 
 	acpi_mmcfg_init();
 
-	acpi_iommu_init();
-
 	erst_init();
 
 	acpi_hest_init();
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1700,15 +1700,30 @@ void __init noreturn __start_xen(unsigne
 
     dmi_scan_machine();
 
-    generic_apic_probe();
-
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
                                   RANGESETF_prettyprint_hex);
 
     xsm_multiboot_init(module_map, mbi);
 
+    /*
+     * IOMMU-related ACPI table parsing may require some of the system domains
+     * to be usable.
+     */
     setup_system_domains();
 
+    /*
+     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
+     * check_x2apic_preenabled() to be able to observe respective findings, in
+     * particular iommu_intremap having got turned off.
+     */
+    acpi_iommu_init();
+
+    /*
+     * APIC probing needs to happen before general ACPI table parsing, as e.g.
+     * generic_bigsmp_probe() may occur only afterwards.
+     */
+    generic_apic_probe();
+
     acpi_boot_init();
 
     if ( smp_found_config )
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -43,14 +43,17 @@ bool __read_mostly iommu_intpost;
 
 void __init acpi_iommu_init(void)
 {
-    int ret;
+    int ret = -ENODEV;
 
     if ( !iommu_enable && !iommu_intremap )
         return;
 
-    ret = acpi_dmar_init();
-    if ( ret == -ENODEV )
-        ret = acpi_ivrs_init();
+    if ( !acpi_disabled )
+    {
+        ret = acpi_dmar_init();
+        if ( ret == -ENODEV )
+            ret = acpi_ivrs_init();
+    }
 
     if ( ret )
     {

Re: [PATCH v2.2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné]

On Mon, Nov 15, 2021 at 03:31:39PM +0100, Jan Beulich wrote:
> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> affected: The call needs to occur after acpi_iommu_init(), such that
> iommu_intremap getting disabled there can be properly taken into account
> by apic_x2apic_probe().
> 
> Note that, for the time being (further cleanup patches following),
> reversing the order of the calls to generic_apic_probe() and
> acpi_boot_init() is not an option:
> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>   genapic to have got filled before,
> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>   occur after generic_apic_probe(),
> - acpi_parse_madt() (called from acpi_process_madt()) calls
>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one comment below.

> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.
> ---
> v2.2: Move generic_apic_probe() again, but only past acpi_iommu_init().
> v2.1: Respect acpi_disabled in acpi_iommu_init().
> v2: Don't move generic_apic_probe() invocation, instead pull out
>     acpi_iommu_init() from acpi_boot_init().
> ---
> 4.16: While investigating the issue addressed by the referenced commit,
>       a variant of that problem was identified when firmware pre-enables
>       x2APIC mode. Whether that's something sane firmware would do when
>       at the same time IOMMU(s) is/are disabled is unclear, so this may
>       be a purely academical consideration. Working around the problem
>       also ought to be as simple as passing "iommu=no-intremap" on the
>       command line. Considering the fragility of the code (as further
>       demonstrated by v1 having been completely wrong), it may therefore
>       be advisable to defer this change until after branching.
>       Nevertheless it will then be a backporting candidate, so
>       considering to take it right away can't simply be put off.
> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
>  
>  	acpi_mmcfg_init();
>  
> -	acpi_iommu_init();
> -
>  	erst_init();
>  
>  	acpi_hest_init();
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1700,15 +1700,30 @@ void __init noreturn __start_xen(unsigne
>  
>      dmi_scan_machine();
>  
> -    generic_apic_probe();
> -
>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>                                    RANGESETF_prettyprint_hex);
>  
>      xsm_multiboot_init(module_map, mbi);
>  
> +    /*
> +     * IOMMU-related ACPI table parsing may require some of the system domains
> +     * to be usable.

I would be a bit more specific and likely add "...to be usable in
order to hide IOMMU PCI devices.".

Thanks, Roger.

Re: [PATCH v2.2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Jan Beulich]

On 15.11.2021 16:07, Roger Pau Monné wrote:
> On Mon, Nov 15, 2021 at 03:31:39PM +0100, Jan Beulich wrote:
>> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
>> when ACPI tables are missing") deals with apic_x2apic_probe() as called
>> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
>> affected: The call needs to occur after acpi_iommu_init(), such that
>> iommu_intremap getting disabled there can be properly taken into account
>> by apic_x2apic_probe().
>>
>> Note that, for the time being (further cleanup patches following),
>> reversing the order of the calls to generic_apic_probe() and
>> acpi_boot_init() is not an option:
>> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>>   genapic to have got filled before,
>> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>>   occur after generic_apic_probe(),
>> - acpi_parse_madt() (called from acpi_process_madt()) calls
>>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1700,15 +1700,30 @@ void __init noreturn __start_xen(unsigne
>>  
>>      dmi_scan_machine();
>>  
>> -    generic_apic_probe();
>> -
>>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>>                                    RANGESETF_prettyprint_hex);
>>  
>>      xsm_multiboot_init(module_map, mbi);
>>  
>> +    /*
>> +     * IOMMU-related ACPI table parsing may require some of the system domains
>> +     * to be usable.
> 
> I would be a bit more specific and likely add "...to be usable in
> order to hide IOMMU PCI devices.".

Hmm, not sure. I did specifically leave out the "why" part, as dom_io
might also become required for something. If you nevertheless think
I should extend the comment, then I'd insist on using "e.g." just
like I did in the comment next to the call to generic_apic_probe().

Jan

Re: [PATCH v2.2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing

[Roger Pau Monné]

On Mon, Nov 15, 2021 at 05:10:04PM +0100, Jan Beulich wrote:
> On 15.11.2021 16:07, Roger Pau Monné wrote:
> > On Mon, Nov 15, 2021 at 03:31:39PM +0100, Jan Beulich wrote:
> >> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> >> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> >> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> >> affected: The call needs to occur after acpi_iommu_init(), such that
> >> iommu_intremap getting disabled there can be properly taken into account
> >> by apic_x2apic_probe().
> >>
> >> Note that, for the time being (further cleanup patches following),
> >> reversing the order of the calls to generic_apic_probe() and
> >> acpi_boot_init() is not an option:
> >> - acpi_process_madt() calls clustered_apic_check() and hence relies on
> >>   genapic to have got filled before,
> >> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
> >>   occur after generic_apic_probe(),
> >> - acpi_parse_madt() (called from acpi_process_madt()) calls
> >>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -1700,15 +1700,30 @@ void __init noreturn __start_xen(unsigne
> >>  
> >>      dmi_scan_machine();
> >>  
> >> -    generic_apic_probe();
> >> -
> >>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
> >>                                    RANGESETF_prettyprint_hex);
> >>  
> >>      xsm_multiboot_init(module_map, mbi);
> >>  
> >> +    /*
> >> +     * IOMMU-related ACPI table parsing may require some of the system domains
> >> +     * to be usable.
> > 
> > I would be a bit more specific and likely add "...to be usable in
> > order to hide IOMMU PCI devices.".
> 
> Hmm, not sure. I did specifically leave out the "why" part, as dom_io
> might also become required for something. If you nevertheless think
> I should extend the comment, then I'd insist on using "e.g." just
> like I did in the comment next to the call to generic_apic_probe().

My request was mostly to make it clear where dom_io is used, it's IMO
not trivial to spot that pci_ro_device requires the dom_io to be
setup. I'm fine if you want to use 'e.g.', or if you think this is not
helpful.

Thanks, Roger.

Re: [PATCH v2 0/6] x86: ACPI / APIC / IOMMU interaction

[Andrew Cooper]

On 05/11/2021 12:30, Jan Beulich wrote:
> While reworking patch 1 (the only one here that I consider a candidate
> for 4.16; see there) I stumbled over quite a few things that have long
> been ripe for cleaning up. Hence the tail of 5 further patches ...
>
> 1: x2APIC: defer probe until after IOMMU ACPI table parsing
> 2: APIC: drop clustered_apic_check() hook
> 3: APIC: drop {acpi_madt,mps}_oem_check() hooks
> 4: APIC: drop probe_default()
> 5: APIC: rename cmdline_apic
> 6: ACPI: drop dead interpreter-related code

Patches 2 thru 6 clean up an area I'd recently stumbled upon.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Since they clean up a pile of function pointers, I've pulled them into 
x86-next.


Patch 1 is looking very risky at this point, especially given the late 
correction, and I think it would be wise to wait for 4.17.

~Andrew

Re: [PATCH v2 0/6] x86: ACPI / APIC / IOMMU interaction

[Jan Beulich]

On 16.11.2021 13:09, Andrew Cooper wrote:
> On 05/11/2021 12:30, Jan Beulich wrote:
>> While reworking patch 1 (the only one here that I consider a candidate
>> for 4.16; see there) I stumbled over quite a few things that have long
>> been ripe for cleaning up. Hence the tail of 5 further patches ...
>>
>> 1: x2APIC: defer probe until after IOMMU ACPI table parsing
>> 2: APIC: drop clustered_apic_check() hook
>> 3: APIC: drop {acpi_madt,mps}_oem_check() hooks
>> 4: APIC: drop probe_default()
>> 5: APIC: rename cmdline_apic
>> 6: ACPI: drop dead interpreter-related code
> 
> Patches 2 thru 6 clean up an area I'd recently stumbled upon.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> Patch 1 is looking very risky at this point, especially given the late 
> correction, and I think it would be wise to wait for 4.17.

Well, that's also been my takeaway from Ian's responses so far. I did
put a question mark there because I wanted it to be considered without
meaning to actually push for it.

Jan

Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing [and 2 more messages]

[Ian Jackson]

I wrote:
> Could we not do a smaller fix that would print something in the boot
> output, mabye ?  That would be a lower risk change.
> 
> So far, I think the tradeoff here isn't looking very good: a risk of
> unclear magnitude for many users, vs a hard crash at boot for a set of
> users we believe to be empty.
> 
> As ever, feel free to contradict me if I have the wrong end of one of
> the many sticks here...

On the question of backporting:

Jan Beulich:
> >Ian Jackson:
> >>>       Nevertheless it will then be a backporting candidate, so
> >>>       considering to take it right away can't simply be put off.
> > 
> > This part confused me.  Under what circumstances would we backport
> > this ?  AIUI it would be backporting a potentially-fragile and
> > not-readily-testable bugfix, for a theoretical scenario with a
> > straightforward workaround.
> 
> Well, I've said "candidate" for this very reason: To me, every bug
> fix is a candidate. Whether risks outweigh the potential benefits is
> then influencing whether to _actually_ take the change. A reason to
> take it despite the available workaround might be that
> "straightforward" doesn't also mean "obvious" here. IOW once you
> know what to do, doing so is easy. But one first needs to arrive
> there.

I didn't find this particularly convincing, TBH.

Despite the above exchanges, this patch is still marked 4.16? and the
release discussion inthe patch does not seem to have bene updated to
take into account this discussion.

In any case, I think at this stage of the freeze this patch is not a
good bet, particularly seeing as it has had to have another round.

Thanks,
Ian.