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

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

[Tomasz Wroblewski]

[-- 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

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

[Jan Beulich]

>>> On 22.01.13 at 13:08, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> 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

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

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

[Tomasz Wroblewski]

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

Jan, thanks for your comments

> 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.
>    
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.

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

[Jan Beulich]

>>> On 22.01.13 at 14:36, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> On 22/01/13 13:58, Jan Beulich wrote:
> 
> Jan, thanks for your comments
> 
>> 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.
>>    
> 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.

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

Thanks.

Jan

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

[Tomasz Wroblewski]

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

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!



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

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbutils.c
--- a/xen/drivers/acpi/tables/tbutils.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/acpi/tables/tbutils.c	Tue Jan 22 15:13:43 2013 +0000
@@ -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;
+}
+
diff -r 4b476378fc35 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/passthrough/vtd/dmar.c	Tue Jan 22 15:13:43 2013 +0000
@@ -29,6 +29,7 @@
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <asm/string.h>
+#include <acpi/actables.h>
 #include "dmar.h"
 #include "iommu.h"
 #include "extern.h"
@@ -46,7 +47,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,26 +821,56 @@
 /* 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 *map_dmar(void)
+{
+    acpi_physical_address addr;
+    u32 sz;
+
+    if (acpi_tb_get_table_physical_location(ACPI_SIG_DMAR, &addr, &sz)) {
+        return NULL;
+    }
+    return (struct acpi_table_header*) ioremap(addr, sz);
+}
+
+static void unmap_dmar(struct acpi_table_header *dmar_table)
+{
+    iounmap(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)
 {
-    if ( dmar_table == NULL )
-        return;
-    dmar_table->signature[0] = 'D';
-    dmar_table->checksum += 'X'-'D';
+    unsigned long flags;
+    struct acpi_table_header *dmar_table;
+    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
+    local_irq_save(flags);
+    dmar_table = map_dmar(); /* needs to be dynamically fetched here as acpi_get_table reuses the returned virtual address */
+    if (dmar_table) {
+        dmar_table->signature[0] = 'D';
+        dmar_table->checksum += 'X'-'D';
+        unmap_dmar(dmar_table);
+    }
+    local_irq_restore(flags);
 }
 
 void acpi_dmar_zap(void)
 {
-    if ( dmar_table == NULL )
-        return;
-    dmar_table->signature[0] = 'X';
-    dmar_table->checksum -= 'X'-'D';
+    unsigned long flags;
+    struct acpi_table_header *dmar_table;
+    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
+    local_irq_save(flags);
+    dmar_table = map_dmar();
+    if (dmar_table) {
+        dmar_table->signature[0] = 'X';
+        dmar_table->checksum -= 'X'-'D';
+        unmap_dmar(dmar_table);
+    }
+    local_irq_restore(flags);
+
 }
 
 int platform_supports_intremap(void)
diff -r 4b476378fc35 xen/include/acpi/actables.h
--- a/xen/include/acpi/actables.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/acpi/actables.h	Tue Jan 22 15:13:43 2013 +0000
@@ -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
  */

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

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

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

[Jan Beulich]

>>> On 22.01.13 at 16:27, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> 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).

> 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

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

[Tomasz Wroblewski]

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

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 <tomasz.wroblewski@citrix.com>


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

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbxface.c
--- a/xen/drivers/acpi/tables/tbxface.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/acpi/tables/tbxface.c	Tue Jan 22 17:13:06 2013 +0000
@@ -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;
+}
diff -r 4b476378fc35 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/passthrough/vtd/dmar.c	Tue Jan 22 17:13:06 2013 +0000
@@ -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);
 }
 
diff -r 4b476378fc35 xen/include/acpi/acpixf.h
--- a/xen/include/acpi/acpixf.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/acpi/acpixf.h	Tue Jan 22 17:13:06 2013 +0000
@@ -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
  */

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

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

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

[Jan Beulich]

>>> On 22.01.13 at 18:22, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> 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

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

Jan

> 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 <tomasz.wroblewski@citrix.com>

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

[Tomasz Wroblewski]

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

On 23/01/13 09:47, Jan Beulich wrote:
> 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 <tomasz.wroblewski@citrix.com>


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

diff -r 4b476378fc35 xen/drivers/acpi/tables/tbxface.c
--- a/xen/drivers/acpi/tables/tbxface.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/acpi/tables/tbxface.c	Wed Jan 23 08:58:43 2013 +0000
@@ -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;
+}
diff -r 4b476378fc35 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/drivers/passthrough/vtd/dmar.c	Wed Jan 23 08:58:43 2013 +0000
@@ -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);
 }
 
diff -r 4b476378fc35 xen/include/acpi/acpixf.h
--- a/xen/include/acpi/acpixf.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/acpi/acpixf.h	Wed Jan 23 08:58:43 2013 +0000
@@ -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
  */

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

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

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

[Jan Beulich]

>>> On 23.01.13 at 10:02, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> On 23/01/13 09:47, Jan Beulich wrote:
>> 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

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