Re: [patch 0/4] x86: PAT followup - Incremental changes and bug fixes

[Ingo Molnar <mingo@elte.hu>]

* Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:

> On Thu, Jan 17, 2008 at 10:13:08PM +0100, Ingo Molnar wrote:
> > but in general we must be robust enough in this case and just degrade 
> > any overlapping page to UC (and emit a warning perhaps) - instead of 
> > failing the ioremap and thus failing the driver (and the bootup).
> 
> But then, this will cause an attribute conflicit. Old one was 
> specifying WB in PAT (ioremap with noflags) and the new ioremap 
> specifies UC.

we could fix up all aliases of that page as well and degrade them to UC?

> As Linus mentioned, main problem is to figure out the correct 
> attribute for ioremap() which doesn't specify the actual attribute to 
> be used.

i think the problem is the proximity of some ACPI tables to actual 
device mmio areas - they share the same physical page. The ACPI tables 
will be mapped WB, the device mmio areas will be UC most of the time.

> One mechanism to fix the issue generically (somewhat atleast) is to 
> use MTRR's and figure out the default MTRR attribute for that physical 
> address and use it for ioremap().

how would this solve the problem at hand? I dont think it's possible to 
guarantee that all the BIOS data pages and mmio areas will have 
compatible attributes. BIOS data pages might be in plain RAM that we 
intend to map WB. Or they might be in reserved areas near the mmio 
addresses.

but if we fixed up aliases (only for that single conflicting page), so 
that all mappings are degraded to UC, we'd have uniform behavior all 
across and the least amount of surprise to drivers. Hm?

> > but i have not seen this message in your boot log. Could you boot 
> > with early_ioremap_debug and send us the dmesg - i'm curious which 
> > ACPI tables are actively mapped while those devices are initialized.
> 
> In this scenario, ACPI is using ioremap() leaving some dangling 
> references. Venki is looking to fix this code. Getting the attribute 
> for MTRR for ioremap noflags, might solve some of these issues aswell. 
> Will look into this.

ok. Resolving that would be nice anyway because the ACPI table might be 
in plain RAM which might be reused by the kernel later on, etc. FYI, 
there's also the patch from Yinghai Lu on lkml, for one such dangling 
reference problem in the SRAT table.

	Ingo

---------------->
From: Yinghai Lu <Yinghai.Lu@Sun.COM>
Subject: [PATCH] x86: copy srat table and unmap in acpi_parse_table

[PATCH] x86: copy srat table and unmap in acpi_parse_table


the old acpi_numa_slit_init was saving old address in early stage acpi_slit
and acpi_parse_table can not unmap address that.
the patch copy the slit in the callback,
so we could unmap table in acpi_parse_table instead of outside track it.

need to revert
"
commit d8d28f25f33c6a035cdfb1d421c79293d16e5c58
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Jan 17 15:26:42 2008 +0100

    x86: ACPI: fix mapping leaks

    ioremap_early() is stateful, hence we cannot tolerate mapping leaks.
"

before appling this patch

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

Index: linux-2.6/arch/x86/mm/srat_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/srat_64.c
+++ linux-2.6/arch/x86/mm/srat_64.c
@@ -23,7 +23,9 @@
 
 int acpi_numa __initdata;
 
-static struct acpi_table_slit *acpi_slit;
+static int slit_copied;
+static u64 slit_locality_count;
+static u8 slit_entry[MAX_NUMNODES * MAX_NUMNODES];
 
 static nodemask_t nodes_parsed __initdata;
 static struct bootnode nodes[MAX_NUMNODES] __initdata;
@@ -130,7 +132,16 @@ void __init acpi_numa_slit_init(struct a
 		printk(KERN_INFO "ACPI: SLIT table looks invalid. Not used.\n");
 		return;
 	}
-	acpi_slit = slit;
+
+	if (slit->locality_count > MAX_NUMNODES)
+		return;
+
+	slit_locality_count = slit->locality_count;
+
+	memcpy(slit_entry, slit->entry,
+	       slit_locality_count * slit_locality_count);
+
+	slit_copied = 1;
 }
 
 /* Callback for Proximity Domain -> LAPIC mapping */
@@ -502,11 +513,11 @@ int __node_distance(int a, int b)
 {
 	int index;
 
-	if (!acpi_slit)
+	if (!slit_copied)
 		return null_slit_node_compare(a, b) ? LOCAL_DISTANCE :
 						      REMOTE_DISTANCE;
-	index = acpi_slit->locality_count * node_to_pxm(a);
-	return acpi_slit->entry[index + node_to_pxm(b)];
+	index = slit_locality_count * node_to_pxm(a);
+	return slit_entry[index + node_to_pxm(b)];
 }
 
 EXPORT_SYMBOL(__node_distance);
Index: linux-2.6/drivers/acpi/tables.c
===================================================================
--- linux-2.6.orig/drivers/acpi/tables.c
+++ linux-2.6/drivers/acpi/tables.c
@@ -260,6 +260,7 @@ int __init acpi_table_parse(char *id, ac
 
 	if (table) {
 		handler(table);
+		acpi_os_unmap_memory(table, table->length);
 		return 0;
 	} else
 		return 1;