[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.