Fix acpi_dmar_zap/reinstate() (fixes S3 regression)

PATCH Superseded Tomasz Wroblewski 2 participants 10 messages Jan 22, 2013 – Jan 23, 2013

lore

Share & tools

lore

Tomasz Wroblewski

author

2013-01-22 12:08 UTC #0 lore raw

PATCH 1/1 Fix acpi_dmar_zap/reinstate() (fixes S3 regression)

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 <[email protected]>

xen/drivers/acpi/osl.c +2 -2

diff -r b6195e277da5 xen/drivers/acpi/osl.c
@@ -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)
 {
 }

xen/drivers/acpi/tables/Makefile +2 -2

diff -r b6195e277da5 xen/drivers/acpi/tables/Makefile
@@ -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

xen/drivers/acpi/tables/tbinstal.c +1 -1

diff -r b6195e277da5 xen/drivers/acpi/tables/tbinstal.c
@@ -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;

xen/drivers/acpi/tables/tbutils.c +1 -1

diff -r b6195e277da5 xen/drivers/acpi/tables/tbutils.c
@@ -175,7 +175,7 @@
  *
  ******************************************************************************/
 
-acpi_status __init
+acpi_status
 acpi_tb_verify_checksum(struct acpi_table_header *table, u32 length)
 {
 	u8 checksum;

xen/drivers/acpi/tables/tbxface.c +1 -1

diff -r b6195e277da5 xen/drivers/acpi/tables/tbxface.c
@@ -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)
 {

xen/drivers/passthrough/vtd/dmar.c +15 -2

diff -r b6195e277da5 xen/drivers/passthrough/vtd/dmar.c
@@ -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';

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2013-01-22 12:58 UTC #1 lore raw

Quoted 18 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
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.

Quoted 4 lines

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

I recognize the need of fixing this, but not this way. We have ioremap() now, and hence the patch could be using this, without re-running the whole acpi_get_table(), but just using the stored physical address of the table (retrieving of which would be the only real code addition needed here).

For the older trees with non-functional ioremap(), I'd prefer simply adding the table range to the 1:1 mapping (thus making ioremap() work for that range, should use of that be needed; if not needed, that's certainly worth considering this even for -unstable).

Also, with your change not even attempting to fix the underlying issue of the ACPI code storing a pointer to the mapped table in acpi_gbl_root_table_list.tables[].pointer, I can't even see how your patch is supposed to work. I'd expect the stored pointer to get re-used by acpi_get_table()/acpi_tb_verify_table(), and hence this shouldn't win you anything.

Jan

Tomasz Wroblewski

author

2013-01-22 13:36 UTC #2 lore raw

On 22/01/13 13:58, Jan Beulich wrote:

Jan, thanks for your comments

Quoted 12 lines

I recognize the need of fixing this, but not this way. We have ioremap() now, and hence the patch could be using this, without re-running the whole acpi_get_table(), but just using the stored physical address of the table (retrieving of which would be the only real code addition needed here).
For the older trees with non-functional ioremap(), I'd prefer simply adding the table range to the 1:1 mapping (thus making ioremap() work for that range, should use of that be needed; if not needed, that's certainly worth considering this even for -unstable).

Quoted 7 lines

Also, with your change not even attempting to fix the underlying issue of the ACPI code storing a pointer to the mapped table in acpi_gbl_root_table_list.tables[].pointer, I can't even see how your patch is supposed to work. I'd expect the stored pointer to get re-used by acpi_get_table()/acpi_tb_verify_table(), and hence this shouldn't win you anything.

It works because the existing code in acpi_get_table actually sets acpi_gbl_root_table_list.tables[i].pointer = NULL every call anyway (after setting *out_table pointer).

I'll have a go at trying this with ioremap() and retrievable table's physical pointer instead.

Jan Beulich

2013-01-22 14:13 UTC #3 lore raw

Quoted 24 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
On 22/01/13 13:58, Jan Beulich wrote:
Jan, thanks for your comments
Quoted 12 lines
I recognize the need of fixing this, but not this way. We have ioremap() now, and hence the patch could be using this, without re-running the whole acpi_get_table(), but just using the stored physical address of the table (retrieving of which would be the only real code addition needed here).
For the older trees with non-functional ioremap(), I'd prefer simply adding the table range to the 1:1 mapping (thus making ioremap() work for that range, should use of that be needed; if not needed, that's certainly worth considering this even for -unstable).
Quoted 7 lines
Also, with your change not even attempting to fix the underlying issue of the ACPI code storing a pointer to the mapped table in acpi_gbl_root_table_list.tables[].pointer, I can't even see how your patch is supposed to work. I'd expect the stored pointer to get re-used by acpi_get_table()/acpi_tb_verify_table(), and hence this shouldn't win you anything.

