[PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Thomas Hellstrom]

Bug #13884
The code was incorrectly reserving memtypes using the page
virtual address instead of the physical address. Furthermore,
the code was not ignoring highmem pages as it ought to.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 arch/x86/mm/pageattr.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index d4327db..96dd4f6 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -590,9 +590,12 @@ static int __change_page_attr(struct cpa_data *cpa, int primary)
 	unsigned int level;
 	pte_t *kpte, old_pte;
 
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		address = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		address = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		address = cpa->vaddr[cpa->curpage];
 	else
 		address = *cpa->vaddr;
@@ -696,9 +699,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		vaddr = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		vaddr = cpa->vaddr[cpa->curpage];
 	else
 		vaddr = *cpa->vaddr;
@@ -1118,7 +1124,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 	int free_idx;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
 			goto err_out;
@@ -1131,7 +1139,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 err_out:
 	free_idx = i;
 	for (i = 0; i < free_idx; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}
@@ -1160,7 +1170,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
 		return retval;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}
-- 
1.5.4.3

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Ingo Molnar]

* Thomas Hellstrom <thellstrom@vmware.com> wrote:

> Bug #13884
> The code was incorrectly reserving memtypes using the page
> virtual address instead of the physical address. Furthermore,
> the code was not ignoring highmem pages as it ought to.
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  arch/x86/mm/pageattr.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> index d4327db..96dd4f6 100644
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -590,9 +590,12 @@ static int __change_page_attr(struct cpa_data *cpa, int primary)
>  	unsigned int level;
>  	pte_t *kpte, old_pte;
>  
> -	if (cpa->flags & CPA_PAGES_ARRAY)
> -		address = (unsigned long)page_address(cpa->pages[cpa->curpage]);
> -	else if (cpa->flags & CPA_ARRAY)
> +	if (cpa->flags & CPA_PAGES_ARRAY) {
> +		struct page *page = cpa->pages[cpa->curpage];
> +		if (unlikely(PageHighMem(page)))
> +			return 0;
> +		address = (unsigned long)page_address(page);
> +	} else if (cpa->flags & CPA_ARRAY)
>  		address = cpa->vaddr[cpa->curpage];

hm, i'm missing a description about how this bug was 
triggered. How did you end up getting highmem pages to a cpa 
call?

