x86: Fix CPA memtype reserving in the set_pages_array cases

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 <[email protected]>

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

arch/x86/mm/pageattr.c +0 -0

index d4327db..96dd4f6 100644
@@ -590,9 +590,12 @@ static int __change_page_attr(struct cpa_data *cpa, int primary)
 	unsigned int level;
 	pte_t *kpte, old_pte;
 

arch/x86/mm/pageattr.c +6 -3

 	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;

arch/x86/mm/pageattr.c +3 -1

@@ -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);
 	}

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

Quoted 20 lines

arch/x86/mm/pageattr.c +6 -3

 	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;

arch/x86/mm/pageattr.c int set_pages_array_uc(struct page **pages, int addrinarray) · line 1132

Quoted 10 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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 <[email protected]>
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?

Quoted 12 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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);
 	}

arch/x86/mm/pageattr.c int set_pages_array_wb(struct page **pages, int addrinarray) · line 1177

Quoted 9 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Quoted 2 lines

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.

Quoted 79 lines

Quoted 22 lines

arch/x86/mm/pageattr.c +6 -3

 	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;

arch/x86/mm/pageattr.c int set_pages_array_uc(struct page **pages, int addrinarray) · line 1132

Quoted 10 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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 <[email protected]>
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?

Quoted 12 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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);
 	}

arch/x86/mm/pageattr.c int set_pages_array_wb(struct page **pages, int addrinarray) · line 1178

Quoted 10 lines

arch/x86/mm/pageattr.c +3 -1

@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Quoted 4 lines · Dave Airlie

Quoted 3 lines

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

Quoted 8 lines

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.

Quoted 8 lines

Quoted 8 lines

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 <[email protected]>
Date: Fri Jun 12 14:11:41 2009 +1000

Quoted 6 lines

Quoted 6 lines

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?

Quoted 8 lines · Thomas Hellstrom

Quoted 6 lines · Dave Airlie

Quoted 3 lines

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.

Quoted 9 lines

Quoted 4 lines

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:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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 <[email protected]>
Date: Fri Jun 12 14:11:41 2009 +1000

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 39 lines · Ingo Molnar

Quoted 13 lines · Thomas Hellstrom

Quoted 11 lines · Dave Airlie

Quoted 3 lines

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.

Quoted 8 lines

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:

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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 <[email protected]>
Date: Fri Jun 12 14:11:41 2009 +1000

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

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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.

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

Quoted 2 lines

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.