Quoted 3 lines

It works because the existing code in acpi_get_table actually sets acpi_gbl_root_table_list.tables[i].pointer = NULL every call anyway (after setting *out_table pointer).

Oh, indeed, I had overlooked that.

Quoted 2 lines

I'll have a go at trying this with ioremap() and retrievable table's physical pointer instead.

Thanks.

Jan

Tomasz Wroblewski

author

2013-01-22 15:27 UTC #4 lore raw

In reply to Jan Beulich

On 22/01/13 13:58, Jan Beulich wrote:

Jan,

attaching updated patch which uses ioremap() instead of acpi_get_table. Tested it across some S3 iterations on xen tip from today and Lenovo T520 and seems to work well (although there are some unrelated scheduler issues on resume which we have patched separately on our end; intending to submit these patches as well later on). Doesn't work on older trees from before your "implement vmap()" commit. You mentioned that this would be fixable on older trees via adding the acpi table range to the 1:1 mapping - how would one go about this as I'm not sure where is the relevant code located?

Thanks!

xen/drivers/acpi/tables/tbutils.c +20 -0

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbutils.c
@@ -525,3 +525,23 @@
 
 	return_ACPI_STATUS(AE_OK);
 }
+
+acpi_status acpi_tb_get_table_physical_location(char *signature, acpi_physical_address *out_addr, u32 *out_size)
+{
+    acpi_native_uint i;
+
+    if (!signature || !out_addr || !out_size) {
+        return (AE_BAD_PARAMETER);
+    }
+
+    for (i = 0; i < acpi_gbl_root_table_list.count; i++) {
+        if (!ACPI_COMPARE_NAME(&(acpi_gbl_root_table_list.tables[i].signature), signature)) {
+            continue;
+        }
+        *out_addr = acpi_gbl_root_table_list.tables[i].address;
+        *out_size = acpi_gbl_root_table_list.tables[i].length;
+        return AE_OK;
+    }
+    return AE_NOT_FOUND;
+}
+

xen/drivers/passthrough/vtd/dmar.c +40 ~~-10~~

79 lines not on this page · raw message

xen/include/acpi/actables.h +2 -0

diff -r 4b476378fc35 xen/include/acpi/actables.h
@@ -61,6 +61,8 @@
 		   char *oem_id,
 		   char *oem_table_id, acpi_native_uint * table_index);
 
+acpi_status acpi_tb_get_table_physical_location(char *signature, acpi_physical_address *out_addr, u32 *out_size);
+
 /*
  * tbinstal - Table removal and deletion
  */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2013-01-22 15:55 UTC #5 lore raw

Quoted 2 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
attaching updated patch which uses ioremap() instead of acpi_get_table.

First of all, I envisioned to keep the static variable "dmar_table", and simply intialize it when the hypervisor boots. That, as a result, will move all of your new code additions to .init.text (which in turn will make it desirable to pick a different ACPI source file to add the new function in).

Second, adding a function to be called from outside ACPI to acpi/actables.h is wrong, which you could also have noticed by the fact that you had the add the inclusion of that header.

Along the same lines, publicly accessible table interfaces are expected to be in drivers/acpi/tables.c or drivers/acpi/tables/tbxface.c. Probably the latter is better suited.

Finally, when adding code to ACPI source files, please make sure you match the coding conventions found in those files, not the general Xen ones (and certainly not a mixture of both).

Quoted 8 lines

Tested it across some S3 iterations on xen tip from today and Lenovo T520 and seems to work well (although there are some unrelated scheduler issues on resume which we have patched separately on our end; intending to submit these patches as well later on). Doesn't work on older trees from before your "implement vmap()" commit. You mentioned that this would be fixable on older trees via adding the acpi table range to the 1:1 mapping - how would one go about this as I'm not sure where is the relevant code located?

That ought to be a simple call to map_pages_to_xen(), equivalent to the respective ones in __start_xen(). Perhaps something like

    map_pages_to_xen(__va(addr), PFN_DOWN(addr), <nr>, PAGE_HYPERVISOR);

You'd then simply set "dmar_table" to __va(addr).

Jan

Tomasz Wroblewski

author

2013-01-22 17:22 UTC #6 lore raw

PATCH v3 1/1 Fix acpi_dmar_zap/reinstate() (fixes S3 regression)

Thanks, attaching version 3 of the patch using your suggestions, which simplify it considerably. This one uses map_pages_to_xen in favour of ioremap, so it works on both tip as well as older trees from before ioremap() rework (tested both).