>  	else
>  		address = *cpa->vaddr;
> @@ -696,9 +699,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
>  	 * No need to redo, when the primary call touched the direct
>  	 * mapping already:
>  	 */
> -	if (cpa->flags & CPA_PAGES_ARRAY)
> -		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
> -	else if (cpa->flags & CPA_ARRAY)
> +	if (cpa->flags & CPA_PAGES_ARRAY) {
> +		struct page *page = cpa->pages[cpa->curpage];
> +		if (unlikely(PageHighMem(page)))
> +			return 0;
> +		vaddr = (unsigned long)page_address(page);
> +	} else if (cpa->flags & CPA_ARRAY)
>  		vaddr = cpa->vaddr[cpa->curpage];
>  	else
>  		vaddr = *cpa->vaddr;
> @@ -1118,7 +1124,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>  	int free_idx;
>  
>  	for (i = 0; i < addrinarray; i++) {
> -		start = (unsigned long)page_address(pages[i]);
> +		if (PageHighMem(pages[i]))
> +			continue;
> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>  		end = start + PAGE_SIZE;
>  		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
>  			goto err_out;

ok, that's a bug introduced in .29 but which was latent until now: 
drivers/char/agp/generic.c now uses it plus (indirectly) a number of 
AGP drivers, since:

 commit 07613ba2f464f59949266f4337b75b91eb610795
 Author: Dave Airlie <airlied@redhat.com>
 Date:   Fri Jun 12 14:11:41 2009 +1000

     agp: switch AGP to use page array instead of unsigned long array

I dont see how it can end up with highmem pages though. All 
the graphics apperture allocations happen to lowmem AFAICS. 
Did GEM add the possibility for user pages (highmem amongst 
them) ending up in that pool? Which code does that?

> @@ -1131,7 +1139,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>  err_out:
>  	free_idx = i;
>  	for (i = 0; i < free_idx; i++) {
> -		start = (unsigned long)page_address(pages[i]);
> +		if (PageHighMem(pages[i]))
> +			continue;
> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>  		end = start + PAGE_SIZE;
>  		free_memtype(start, end);
>  	}
> @@ -1160,7 +1170,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
>  		return retval;
>  
>  	for (i = 0; i < addrinarray; i++) {
> -		start = (unsigned long)page_address(pages[i]);
> +		if (PageHighMem(pages[i]))
> +			continue;
> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>  		end = start + PAGE_SIZE;
>  		free_memtype(start, end);

In any case it's a must-have fix for .31. Possibly even a backport 
tag is needed, in case a distro does a .30 kernel with more recent 
graphics bits.

	Ingo

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Dave Airlie]

> hm, i'm missing a description about how this bug was 
> triggered. How did you end up getting highmem pages to a cpa 
> call?

GEM and TTM both allocate page arrays and just pass them to cpa,
we don't know what type of pages the allocator gives us back and we really 
shouldn't have to, so having cpa ignore highmem pages is certainly the 
right option.

GEM just uses shmem code to alloc the pages and TTM has its own allocator.

Dave.

 > 
> >  	else
> >  		address = *cpa->vaddr;
> > @@ -696,9 +699,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
> >  	 * No need to redo, when the primary call touched the direct
> >  	 * mapping already:
> >  	 */
> > -	if (cpa->flags & CPA_PAGES_ARRAY)
> > -		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
> > -	else if (cpa->flags & CPA_ARRAY)
> > +	if (cpa->flags & CPA_PAGES_ARRAY) {
> > +		struct page *page = cpa->pages[cpa->curpage];
> > +		if (unlikely(PageHighMem(page)))
> > +			return 0;
> > +		vaddr = (unsigned long)page_address(page);
> > +	} else if (cpa->flags & CPA_ARRAY)
> >  		vaddr = cpa->vaddr[cpa->curpage];
> >  	else
> >  		vaddr = *cpa->vaddr;
> > @@ -1118,7 +1124,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
> >  	int free_idx;
> >  
> >  	for (i = 0; i < addrinarray; i++) {
> > -		start = (unsigned long)page_address(pages[i]);
> > +		if (PageHighMem(pages[i]))
> > +			continue;
> > +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
> >  		end = start + PAGE_SIZE;
> >  		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
> >  			goto err_out;
> 
> ok, that's a bug introduced in .29 but which was latent until now: 
> drivers/char/agp/generic.c now uses it plus (indirectly) a number of 
> AGP drivers, since:
> 
>  commit 07613ba2f464f59949266f4337b75b91eb610795
>  Author: Dave Airlie <airlied@redhat.com>
>  Date:   Fri Jun 12 14:11:41 2009 +1000
> 
>      agp: switch AGP to use page array instead of unsigned long array
> 
> I dont see how it can end up with highmem pages though. All 
> the graphics apperture allocations happen to lowmem AFAICS. 
> Did GEM add the possibility for user pages (highmem amongst 
> them) ending up in that pool? Which code does that?
> 
> > @@ -1131,7 +1139,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
> >  err_out:
> >  	free_idx = i;
> >  	for (i = 0; i < free_idx; i++) {
> > -		start = (unsigned long)page_address(pages[i]);
> > +		if (PageHighMem(pages[i]))
> > +			continue;
> > +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
> >  		end = start + PAGE_SIZE;
> >  		free_memtype(start, end);
> >  	}
> > @@ -1160,7 +1170,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
> >  		return retval;
> >  
> >  	for (i = 0; i < addrinarray; i++) {
> > -		start = (unsigned long)page_address(pages[i]);
> > +		if (PageHighMem(pages[i]))
> > +			continue;
> > +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
> >  		end = start + PAGE_SIZE;
> >  		free_memtype(start, end);
> 
> In any case it's a must-have fix for .31. Possibly even a backport 
> tag is needed, in case a distro does a .30 kernel with more recent 
> graphics bits.
> 
> 	Ingo
> 
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
> trial. Simplify your report design, integration and deployment - and focus on 
> what you do best, core application coding. Discover what's new with 
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> --
> _______________________________________________
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
> 
> 

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Thomas Hellstrom]

Dave Airlie wrote:
>> hm, i'm missing a description about how this bug was 
>> triggered. How did you end up getting highmem pages to a cpa 
>> call?
>>     
>
> GEM and TTM both allocate page arrays and just pass them to cpa,
> we don't know what type of pages the allocator gives us back and we really 
> shouldn't have to, so having cpa ignore highmem pages is certainly the 
> right option.
>
> GEM just uses shmem code to alloc the pages and TTM has its own allocator.
>
>   
Yes, Dave is right.

Although I'm not 100% sure the TTM code I was using that triggered this 
has made it into 2.6.31.
Old AGP uses (GFP_KERNEL | GFP_DMA32 | __GFP_ZERO), which (correct me if 
I'm wrong) never hands back highmem pages. This means that Intel's GEM 
is the only likely user for 2.6.31.

/Thomas

> Dave.
>
>  > 
>   
>>>  	else
>>>  		address = *cpa->vaddr;
>>> @@ -696,9 +699,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
>>>  	 * No need to redo, when the primary call touched the direct
>>>  	 * mapping already:
>>>  	 */
>>> -	if (cpa->flags & CPA_PAGES_ARRAY)
>>> -		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
>>> -	else if (cpa->flags & CPA_ARRAY)
>>> +	if (cpa->flags & CPA_PAGES_ARRAY) {
>>> +		struct page *page = cpa->pages[cpa->curpage];
>>> +		if (unlikely(PageHighMem(page)))
>>> +			return 0;
>>> +		vaddr = (unsigned long)page_address(page);
>>> +	} else if (cpa->flags & CPA_ARRAY)
>>>  		vaddr = cpa->vaddr[cpa->curpage];
>>>  	else
>>>  		vaddr = *cpa->vaddr;
>>> @@ -1118,7 +1124,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>>>  	int free_idx;
>>>  
>>>  	for (i = 0; i < addrinarray; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
>>>  			goto err_out;
>>>       
>> ok, that's a bug introduced in .29 but which was latent until now: 
>> drivers/char/agp/generic.c now uses it plus (indirectly) a number of 
>> AGP drivers, since:
>>
>>  commit 07613ba2f464f59949266f4337b75b91eb610795
>>  Author: Dave Airlie <airlied@redhat.com>
>>  Date:   Fri Jun 12 14:11:41 2009 +1000
>>
>>      agp: switch AGP to use page array instead of unsigned long array
>>
>> I dont see how it can end up with highmem pages though. All 
>> the graphics apperture allocations happen to lowmem AFAICS. 
>> Did GEM add the possibility for user pages (highmem amongst 
>> them) ending up in that pool? Which code does that?
>>
>>     
>>> @@ -1131,7 +1139,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>>>  err_out:
>>>  	free_idx = i;
>>>  	for (i = 0; i < free_idx; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		free_memtype(start, end);
>>>  	}
>>> @@ -1160,7 +1170,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
>>>  		return retval;
>>>  
>>>  	for (i = 0; i < addrinarray; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		free_memtype(start, end);
>>>       
>> In any case it's a must-have fix for .31. Possibly even a backport 
>> tag is needed, in case a distro does a .30 kernel with more recent 
>> graphics bits.
>>
>> 	Ingo
>>
>> ------------------------------------------------------------------------------
>> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
>> trial. Simplify your report design, integration and deployment - and focus on 
>> what you do best, core application coding. Discover what's new with 
>> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
>> --
>> _______________________________________________
>> Dri-devel mailing list
>> Dri-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/dri-devel
>>
>>
>>     

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Ingo Molnar]

* Dave Airlie <airlied@linux.ie> wrote:

> > hm, i'm missing a description about how this bug was triggered. 
> > How did you end up getting highmem pages to a cpa call?
> 
> GEM and TTM both allocate page arrays and just pass them to cpa, 
> we don't know what type of pages the allocator gives us back and 
> we really shouldn't have to, so having cpa ignore highmem pages is 
> certainly the right option.
> 
> GEM just uses shmem code to alloc the pages and TTM has its own 
> allocator.

Neither of my questions was answered though: do highmem pages ever 
get passed in and what were the effects of the bug and how was it 
noticed?

If no highmem pages can be passed in then the right solution is not 
to ignore them silently but to add a WARN_ONCE() to enforce that 
they are not used in the future either.

If they are used today then i'd like to know where exactly and 
double check that all the cache attributes are consistent: as a 
highmem page might be visible to user-space as well or might be 
mapped via the vmalloc space, etc.

And yes, if it happens and all the other mapping aliases are fine 
then ignoring them is the right solution.

	Ingo

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Ingo Molnar]

* Thomas Hellstrom <thellstrom@vmware.com> wrote:

> Dave Airlie wrote:
>>> hm, i'm missing a description about how this bug was triggered. How 
>>> did you end up getting highmem pages to a cpa call?
>>>     
>>
>> GEM and TTM both allocate page arrays and just pass them to cpa,
>> we don't know what type of pages the allocator gives us back and we 
>> really shouldn't have to, so having cpa ignore highmem pages is 
>> certainly the right option.
>>
>> GEM just uses shmem code to alloc the pages and TTM has its own allocator.
>>
>>   
> Yes, Dave is right.
>
> Although I'm not 100% sure the TTM code I was using that triggered this  
> has made it into 2.6.31.
> Old AGP uses (GFP_KERNEL | GFP_DMA32 | __GFP_ZERO), which (correct me if  
> I'm wrong) never hands back highmem pages. This means that Intel's GEM  
> is the only likely user for 2.6.31.

(please dont top-post) I'm not sure you folks noticed this bit of my 
mail:

>>> ok, that's a bug introduced in .29 but which was latent until now:  
>>> drivers/char/agp/generic.c now uses it plus (indirectly) a number of  
>>> AGP drivers, since:
>>>
>>>  commit 07613ba2f464f59949266f4337b75b91eb610795
>>>  Author: Dave Airlie <airlied@redhat.com>
>>>  Date:   Fri Jun 12 14:11:41 2009 +1000
>>>
>>>      agp: switch AGP to use page array instead of unsigned long array
>>>
>>> I dont see how it can end up with highmem pages though. All the 
>>> graphics apperture allocations happen to lowmem AFAICS. Did GEM add 
>>> the possibility for user pages (highmem amongst them) ending up in 
>>> that pool? Which code does that?

	Ingo

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Thomas Hellstrom]

Ingo Molnar wrote:
> * Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
>   
>> Dave Airlie wrote:
>>     
>>>> hm, i'm missing a description about how this bug was triggered. How 
>>>> did you end up getting highmem pages to a cpa call?
>>>>     
>>>>         
>>> GEM and TTM both allocate page arrays and just pass them to cpa,
>>> we don't know what type of pages the allocator gives us back and we 
>>> really shouldn't have to, so having cpa ignore highmem pages is 
>>> certainly the right option.
>>>
>>> GEM just uses shmem code to alloc the pages and TTM has its own allocator.
>>>
>>>   
>>>       
>> Yes, Dave is right.
>>
>> Although I'm not 100% sure the TTM code I was using that triggered this  
>> has made it into 2.6.31.
>> Old AGP uses (GFP_KERNEL | GFP_DMA32 | __GFP_ZERO), which (correct me if  
>> I'm wrong) never hands back highmem pages. This means that Intel's GEM  
>> is the only likely user for 2.6.31.
>>     
>
> (please dont top-post) I'm not sure you folks noticed this bit of my 
> mail:
>
>   
>>>> ok, that's a bug introduced in .29 but which was latent until now:  
>>>> drivers/char/agp/generic.c now uses it plus (indirectly) a number of  
>>>> AGP drivers, since:
>>>>
>>>>  commit 07613ba2f464f59949266f4337b75b91eb610795
>>>>  Author: Dave Airlie <airlied@redhat.com>
>>>>  Date:   Fri Jun 12 14:11:41 2009 +1000
>>>>
>>>>      agp: switch AGP to use page array instead of unsigned long array
>>>>
>>>> I dont see how it can end up with highmem pages though. All the 
>>>> graphics apperture allocations happen to lowmem AFAICS. Did GEM add 
>>>> the possibility for user pages (highmem amongst them) ending up in 
>>>> that pool? Which code does that?
>>>>         
>
> 	Ingo
>   
Sorry, my bad.

The TTM code I tested is not in yet, and after double-checking it looks 
like Intel's gem is not changing caching policy before binding to AGP.

This means the highmem problems that I saw were triggered by a 
combination of the virtual->physical bugfix and code that's not in the 
kernel yet, and since it's an optimization of the current code it's not 
likely to land in 2.6.31. The highmem fixes could thus, AFAICT be 
stripped out of the patch, unless GFP_DMA32 on a highmem system can 
actually hand back highmem pages, in which case AGP will not work correctly.

As for highmem use in the future, the TTM page arrays are populated 
using fault(), which means that there will be an overhead ordering the 
pages so that we can use the set_pages_array() interface instead of 
set_memory() that we use today. Therefore, if possible, I'd prefer if we 
could pass arrays containing highmem pages to the set_pages_array() 
interface.

There are no aliased mappings since

1) Any user space mappings to these pages are killed before changing 
caching policy.
2) The pages are allocated and owned by the driver.
3) kmap_atomic_prot() and vmap() are used to map these pages in kernel 
space.

Code is in ttm_tt.c ttm_bo.c and ttm_bo_util.c

Thanks.
/Thomas

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Ingo Molnar]

