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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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


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