Changes from v2/v1:
- keeping global dmar_table pointer and initializing it in acpi_dmar_init()
- use map_pages_to_xen to add a static mapping (instead of ioremap)
- moved acpi_get_table_physical_location to tbxface.c. Added instance argument to make it symmetric with acpi_get_table.
- coding convention fixes

Commit message:

Fix S3 regression introduced by cs 23013:65d26504e843 (ACPI: large cleanup). The dmar virtual pointer returned from acpi_get_table cannot be safely stored away and used later, as the underlying acpi_os_map_memory / __acpi_map_table functions overwrite the mapping causing it to point to different tables than dmar (last fetched table is used). This subsequently causes acpi_dmar_reinstate() and acpi_dmar_zap() to write data to wrong table, causing its corruption and problems with consecutive s3 resumes.

Added a new function to fetch ACPI table physical address, and establishing separate static mapping for dmar_table pointer instead of using acpi_get_table().

Signed-off-by Tomasz Wroblewski <[email protected]>

xen/drivers/acpi/tables/tbxface.c +41 -0

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbxface.c
@@ -205,3 +205,44 @@
 
 	return (AE_NOT_FOUND);
 }
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_get_table_physical_location
+ *
+ * PARAMETERS:  Signature           - ACPI signature of needed table
+ *              Instance            - Which instance (for SSDTs)
+ *              out_addr            - Where the table's physical address is returned
+ *              out_len             - Where the length of table is returned
+ *
+ * RETURN:      Status, pointer and length of table
+ *
+ * DESCRIPTION: Finds physical address and length of ACPI table
+ *
+ *****************************************************************************/
+acpi_status __init
+acpi_get_table_physical_location(char *signature,
+				 acpi_native_uint instance,
+				 acpi_physical_address *out_addr, u32 *out_len)
+{
+	acpi_native_uint i;
+	acpi_native_uint j;
+
+	if (!signature || !out_addr || !out_len) {
+		return (AE_BAD_PARAMETER);
+	}
+
+	for (i = 0, j=0; i < acpi_gbl_root_table_list.count; i++) {
+		if (!ACPI_COMPARE_NAME(&(acpi_gbl_root_table_list.tables[i].signature), signature)) {
+			continue;
+		}
+		if (++j < instance) {
+			continue;
+		}
+		*out_addr = acpi_gbl_root_table_list.tables[i].address;
+		*out_len = acpi_gbl_root_table_list.tables[i].length;
+		return AE_OK;
+	}
+
+	return AE_NOT_FOUND;
+}

xen/drivers/passthrough/vtd/dmar.c +11 -1

diff -r 4b476378fc35 xen/drivers/passthrough/vtd/dmar.c
@@ -823,7 +823,17 @@
 
 int __init acpi_dmar_init(void)
 {
-    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
+    acpi_physical_address dmar_addr;
+    u32 dmar_len;
+    if ( !acpi_get_table_physical_location(
+             ACPI_SIG_DMAR, 0, &dmar_addr, &dmar_len) )
+    {
+        map_pages_to_xen((unsigned long)__va(dmar_addr), PFN_DOWN(dmar_addr),
+                         PFN_UP(dmar_addr) - PFN_DOWN(dmar_addr) + 1,
+                         PAGE_HYPERVISOR);
+        dmar_table = (struct acpi_table_header*) __va(dmar_addr);
+    }
+
     return parse_dmar_table(acpi_parse_dmar);
 }

xen/include/acpi/acpixf.h +4 -0

diff -r 4b476378fc35 xen/include/acpi/acpixf.h
@@ -77,6 +77,10 @@
 acpi_get_table(acpi_string signature,
 	       acpi_native_uint instance, struct acpi_table_header **out_table);
 
+acpi_status
+acpi_get_table_physical_location(char *signature,
+				 acpi_native_uint instance,
+				 acpi_physical_address *out_addr, u32 *out_len);
 /*
  * Namespace and name interfaces
  */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2013-01-23 08:47 UTC #7 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Thanks, attaching version 3 of the patch using your suggestions, which simplify it considerably. This one uses map_pages_to_xen in favour of ioremap, so it works on both tip as well as older trees from before ioremap() rework (tested both).

Quoted 6 lines

Changes from v2/v1:
- keeping global dmar_table pointer and initializing it in acpi_dmar_init()
- use map_pages_to_xen to add a static mapping (instead of ioremap)
- moved acpi_get_table_physical_location to tbxface.c. Added instance argument to make it symmetric with acpi_get_table.
- coding convention fixes

This looks much better; I'll slightly edit it before committing though, so please double check the result.

Jan

Quoted 16 lines