* Thomas Hellstrom <thellstrom@vmware.com> wrote:

> Ingo Molnar wrote:
>> * Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>
>>   
>>> Dave Airlie wrote:
>>>     
>>>>> hm, i'm missing a description about how this bug was triggered. 
>>>>> How did you end up getting highmem pages to a cpa call?
>>>>>             
>>>> GEM and TTM both allocate page arrays and just pass them to cpa,
>>>> we don't know what type of pages the allocator gives us back and we 
>>>> really shouldn't have to, so having cpa ignore highmem pages is  
>>>> certainly the right option.
>>>>
>>>> GEM just uses shmem code to alloc the pages and TTM has its own allocator.
>>>>
>>>>         
>>> Yes, Dave is right.
>>>
>>> Although I'm not 100% sure the TTM code I was using that triggered 
>>> this  has made it into 2.6.31.
>>> Old AGP uses (GFP_KERNEL | GFP_DMA32 | __GFP_ZERO), which (correct me 
>>> if  I'm wrong) never hands back highmem pages. This means that 
>>> Intel's GEM  is the only likely user for 2.6.31.
>>>     
>>
>> (please dont top-post) I'm not sure you folks noticed this bit of my  
>> mail:
>>
>>   
>>>>> ok, that's a bug introduced in .29 but which was latent until 
>>>>> now:  drivers/char/agp/generic.c now uses it plus (indirectly) a 
>>>>> number of  AGP drivers, since:
>>>>>
>>>>>  commit 07613ba2f464f59949266f4337b75b91eb610795
>>>>>  Author: Dave Airlie <airlied@redhat.com>
>>>>>  Date:   Fri Jun 12 14:11:41 2009 +1000
>>>>>
>>>>>      agp: switch AGP to use page array instead of unsigned long array
>>>>>
>>>>> I dont see how it can end up with highmem pages though. All the  
>>>>> graphics apperture allocations happen to lowmem AFAICS. Did GEM 
>>>>> add the possibility for user pages (highmem amongst them) ending 
>>>>> up in that pool? Which code does that?
>>>>>         
>>
>> 	Ingo
>>   
> Sorry, my bad.
>
> The TTM code I tested is not in yet, and after double-checking it 
> looks like Intel's gem is not changing caching policy before 
> binding to AGP.
>
> This means the highmem problems that I saw were triggered by a 
> combination of the virtual->physical bugfix and code that's not in 
> the kernel yet, and since it's an optimization of the current code 
> it's not likely to land in 2.6.31. The highmem fixes could thus, 
> AFAICT be stripped out of the patch, unless GFP_DMA32 on a highmem 
> system can actually hand back highmem pages, in which case AGP 
> will not work correctly.
>
> As for highmem use in the future, the TTM page arrays are 
> populated using fault(), which means that there will be an 
> overhead ordering the pages so that we can use the 
> set_pages_array() interface instead of set_memory() that we use 
> today. Therefore, if possible, I'd prefer if we could pass arrays 
> containing highmem pages to the set_pages_array() interface.
>
> There are no aliased mappings since
>
> 1) Any user space mappings to these pages are killed before changing  
>    caching policy.
> 2) The pages are allocated and owned by the driver.
> 3) kmap_atomic_prot() and vmap() are used to map these pages in kernel  
> space.
>
> Code is in ttm_tt.c ttm_bo.c and ttm_bo_util.c

ok - thanks for the explanation. Since you intentionally want to use 
highmem pages (and your use is safe) i concur with your original 
patch in its entirety - even if that planned highmem use is not 
upstream yet. Will get it .31-wards ASAP.

	Ingo

[tip:x86/urgent] x86: Fix CPA memtype reserving in the set_pages_array*() cases

[tip-bot for Thomas Hellstrom]

Commit-ID:  9e5b74fef5a7e8e408ba514db8dee5320e633d40
Gitweb:     http://git.kernel.org/tip/9e5b74fef5a7e8e408ba514db8dee5320e633d40
Author:     Thomas Hellstrom <thellstrom@vmware.com>
AuthorDate: Mon, 3 Aug 2009 09:25:45 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 3 Aug 2009 12:47:56 +0200

x86: Fix CPA memtype reserving in the set_pages_array*() cases

The code was incorrectly reserving memtypes using the page
virtual address instead of the physical address. Furthermore,
the code was not ignoring highmem pages as it ought to.

( upstream does not pass in highmem pages yet - but upcoming
  graphics code will do it and there's no reason to not handle
  this properly in the CPA APIs.)

Fixes: http://bugzilla.kernel.org/show_bug.cgi?id=13884

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Cc: dri-devel@lists.sourceforge.net
Cc: venkatesh.pallipadi@intel.com
Cc: suresh.b.siddha@intel.com
LKML-Reference: <1249284345-7654-1-git-send-email-thellstrom@vmware.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/mm/pageattr.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 895d90e..7e600c1 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -591,9 +591,12 @@ static int __change_page_attr(struct cpa_data *cpa, int primary)
 	unsigned int level;
 	pte_t *kpte, old_pte;
 
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		address = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		address = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		address = cpa->vaddr[cpa->curpage];
 	else
 		address = *cpa->vaddr;
@@ -697,9 +700,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		vaddr = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		vaddr = cpa->vaddr[cpa->curpage];
 	else
 		vaddr = *cpa->vaddr;
@@ -1122,7 +1128,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 	int free_idx;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
 			goto err_out;
@@ -1135,7 +1143,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 err_out:
 	free_idx = i;
 	for (i = 0; i < free_idx; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}
@@ -1164,7 +1174,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
 		return retval;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}

Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array cases

[Suresh Siddha]

On Mon, 2009-08-03 at 03:44 -0700, Ingo Molnar wrote:
> ok - thanks for the explanation. Since you intentionally want to use 
> highmem pages (and your use is safe) i concur with your original 
> patch in its entirety - even if that planned highmem use is not 
> upstream yet. Will get it .31-wards ASAP.

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

While none of the drivers in 2.6.30 use this interface, it will be good
if we backport this to 2.6.30-stable aswell.

Also, now that we have set_pages_array interface, do we still need the
set_memory_array_uc interface? Removing that should clean up the cpa
code a bit.

thanks,
suresh

[tip:x86/urgent] x86: Fix CPA memtype reserving in the set_pages_array*() cases

[tip-bot for Thomas Hellstrom]

Commit-ID:  8523acfe40efc1a8d3da8f473ca67cb195b06f0c
Gitweb:     http://git.kernel.org/tip/8523acfe40efc1a8d3da8f473ca67cb195b06f0c
Author:     Thomas Hellstrom <thellstrom@vmware.com>
AuthorDate: Mon, 3 Aug 2009 09:25:45 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 3 Aug 2009 19:36:09 +0200

x86: Fix CPA memtype reserving in the set_pages_array*() cases

The code was incorrectly reserving memtypes using the page
virtual address instead of the physical address. Furthermore,
the code was not ignoring highmem pages as it ought to.

( upstream does not pass in highmem pages yet - but upcoming
  graphics code will do it and there's no reason to not handle
  this properly in the CPA APIs.)

Fixes: http://bugzilla.kernel.org/show_bug.cgi?id=13884

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: <stable@kernel.org>
Cc: dri-devel@lists.sourceforge.net
Cc: venkatesh.pallipadi@intel.com
LKML-Reference: <1249284345-7654-1-git-send-email-thellstrom@vmware.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/mm/pageattr.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 895d90e..7e600c1 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -591,9 +591,12 @@ static int __change_page_attr(struct cpa_data *cpa, int primary)
 	unsigned int level;
 	pte_t *kpte, old_pte;
 
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		address = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		address = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		address = cpa->vaddr[cpa->curpage];
 	else
 		address = *cpa->vaddr;
@@ -697,9 +700,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (cpa->flags & CPA_PAGES_ARRAY)
-		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
-	else if (cpa->flags & CPA_ARRAY)
+	if (cpa->flags & CPA_PAGES_ARRAY) {
+		struct page *page = cpa->pages[cpa->curpage];
+		if (unlikely(PageHighMem(page)))
+			return 0;
+		vaddr = (unsigned long)page_address(page);
+	} else if (cpa->flags & CPA_ARRAY)
 		vaddr = cpa->vaddr[cpa->curpage];
 	else
 		vaddr = *cpa->vaddr;
