Re: issue with patch "x86: no CPA on iounmap"

[Ingo Molnar <mingo@elte.hu>]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> Siddha, Suresh B wrote:
>> This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
>> 	"x86: no CPA on iounmap"
>>
>> This can use performance issue. When a GART driver unmaps a RAM page,
>
> thinking about this some more...
>
> afaik the gart driver doesn't use ioremap....
>
> (and it does caching control explicitly, and sets its pages back to 
> cached)

there are many GART drivers, and the method used depends on the GART 
driver. The following GART drivers still use ioremap in one way or 
another:

 drivers/char/agp/amd-k7-agp.c
 drivers/char/agp/ati-agp.c
 drivers/char/agp/generic.c
 drivers/char/agp/sworks-agp.c
 drivers/char/drm/radeon_cp.c

the method use is in all cases the same: they use __get_free_page() to 
pick up a general RAM page, they do SetPageReserved() and then they use 
ioremap_nocache() to map it non-cached, and then they also program the 
GART to access those pages.

when the GART code deinits, it does an iounmap() on those pages, unmaps 
it from the GART hardware itself, does a ClearPageReserved() and does 
__free_page() to put the page into the general page pool again. So 
Suresh is right: these pages are currently marked UC at this point and 
we need to mark them cacheable.

we could do this automatically in iounmap() upon seeing a page_is_ram() 
that has PageReserved set. Or we could stick in a set_memory_wb() into 
the deinit [and ioremap_nocache()-failure] sequence.

Since we treat PageReserved pages specially in ioremap() already [we 
allow them, despite them being listed in the e820 map], i think the more 
robust solution is to recognize them in iounmap() as well - this way it 
cannot be forgotten accidentally. (and UC pages in the buddy are _hard_ 
to notice after the fact) There is no aliasing danger i believe: IO bars 
should never be marked as general RAM in the e820.

	Ingo