Commit message:
Fix S3 regression introduced by cs 23013:65d26504e843 (ACPI: large cleanup). The dmar virtual pointer returned from acpi_get_table cannot be safely stored away and used later, as the underlying acpi_os_map_memory / __acpi_map_table functions overwrite the mapping causing it to point to different tables than dmar (last fetched table is used). This subsequently causes acpi_dmar_reinstate() and acpi_dmar_zap() to write data to wrong table, causing its corruption and problems with consecutive s3 resumes.
Added a new function to fetch ACPI table physical address, and establishing separate static mapping for dmar_table pointer instead of using acpi_get_table().
Signed-off-by Tomasz Wroblewski <[email protected]>

Tomasz Wroblewski

author

2013-01-23 09:02 UTC #8 lore raw

PATCH v4 1/1 Fix acpi_dmar_zap/reinstate() (fixes S3 regression)

Quoted 4 lines · Jan Beulich

This looks much better; I'll slightly edit it before committing though, so please double check the result.

I just noticed I made a mistake calculating the number of pages to map, which should be just PFN_UP(dmar_len) (although in practice both calculations ten to yield 1 page anyway); but resubmiting v4 with that fixed

Signed-off-by Tomasz Wroblewski <[email protected]>

xen/drivers/acpi/tables/tbxface.c +41 -0

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbxface.c
@@ -205,3 +205,44 @@
 
 	return (AE_NOT_FOUND);
 }
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_get_table_physical_location
+ *
+ * PARAMETERS:  Signature           - ACPI signature of needed table
+ *              Instance            - Which instance (for SSDTs)
+ *              out_addr            - Where the table's physical address is returned
+ *              out_len             - Where the length of table is returned
+ *
+ * RETURN:      Status, pointer and length of table
+ *
+ * DESCRIPTION: Finds physical address and length of ACPI table
+ *
+ *****************************************************************************/
+acpi_status __init
+acpi_get_table_physical_location(char *signature,
+				 acpi_native_uint instance,
+				 acpi_physical_address *out_addr, u32 *out_len)
+{
+	acpi_native_uint i;
+	acpi_native_uint j;
+
+	if (!signature || !out_addr || !out_len) {
+		return (AE_BAD_PARAMETER);
+	}
+
+	for (i = 0, j=0; i < acpi_gbl_root_table_list.count; i++) {
+		if (!ACPI_COMPARE_NAME(&(acpi_gbl_root_table_list.tables[i].signature), signature)) {
+			continue;
+		}
+		if (++j < instance) {
+			continue;
+		}
+		*out_addr = acpi_gbl_root_table_list.tables[i].address;
+		*out_len = acpi_gbl_root_table_list.tables[i].length;
+		return AE_OK;
+	}
+
+	return AE_NOT_FOUND;
+}

xen/drivers/passthrough/vtd/dmar.c +11 -1

diff -r 4b476378fc35 xen/drivers/passthrough/vtd/dmar.c
@@ -823,7 +823,17 @@
 
 int __init acpi_dmar_init(void)
 {
-    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
+    acpi_physical_address dmar_addr;
+    u32 dmar_len;
+    if ( !acpi_get_table_physical_location(
+             ACPI_SIG_DMAR, 0, &dmar_addr, &dmar_len) )
+    {
+        map_pages_to_xen((unsigned long)__va(dmar_addr), PFN_DOWN(dmar_addr),
+                         PFN_UP(dmar_len),
+                         PAGE_HYPERVISOR);
+        dmar_table = (struct acpi_table_header*) __va(dmar_addr);
+    }
+
     return parse_dmar_table(acpi_parse_dmar);
 }

xen/include/acpi/acpixf.h +4 -0

diff -r 4b476378fc35 xen/include/acpi/acpixf.h
@@ -77,6 +77,10 @@
 acpi_get_table(acpi_string signature,
 	       acpi_native_uint instance, struct acpi_table_header **out_table);
 
+acpi_status
+acpi_get_table_physical_location(char *signature,
+				 acpi_native_uint instance,
+				 acpi_physical_address *out_addr, u32 *out_len);
 /*
  * Namespace and name interfaces
  */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2013-01-23 09:26 UTC #9 lore raw

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 4 lines · Jan Beulich
This looks much better; I'll slightly edit it before committing though, so please double check the result.
I just noticed I made a mistake calculating the number of pages to map, which should be just PFN_UP(dmar_len) (although in practice both calculations ten to yield 1 page anyway); but resubmiting v4 with that fixed

I fixed this already (and properly - the way you did it doesn't account for the table crossing a page boundary) as part of the indicated editing I would do.

Thanks anyway, Jan

Quoted 1 line

Signed-off-by Tomasz Wroblewski <[email protected]>