@@ -1122,7 +1128,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 	int free_idx;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
 			goto err_out;
@@ -1135,7 +1143,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
 err_out:
 	free_idx = i;
 	for (i = 0; i < free_idx; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}
@@ -1164,7 +1174,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
 		return retval;
 
 	for (i = 0; i < addrinarray; i++) {
-		start = (unsigned long)page_address(pages[i]);
+		if (PageHighMem(pages[i]))
+			continue;
+		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
 		end = start + PAGE_SIZE;
 		free_memtype(start, end);
 	}

CPA interfaces WAS x86: Fix CPA memtype reserving in the set_pages_array cases

[Thomas Hellström]

Suresh Siddha skrev:
>
> Also, now that we have set_pages_array interface, do we still need the
> set_memory_array_uc interface? Removing that should clean up the cpa
> code a bit.
>
> thanks,
> suresh
>
>   
This might be a good time to discuss how graphics drivers will use this 
interface in the not too distant future. These are my thoughts.

1) A pool of uc / wc pages: Jerome Glisse and Dave Airlie have been 
working on this. It will hugely speed up the allocation and freeing of 
transient uc / wc buffers. Pages will be allocated and freed in chunks 
of a couple of megabytes, and caching attributes are changed using the 
set_pages_array() interface. The pool will sit in the TTM driver.
1a) Using large pages: With such a pool it would be beneficial to 
allocate and free large pages to avoid page splitting in the CPA code. 
There is no code in the pool implementation for that yet, but I think 
the set_memory_array_xx will be the best interface for that, although I 
guess set_pages_array_xx would work.

2) Optimize cache flushing:
2a) I think both the TTM code and AGP code flushes the cache before 
changing from wb, to make sure the device sees the data. Therefore with 
these callers, I don't think the CPA code  needs to flush the cache 
again, and it might be beneficial with a "cache_already_flushed" bool to 
some of the function apis.
2b) Use self-snooping where available regardless of the value of the 
"cache_already_flushed" bool.

3) set_pages_array_np():
This is a bit controversial, but will enable a certain valuable class of 
graphics optimizations. The idea is to, instead of marking the PTEs as 
wc or uc, marking them as non-present, np. This means that the TTM code 
does not need to bother with CPA if it wants to make quick cached reads 
from graphics buffers. Just use kmap_atomic() and make sure to clflush 
before unmap.
The caveat is that all np pages must be marked wb before hibernation 
since the hibernation code will try to read from the linear kernel map. 
But we have to deal with that today for uc / wc highmem pages today as 
well since the hibernation code will try to kmap() them with wb page 
attributes.

It would be nice to have some comments on these general ideas before 
starting to work on them.

/Thomas