[PATCH] Fix acpi_dmar_zap/reinstate() (fixes S3 regression)

[Tomasz Wroblewski <tomasz.wroblewski@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

Hello all,

Changeset  23013:65d26504e843 (ACPI: large cleanup) modified 
acpi_dmar_reinstate() and acpi_dmar_zap() to use global "dmar_table" 
pointer instead of fetching it dynamically via acpi_get_table (and it 
removed the get_dmar() function which was used to this). This global 
pointer is initialised once when parsing the acpi table.

However, this seems incorrect due to how acpi_get_table() and underlying 
__acpi_map_table() is implemented. It pretty much always returns the 
same virtual pointer but instead remaps the fixmap underlying that 
virtual pointer. So storing this virtual pointer in global variable is 
incorrect because the pointer is invalidated next time acpi_get_table() 
is called.  Therefore acpi_dmar_reinstate()/zap() actually modify not 
the DMAR table but the table which was last accessed by acpi_get_table 
(which happens to not be DMAR at least on some Lenovos we tested but 
likely more platforms). This causes the affected table corruption, its 
checksum corruption, and failure to resume from S3 second consecutive time.

Attached patch restores the previous behaviour before cset 
23013:65d26504e843 of fetching the dmar_table pointer dynamically. Some 
__init annotations needed to be removed as the acpi_get_table() function 
is now again used on suspend/resume path

Signed-off-by: Tomasz Wroblewski <tomasz.wroblewski@citrix.com>




[-- Attachment #2: fix-dmar-zap-reinstate.patch --]
[-- Type: text/x-patch, Size: 3924 bytes --]

diff -r b6195e277da5 xen/drivers/acpi/osl.c
--- a/xen/drivers/acpi/osl.c	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/acpi/osl.c	Tue Jan 22 11:07:49 2013 +0000
@@ -83,13 +83,13 @@
 	}
 }
 
-void __iomem *__init
+void __iomem *
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
 	return __acpi_map_table((unsigned long)phys, size);
 }
 
-void __init acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
+void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
 {
 }
 
diff -r b6195e277da5 xen/drivers/acpi/tables/Makefile
--- a/xen/drivers/acpi/tables/Makefile	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/acpi/tables/Makefile	Tue Jan 22 11:07:49 2013 +0000
@@ -1,5 +1,5 @@
 obj-bin-y += tbfadt.init.o
-obj-bin-y += tbinstal.init.o
+obj-y += tbinstal.o
 obj-y += tbutils.o
-obj-bin-y += tbxface.init.o
+obj-y += tbxface.o
 obj-bin-y += tbxfroot.init.o
diff -r b6195e277da5 xen/drivers/acpi/tables/tbinstal.c
--- a/xen/drivers/acpi/tables/tbinstal.c	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/acpi/tables/tbinstal.c	Tue Jan 22 11:07:49 2013 +0000
@@ -59,7 +59,7 @@
  * DESCRIPTION: this function is called to verify and map table
  *
  *****************************************************************************/
-acpi_status __init acpi_tb_verify_table(struct acpi_table_desc *table_desc)
+acpi_status acpi_tb_verify_table(struct acpi_table_desc *table_desc)
 {
 	acpi_status status = AE_OK;
 
diff -r b6195e277da5 xen/drivers/acpi/tables/tbutils.c
--- a/xen/drivers/acpi/tables/tbutils.c	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/acpi/tables/tbutils.c	Tue Jan 22 11:07:49 2013 +0000
@@ -175,7 +175,7 @@
  *
  ******************************************************************************/
 
-acpi_status __init
+acpi_status
 acpi_tb_verify_checksum(struct acpi_table_header *table, u32 length)
 {
 	u8 checksum;
diff -r b6195e277da5 xen/drivers/acpi/tables/tbxface.c
--- a/xen/drivers/acpi/tables/tbxface.c	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/acpi/tables/tbxface.c	Tue Jan 22 11:07:49 2013 +0000
@@ -164,7 +164,7 @@
  * DESCRIPTION: Finds and verifies an ACPI table.
  *
  *****************************************************************************/
-acpi_status __init
+acpi_status
 acpi_get_table(char *signature,
 	       acpi_native_uint instance, struct acpi_table_header **out_table)
 {
diff -r b6195e277da5 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Wed Jan 16 14:15:44 2013 +0000
+++ b/xen/drivers/passthrough/vtd/dmar.c	Tue Jan 22 11:07:49 2013 +0000
@@ -46,7 +46,6 @@
 static LIST_HEAD_READ_MOSTLY(acpi_atsr_units);
 static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
 
-static struct acpi_table_header *__read_mostly dmar_table;
 static int __read_mostly dmar_flags;
 static u64 __read_mostly igd_drhd_address;
 
@@ -821,14 +820,27 @@
 /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */
 #define parse_dmar_table(h) tboot_parse_dmar_table(h)
 
+static struct acpi_table_header *get_dmar(void)
+{
+    struct acpi_table_header *dmar_table = NULL;
+    unsigned long flags;
+
+    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
+    local_irq_save(flags);
+    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
+    local_irq_restore(flags);
+
+    return dmar_table;
+}
+
 int __init acpi_dmar_init(void)
 {
-    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
     return parse_dmar_table(acpi_parse_dmar);
 }
 
 void acpi_dmar_reinstate(void)
 {
+    struct acpi_table_header *dmar_table = get_dmar(); /* needs to be dynamically fetched here as acpi_get_table reuses the returned virtual address */
     if ( dmar_table == NULL )
         return;
     dmar_table->signature[0] = 'D';
@@ -837,6 +849,7 @@
 
 void acpi_dmar_zap(void)
 {
+    struct acpi_table_header *dmar_table = get_dmar();
     if ( dmar_table == NULL )
         return;
     dmar_table->signature[0] = 'X';

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel