Skip to contentSkip to conversation

x86 PAT: track pfnmap mappings with remap_pfn_range and vm_insert_pfn

PATCH In review Venkatesh Pallipadi 5 participants 27 messages Nov 12, 2008 – Dec 16, 2008

Subsystems (6)
Memory management Documentation x86 MM core +2 more subsystems
lore
Share & tools
lore

Venkatesh Pallipadi

author
#0 lore raw

cover x86 PAT: track pfnmap mappings with remap_pfn_range and vm_insert_pfn

(Sorry about the duplicates. Resending as the original mails did not make it to
the list due to some errors in my email header)

Drivers use mmap followed by pgprot_* and remap_pfn_range or vm_insert_pfn, in order to export reserved memory to userspace. Currently, such mappings are not tracked and hence not kept consistent with other mappings (/dev/mem, pci resource, ioremap) for the sme memory, that may exist in the system.

The following patchset adds x86 PAT attribute tracking and untracking for pfnmap related APIs.

First four patches in the patchset are changing the generic mm code to fit in this tracking. Last four patches are x86 specific to make things work with x86 PAT code. The patchset aso introduces pgprot_writecombine interface, which gives writecombine mapping when enabled, falling back to pgprot_noncached otherwise.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
author
#2 lore raw

PATCH 2/8 x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 mm/memory.c |    2 ++
 1 file changed, 2 insertions(+)
mm/memory.c +2 -0
Index: tip/mm/memory.c
===================================================================
@@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct 
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return -EFAULT;
+
+	vma->vm_flags |= VM_PFNMAP;
 	return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
 }
 EXPORT_SYMBOL(vm_insert_pfn);
author
#14 lore raw

PATCH 3/8 x86 PAT: Add follow_pfnmp_pte routine to help tracking pfnmap pages

In reply to Venkatesh Pallipadi

Add a generic interface to follow pfn in a pfnmap vma range. This is used by one of the subsequent x86 PAT related patch to keep track of memory types for vma regions across vma copy and free.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 include/linux/mm.h |    3 +++
 mm/memory.c        |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
include/linux/mm.h +3 -0
Index: tip/include/linux/mm.h
===================================================================
@@ -1223,6 +1223,9 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_ANON	0x08	/* give ZERO_PAGE if no pgtable */
 
+unsigned long follow_pfnmap_pte(struct vm_area_struct *vma,
+				unsigned long address, pte_t *ret_ptep);
+
 typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
 			void *data);
 extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
mm/memory.c +43 -0

49 lines not on this page · raw message

author
#17 lore raw

PATCH 4/8 x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

In reply to Venkatesh Pallipadi

Introduce generic hooks in remap_pfn_range and vm_insert_pfn and corresponding copy and free routines with reserve and free tracking.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 include/linux/mm.h |    6 +++++
 mm/memory.c        |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)
mm/memory.c +53 -1
Index: tip/mm/memory.c
===================================================================
@@ -99,6 +99,28 @@ int randomize_va_space __read_mostly =
 					2;
 #endif
 
+#ifndef track_pfn_vma_new
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	return 0;
+}
+#endif
+
+#ifndef track_pfn_vma_copy
+int track_pfn_vma_copy(struct vm_area_struct *vma)
+{
+	return 0;
+}
+#endif
+
+#ifndef untrack_pfn_vma
+void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+			unsigned long size)
+{
+}
+#endif
+
 static int __init disable_randmaps(char *s)
 {
 	randomize_va_space = 0;

@@ -669,6 +691,16 @@ int copy_page_range(struct mm_struct *ds
 	if (is_vm_hugetlb_page(vma))
 		return copy_hugetlb_page_range(dst_mm, src_mm, vma);
 
+	if (is_pfn_mapping(vma)) {
+		/*
+		 * We do not free on error cases below as remove_vma
+		 * gets called on error from higher level routine
+		 */
+		ret = track_pfn_vma_copy(vma);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * We need to invalidate the secondary MMU mappings only when
 	 * there could be a permission downgrade on the ptes of the

@@ -915,6 +947,9 @@ unsigned long unmap_vmas(struct mmu_gath
 		if (vma->vm_flags & VM_ACCOUNT)
 			*nr_accounted += (end - start) >> PAGE_SHIFT;
 
+		if (is_pfn_mapping(vma))
+			untrack_pfn_vma(vma, 0, 0);
+
 		while (start != end) {
 			if (!tlb_start_valid) {
 				tlb_start = start;

@@ -1473,6 +1508,7 @@ out:
 int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 			unsigned long pfn)
 {
+	int ret;
 	/*
 	 * Technically, architectures with pte_special can avoid all these
 	 * restrictions (same for remap_pfn_range).  However we would like

@@ -1489,7 +1525,15 @@ int vm_insert_pfn(struct vm_area_struct 
 		return -EFAULT;
 
 	vma->vm_flags |= VM_PFNMAP;
-	return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
+	if (track_pfn_vma_new(vma, vma->vm_page_prot, pfn, PAGE_SIZE))
+		return -EINVAL;
+
+  	ret = insert_pfn(vma, addr, pfn, vma->vm_page_prot);
+
+	if (ret)
+		untrack_pfn_vma(vma, pfn, PAGE_SIZE);
+
+	return ret;
 }
 EXPORT_SYMBOL(vm_insert_pfn);
 

@@ -1627,6 +1671,10 @@ int remap_pfn_range(struct vm_area_struc
 
 	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;
 
+	err = track_pfn_vma_new(vma, prot, pfn, PAGE_ALIGN(size));
+	if (err)
+		return -EINVAL;
+
 	BUG_ON(addr >= end);
 	pfn -= addr >> PAGE_SHIFT;
 	pgd = pgd_offset(mm, addr);

@@ -1638,6 +1686,10 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+
+	if (err)
+		untrack_pfn_vma(vma, pfn, PAGE_ALIGN(size));
+
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
include/linux/mm.h +6 -0
Index: tip/include/linux/mm.h
===================================================================
@@ -155,6 +155,12 @@ static inline int is_pfn_mapping(struct 
 	return (vma->vm_flags & VM_PFNMAP);
 }
 
+extern int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+				unsigned long pfn, unsigned long size);
+extern int track_pfn_vma_copy(struct vm_area_struct *vma);
+extern void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+				unsigned long size);
+
 /*
  * vm_fault is filled by the the pagefault handler and passed to the vma's
  * ->fault function. The vma's ->fault is responsible for returning a bitmask
author
#25 lore raw

PATCH 7/8 x86 PAT: add pgprot_writecombine() interface for drivers

In reply to Venkatesh Pallipadi

Add pgprot_writecombine. pgprot_writecombine will be aliased to pgprot_noncached when not supported by the architecture.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 arch/x86/include/asm/pgtable.h |    3 +++
 arch/x86/mm/pat.c              |    8 ++++++++
 include/asm-generic/pgtable.h  |    4 ++++
 3 files changed, 15 insertions(+)
include/asm-generic/pgtable.h +4 -0
Index: tip/include/asm-generic/pgtable.h
===================================================================
@@ -129,6 +129,10 @@ static inline void ptep_set_wrprotect(st
 #define move_pte(pte, prot, old_addr, new_addr)	(pte)
 #endif
 
+#ifndef pgprot_writecombine
+#define pgprot_writecombine pgprot_noncached
+#endif
+
 /*
  * When walking page tables, get the address of the next boundary,
  * or the end address of the range if that comes earlier.  Although no
arch/x86/include/asm/pgtable.h +3 -0
Index: tip/arch/x86/include/asm/pgtable.h
===================================================================
@@ -168,6 +168,9 @@
 
 #ifndef __ASSEMBLY__
 
+#define pgprot_writecombine	pgprot_writecombine
+extern pgprot_t pgprot_writecombine(pgprot_t prot);
+
 /*
  * ZERO_PAGE is a global shared page that is always zero: used
  * for zero-mapped memory areas etc..
arch/x86/mm/pat.c +8 -0
Index: tip/arch/x86/mm/pat.c
===================================================================
@@ -797,6 +797,14 @@ void untrack_pfn_vma(struct vm_area_stru
 	}
 }
 
+pgprot_t pgprot_writecombine(pgprot_t prot)
+{
+	if (pat_enabled)
+		return __pgprot(pgprot_val(prot) | _PAGE_CACHE_WC);
+	else
+		return pgprot_noncached(prot);
+}
+
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
 
 /* get Nth element of the linked list */
author
#21 lore raw

PATCH 5/8 x86 PAT: Implement track/untrack of pfnmap regions for x86

In reply to Venkatesh Pallipadi

Hookup remap_pfn_range and vm_insert_pfn and corresponding copy and free routines with reserve and free tracking.

reserve and free here only takes care of non RAM region mapping. For RAM region, driver should use set_memory_[uc|wc|wb] to set the cache type and then setup the mapping for user pte. We can bypass below reserve/free in that case.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 arch/x86/include/asm/pgtable.h |   10 ++
 arch/x86/mm/pat.c              |  201 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 211 insertions(+)
arch/x86/mm/pat.c +201 -0
Index: tip/arch/x86/mm/pat.c
===================================================================
@@ -596,6 +596,207 @@ void unmap_devmem(unsigned long pfn, uns
 	free_memtype(addr, addr + size);
 }
 
+int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t vma_prot)
+{
+	unsigned long flags;
+	unsigned long want_flags = (pgprot_val(vma_prot) & _PAGE_CACHE_MASK);
+
+	int is_ram = 0;
+	int id_sz, ret;
+
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+
+	if (is_ram != 0) {
+		/*
+		 * For mapping RAM pages, drivers need to call
+		 * set_memory_[uc|wc|wb] directly, for reserve and free, before
+		 * setting up the PTE.
+		 */
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	ret = reserve_memtype(paddr, paddr + size, want_flags, &flags);
+	if (ret)
+		return ret;
+
+	if (flags != want_flags) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO
+		"%s:%d map pfn expected mapping type %s for %Lx-%Lx, got %s\n",
+			current->comm, current->pid,
+			cattr_name(want_flags),
+			paddr, (unsigned long long)(paddr + size),
+			cattr_name(flags));
+		return -EINVAL;
+	}
+
+	/* Need to keep identity mapping in sync */
+	if (paddr >= __pa(high_memory))
+		return 0;
+
+	id_sz = (__pa(high_memory) < paddr + size) ?
+				__pa(high_memory) - paddr :
+				size;
+
+	if (ioremap_change_attr((unsigned long)__va(paddr), id_sz, flags) < 0) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO
+			"%s:%d reserve_pfn_range ioremap_change_attr failed %s "
+			"for %Lx-%Lx\n",
+			current->comm, current->pid,
+			cattr_name(flags),
+			paddr, (unsigned long long)(paddr + size));
+		return -EINVAL;
+	}
+	return 0;
+}
+
+void free_pfn_range(u64 paddr, unsigned long size)
+{
+	int is_ram;
+
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+	if (is_ram == 0)
+		free_memtype(paddr, paddr + size);
+}
+
+int track_pfn_vma_copy(struct vm_area_struct *vma)
+{
+	int retval = 0;
+	unsigned long i, j;
+	u64 paddr;
+	pgprot_t prot;
+	pte_t pte;
+	unsigned long vma_start = vma->vm_start;
+	unsigned long vma_end = vma->vm_end;
+	unsigned long vma_size = vma_end - vma_start;
+
+	if (!pat_enabled)
+		return 0;
+
+	if (is_linear_pfn_mapping(vma)) {
+		/*
+		 * reserve the whole chunk starting from vm_pgoff,
+		 * But, we have to get the protection from pte.
+		 */
+		if (follow_pfnmap_pte(vma, vma_start, &pte)) {
+			WARN_ON_ONCE(1);
+			return -1;
+		}
+		prot = pte_pgprot(pte);
+		paddr = (u64)vma->vm_pgoff << PAGE_SHIFT;
+		return reserve_pfn_range(paddr, vma_size, prot);
+	}
+
+	/* reserve entire vma page by page, using pfn and prot from pte */
+	for (i = 0; i < vma_size; i += PAGE_SIZE) {
+		if (follow_pfnmap_pte(vma, vma_start + i, &pte))
+			continue;
+
+		paddr = pte_pa(pte);
+		prot = pte_pgprot(pte);
+		retval = reserve_pfn_range(paddr, PAGE_SIZE, prot);
+		if (retval)
+			goto cleanup_ret;
+	}
+	return 0;
+
+cleanup_ret:
+	/* Reserve error: Cleanup partial reservation and return error */
+	for (j = 0; j < i; j += PAGE_SIZE) {
+		if (follow_pfnmap_pte(vma, vma_start + j, &pte))
+			continue;
+
+		paddr = pte_pa(pte);
+		free_pfn_range(paddr, PAGE_SIZE);
+	}
+
+	return retval;
+}
+
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	int retval = 0;
+	unsigned long i, j;
+	u64 base_paddr;
+	u64 paddr;
+	unsigned long vma_start = vma->vm_start;
+	unsigned long vma_end = vma->vm_end;
+	unsigned long vma_size = vma_end - vma_start;
+
+	if (!pat_enabled)
+		return 0;
+
+	if (is_linear_pfn_mapping(vma)) {
+		/* reserve the whole chunk starting from vm_pgoff */
+		paddr = (u64)vma->vm_pgoff << PAGE_SHIFT;
+		return reserve_pfn_range(paddr, vma_size, prot);
+	}
+
+	/* reserve page by page using pfn and size */
+	base_paddr = (u64)pfn << PAGE_SHIFT;
+	for (i = 0; i < size; i += PAGE_SIZE) {
+		paddr = base_paddr + i;
+		retval = reserve_pfn_range(paddr, PAGE_SIZE, prot);
+		if (retval)
+			goto cleanup_ret;
+	}
+	return 0;
+
+cleanup_ret:
+	/* Reserve error: Cleanup partial reservation and return error */
+	for (j = 0; j < i; j += PAGE_SIZE) {
+		paddr = base_paddr + j;
+		free_pfn_range(paddr, PAGE_SIZE);
+	}
+
+	return retval;
+}
+
+void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+			unsigned long size)
+{
+	unsigned long i;
+	u64 paddr;
+	unsigned long vma_start = vma->vm_start;
+	unsigned long vma_end = vma->vm_end;
+	unsigned long vma_size = vma_end - vma_start;
+
+	if (!pat_enabled)
+		return;
+
+	if (is_linear_pfn_mapping(vma)) {
+		/* free the whole chunk starting from vm_pgoff */
+		paddr = (u64)vma->vm_pgoff << PAGE_SHIFT;
+		free_pfn_range(paddr, vma_size);
+		return;
+	}
+
+	if (size != 0 && size != vma_size) {
+		/* free page by page, using pfn and size */
+
+		paddr = (u64)pfn << PAGE_SHIFT;
+		for (i = 0; i < size; i += PAGE_SIZE) {
+			paddr = paddr + i;
+			free_pfn_range(paddr, PAGE_SIZE);
+		}
+	} else {
+		/* free entire vma, page by page, using the pfn from pte */
+
+		for (i = 0; i < vma_size; i += PAGE_SIZE) {
+			pte_t pte;
+
+			if (follow_pfnmap_pte(vma, vma_start + i, &pte))
+				continue;
+
+			paddr = pte_pa(pte);
+			free_pfn_range(paddr, PAGE_SIZE);
+		}
+	}
+}
+
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
 
 /* get Nth element of the linked list */
arch/x86/include/asm/pgtable.h +10 -0
Index: tip/arch/x86/include/asm/pgtable.h
===================================================================
@@ -219,6 +219,11 @@ static inline unsigned long pte_pfn(pte_
 	return (pte_val(pte) & PTE_PFN_MASK) >> PAGE_SHIFT;
 }
 
+static inline u64 pte_pa(pte_t pte)
+{
+	return pte_val(pte) & PTE_PFN_MASK;
+}
+
 #define pte_page(pte)	pfn_to_page(pte_pfn(pte))
 
 static inline int pmd_large(pmd_t pte)

@@ -328,6 +333,11 @@ static inline pgprot_t pgprot_modify(pgp
 
 #define canon_pgprot(p) __pgprot(pgprot_val(p) & __supported_pte_mask)
 
+/* Indicate that x86 has its own track and untrack pfn vma functions */
+#define track_pfn_vma_new track_pfn_vma_new
+#define track_pfn_vma_copy track_pfn_vma_copy
+#define untrack_pfn_vma untrack_pfn_vma
+
 #ifndef __ASSEMBLY__
 #define __HAVE_PHYS_MEM_ACCESS_PROT
 struct file;
author
#24 lore raw

PATCH 6/8 x86 PAT: change pgprot_noncached to uc_minus instead of strong uc

In reply to Venkatesh Pallipadi

Make pgprot_noncached uc_minus instead of strong UC. This will make pgprot_noncached to be in line with ioremap_nocache() and all the other APIs that map page uc_minus on uc request.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 arch/x86/include/asm/pgtable.h    |    8 ++++++++
 arch/x86/include/asm/pgtable_32.h |    9 ---------
 arch/x86/include/asm/pgtable_64.h |    6 ------
 3 files changed, 8 insertions(+), 15 deletions(-)
arch/x86/include/asm/pgtable.h +8 -0
Index: tip/arch/x86/include/asm/pgtable.h
===================================================================
@@ -158,6 +158,14 @@
 #define PGD_IDENT_ATTR	 0x001		/* PRESENT (no other attributes) */
 #endif
 
+/*
+ * Macro to mark a page protection value as UC-
+ */
+#define pgprot_noncached(prot)					\
+	((boot_cpu_data.x86 > 3)				\
+	 ? (__pgprot(pgprot_val(prot) | _PAGE_CACHE_UC_MINUS))	\
+	 : (prot))
+
 #ifndef __ASSEMBLY__
 
 /*
arch/x86/include/asm/pgtable_32.h +0 -9
Index: tip/arch/x86/include/asm/pgtable_32.h
===================================================================
@@ -107,15 +107,6 @@ extern unsigned long pg0[];
 #endif
 
 /*
- * Macro to mark a page protection value as "uncacheable".
- * On processors which do not support it, this is a no-op.
- */
-#define pgprot_noncached(prot)					\
-	((boot_cpu_data.x86 > 3)				\
-	 ? (__pgprot(pgprot_val(prot) | _PAGE_PCD | _PAGE_PWT))	\
-	 : (prot))
-
-/*
  * Conversion functions: convert a page and protection to a page entry,
  * and a page entry and page directory to the page they refer to.
  */
arch/x86/include/asm/pgtable_64.h +0 -6
Index: tip/arch/x86/include/asm/pgtable_64.h
===================================================================
@@ -183,12 +183,6 @@ static inline int pmd_bad(pmd_t pmd)
 #define pages_to_mb(x)	((x) >> (20 - PAGE_SHIFT))   /* FIXME: is this right? */
 
 /*
- * Macro to mark a page protection value as "uncacheable".
- */
-#define pgprot_noncached(prot)					\
-	(__pgprot(pgprot_val((prot)) | _PAGE_PCD | _PAGE_PWT))
-
-/*
  * Conversion functions: convert a page and protection to a page entry,
  * and a page entry and page directory to the page they refer to.
  */
author
#26 lore raw

PATCH 8/8 x86 PAT: update documentation to cover pgprot and remap_pfn related changes

In reply to Venkatesh Pallipadi

Add documentation related to pgprot_* change.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 Documentation/x86/pat.txt |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Documentation/x86/pat.txt +24 -0
Index: tip/Documentation/x86/pat.txt
===================================================================
@@ -80,6 +80,30 @@ pci proc               |    --    |    -
                        |          |            |                  |
 -------------------------------------------------------------------
 
+Advanced APIs for drivers
+-------------------------
+A. Exporting pages to user with remap_pfn_range, io_remap_pfn_range,
+vm_insert_pfn
+
+Drivers wanting to export some pages to userspace, do it by using mmap
+interface and a combination of
+1) pgprot_noncached()
+2) io_remap_pfn_range() or remap_pfn_range() or vm_insert_pfn()
+
+With pat support, a new API pgprot_writecombine is being added. So, driver can
+continue to use the above sequence, with either pgprot_noncached() or
+pgprot_writecombine() in step 1, followed by step 2.
+
+In addition, step 2 internally tracks the region as UC or WC in memtype
+list in order to ensure no conflicting mapping.
+
+Note that this set of APIs only work with IO (non RAM) regions. If driver
+wants to export RAM region, it has to do set_memory_uc() or set_memory_wc()
+as step 0 above and also track the usage of those pages and use set_memory_wb()
+before the page is freed to free pool.
+
+
+
 Notes:
 
 -- in the above table mean "Not suggested usage for the API". Some of the --'s
author
#1 lore raw

PATCH 1/8 x86 PAT: store vm_pgoff for all linear_over_vma_region mappings

In reply to Venkatesh Pallipadi

Drivers use mmap followed by pgprot_* and remap_pfn_range or vm_insert_pfn, in order to export reserved memory to userspace. Currently, such mappings are not tracked and hence not kept consistent with other mappings (/dev/mem, pci resource, ioremap) for the sme memory, that may exist in the system.

The following patchset adds x86 PAT attribute tracking and untracking for pfnmap related APIs.

First four patches in the patchset are changing the generic mm code to fit in this tracking. Last four patches are x86 specific to make things work with x86 PAT code. The patchset aso introduces pgprot_writecombine interface, which gives writecombine mapping when enabled, falling back to pgprot_noncached otherwise.

This patch:

While working on x86 PAT, we faced some hurdles with trackking remap_pfn_range() regions, as we do not have any information to say whether that PFNMAP mapping is linear for the entire vma range or it is smaller granularity regions within the vma.

A simple solution to this is to use vm_pgoff as an indicator for linear mapping over the vma region. Currently, remap_pfn_range only sets vm_pgoff for COW mappings. Below patch changes the logic and sets the vm_pgoff irrespective of COW. This will still not be enough for the case where pfn is zero (vma region mapped to physical address zero). But, for all the other cases, we can look at pfnmap VMAs and say whether the mappng is for the entire vma region or not.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 include/linux/mm.h |    9 +++++++++
 mm/memory.c        |    7 +++----
 2 files changed, 12 insertions(+), 4 deletions(-)
mm/memory.c +3 -4
Index: tip/mm/memory.c
===================================================================
@@ -1575,11 +1575,10 @@ int remap_pfn_range(struct vm_area_struc
 	 * behaviour that some programs depend on. We mark the "original"
 	 * un-COW'ed pages by matching them up with "vma->vm_pgoff".
 	 */
-	if (is_cow_mapping(vma->vm_flags)) {
-		if (addr != vma->vm_start || end != vma->vm_end)
-			return -EINVAL;
+	if (addr == vma->vm_start && end == vma->vm_end)
 		vma->vm_pgoff = pfn;
-	}
+	else if (is_cow_mapping(vma->vm_flags))
+		return -EINVAL;
 
 	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;
include/linux/mm.h +9 -0
Index: tip/include/linux/mm.h
===================================================================
@@ -145,6 +145,15 @@ extern pgprot_t protection_map[16];
 #define FAULT_FLAG_WRITE	0x01	/* Fault was a write access */
 #define FAULT_FLAG_NONLINEAR	0x02	/* Fault was via a nonlinear mapping */
 
+static inline int is_linear_pfn_mapping(struct vm_area_struct *vma)
+{
+	return ((vma->vm_flags & VM_PFNMAP) && vma->vm_pgoff);
+}
+
+static inline int is_pfn_mapping(struct vm_area_struct *vma)
+{
+	return (vma->vm_flags & VM_PFNMAP);
+}
 
 /*
  * vm_fault is filled by the the pagefault handler and passed to the vma's
#3 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

In reply to Venkatesh Pallipadi

You have to be careful of this, because it can be called with mmap_sem held for read only. Hmm, I guess vm_insert_page is doing the same thing. Probably mostly works because all other modifiers of vm_flags are holding mmap_sem.

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover most of those cases).

Still, I'd be much happier if we could make these into BUG_ON, and then teach callers to set it in their .mmap routines.

Quoted 24 lines · Venkatesh Pallipadi

vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 mm/memory.c |    2 ++
 1 file changed, 2 insertions(+)

Index: tip/mm/memory.c
===================================================================

mm/memory.c +2 -0
@@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct 
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return -EFAULT;
+
+	vma->vm_flags |= VM_PFNMAP;
 	return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
 }
 EXPORT_SYMBOL(vm_insert_pfn);
Signature · 1 line
--
#15 lore raw

Re: [patch 3/8] x86 PAT: Add follow_pfnmp_pte routine to help tracking pfnmap pages

In reply to Venkatesh Pallipadi
Quoted 32 lines · Venkatesh Pallipadi

Add a generic interface to follow pfn in a pfnmap vma range. This is used by one of the subsequent x86 PAT related patch to keep track of memory types for vma regions across vma copy and free.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 include/linux/mm.h |    3 +++
 mm/memory.c        |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

Index: tip/include/linux/mm.h
===================================================================

include/linux/mm.h +3 -0
@@ -1223,6 +1223,9 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_ANON	0x08	/* give ZERO_PAGE if no pgtable */
 
+unsigned long follow_pfnmap_pte(struct vm_area_struct *vma,
+				unsigned long address, pte_t *ret_ptep);
+
 typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
 			void *data);
 extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,

Index: tip/mm/memory.c
===================================================================

mm/memory.c no_page_table: · line 1116

Quoted 6 lines
mm/memory.c +3 -0
@@ -1111,6 +1111,49 @@ no_page_table:
 	return page;
 }
 
+unsigned long follow_pfnmap_pte(struct vm_area_struct *vma,
+			unsigned long address, pte_t *ret_ptep)
+{

I think we'd typically return an int for functions like this. It probably wouldn't hurt to return more descriptive errors either. -EINVAL, perhaps, seems to fit best with what existing code there is doing.

Quoted 45 lines
mm/memory.c +40 -0
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *ptep, pte;
+	spinlock_t *ptl;
+	struct page *page;
+	struct mm_struct *mm = vma->vm_mm;
+
+	if (!is_pfn_mapping(vma))
+		goto err;
+
+	page = NULL;
+	pgd = pgd_offset(mm, address);
+	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
+		goto err;
+
+	pud = pud_offset(pgd, address);
+	if (pud_none(*pud) || unlikely(pud_bad(*pud)))
+		goto err;
+
+	pmd = pmd_offset(pud, address);
+	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
+		goto err;
+
+	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
+
+	pte = *ptep;
+	if (!pte_present(pte))
+		goto err_unlock;
+
+	*ret_ptep = pte;
+	pte_unmap_unlock(ptep, ptl);
+	return 0;
+
+err_unlock:
+	pte_unmap_unlock(ptep, ptl);
+err:
+	return -1;
+}
+
 /* Can we do the FOLL_ANON optimization? */
 static inline int use_zero_page(struct vm_area_struct *vma)
 {
Signature · 1 line
--
author
#16 lore raw

RE: [patch 3/8] x86 PAT: Add follow_pfnmp_pte routine to help tracking pfnmap pages

Quoted 54 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 3:27 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 3/8] x86 PAT: Add follow_pfnmp_pte routine to help tracking pfnmap pages

Quoted 2 lines · Venkatesh Pallipadi

Add a generic interface to follow pfn in a pfnmap vma range.

This is used by

Quoted 1 line

one of the subsequent x86 PAT related patch to keep track of

memory types

Quoted 8 lines

for vma regions across vma copy and free.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 include/linux/mm.h |    3 +++
 mm/memory.c        |   43

+++++++++++++++++++++++++++++++++++++++++++

Quoted 5 lines
 2 files changed, 46 insertions(+)

Index: tip/include/linux/mm.h
===================================================================
--- tip.orig/include/linux/mm.h 2008-11-06

15:41:43.000000000 -0800

include/linux/mm.h struct page *follow_page(struct vm_area_ · line 1229

Quoted 9 lines

+++ tip/include/linux/mm.h 2008-11-10 09:44:45.000000000 -0800

include/linux/mm.h +3 -0
@@ -1223,6 +1223,9 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_ANON	0x08	/* give ZERO_PAGE if no pgtable */
 
+unsigned long follow_pfnmap_pte(struct vm_area_struct *vma,
+				unsigned long address, pte_t *ret_ptep);
+
 typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token,

unsigned long addr,

Review struct page *follow_page(struct vm_area_ · line 1231

Quoted 2 lines
(quoted diff) +0 -0
 			void *data);
 extern int apply_to_page_range(struct mm_struct *mm,

unsigned long address,

Quoted 8 lines

Index: tip/mm/memory.c
===================================================================

mm/memory.c +0 -0
@@ -1111,6 +1111,49 @@ no_page_table:
 	return page;
 }
Quoted 9 lines

mm/memory.c no_page_table: · line 1116

Quoted 3 lines
mm/memory.c +3 -0
+unsigned long follow_pfnmap_pte(struct vm_area_struct *vma,
+			unsigned long address, pte_t *ret_ptep)
+{

I think we'd typically return an int for functions like this. It probably wouldn't hurt to return more descriptive errors either. -EINVAL, perhaps, seems to fit best with what existing code there is doing.

Yes. Will change the return type to int with more appropriate return values and update the patch.

Thanks,
Venki

author
#4 lore raw

RE: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

In reply to Nick Piggin
Quoted 11 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 3:23 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 6 lines

You have to be careful of this, because it can be called with mmap_sem held for read only. Hmm, I guess vm_insert_page is doing the same thing.
Probably mostly works because all other modifiers of vm_flags are holding
mmap_sem.

Yes. I did the patch looking at vm_insert_page doing similar thing.

Quoted 7 lines

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover most of those cases).

Still, I'd be much happier if we could make these into BUG_ON, and then teach callers to set it in their .mmap routines.

Actually, vm_insert_pfn() already has a BUG_ON() at the start for cases where neither (or both) MIXEDMAP and PFNMAP is not set. So, that should cover the case we are worried about it here and we can eliminate this patch altogether. Only part I am not sure about is why we are looking for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?

Thanks,
Venki

#5 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 25 lines · Venkatesh
Quoted 11 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 3:23 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 6 lines

You have to be careful of this, because it can be called with mmap_sem held for read only. Hmm, I guess vm_insert_page is doing the same thing.
Probably mostly works because all other modifiers of vm_flags are holding
mmap_sem.

Yes. I did the patch looking at vm_insert_page doing similar thing.

Quoted 5 lines

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover most of those cases).

Quoted 8 lines
Quoted 2 lines

Still, I'd be much happier if we could make these into BUG_ON, and then teach callers to set it in their .mmap routines.

Actually, vm_insert_pfn() already has a BUG_ON() at the start for cases where neither (or both) MIXEDMAP and PFNMAP is not set. So, that should cover the case we are worried about it here and we can eliminate this patch altogether. Only part I am not sure about is why we are looking for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?

They should, but it will do an inesrt_pfn in some cases, won't it?

author
#6 lore raw

RE: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 45 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 7:44 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 10 lines · Venkatesh
Quoted 10 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 3:23 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in

vm_insert_pfn

Quoted 2 lines
Quoted 2 lines

You have to be careful of this, because it can be called

with mmap_sem

Quoted 11 lines
Quoted 5 lines

held for read only. Hmm, I guess vm_insert_page is doing the same thing.
Probably mostly works because all other modifiers of vm_flags are holding
mmap_sem.

Yes. I did the patch looking at vm_insert_page doing similar thing.

Quoted 3 lines

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover

most of those

Quoted 3 lines
Quoted 3 lines

cases).

Still, I'd be much happier if we could make these into

BUG_ON, and then

Quoted 3 lines
Quoted 1 line

teach callers to set it in their .mmap routines.

Actually, vm_insert_pfn() already has a BUG_ON() at the

Quoted 9 lines

start for cases

Quoted 1 line

where neither (or both) MIXEDMAP and PFNMAP is not set. So,

that should

Quoted 3 lines

cover the case we are worried about it here and we can eliminate this patch altogether. Only part I am not sure about is why we are looking for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?

They should, but it will do an inesrt_pfn in some cases, won't it?

Yes. It does. But, it calls a lower level insert_pfn() function. The lower level insert_pfn() does not have any bug checks. But the higher level vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Thanks,
Venki

#7 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 38 lines · Venkatesh
Quoted 38 lines
Quoted 8 lines
Quoted 8 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Wednesday, November 12, 2008 3:23 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H.Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in

vm_insert_pfn

Quoted 2 lines
Quoted 2 lines

You have to be careful of this, because it can be called

with mmap_sem

Quoted 11 lines
Quoted 5 lines

held for read only. Hmm, I guess vm_insert_page is doing the same thing.
Probably mostly works because all other modifiers of vm_flags are holding
mmap_sem.

Yes. I did the patch looking at vm_insert_page doing similar thing.

Quoted 3 lines

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover

most of those

Quoted 3 lines
Quoted 3 lines

cases).

Still, I'd be much happier if we could make these into

BUG_ON, and then

Quoted 3 lines
Quoted 1 line

teach callers to set it in their .mmap routines.

Actually, vm_insert_pfn() already has a BUG_ON() at the

start for cases

Quoted 1 line

where neither (or both) MIXEDMAP and PFNMAP is not set. So,

that should

Quoted 3 lines

cover the case we are worried about it here and we can eliminate this patch altogether. Only part I am not sure about is why we are looking for MIXEDMAP here. Shouldn't they be using vm_insert_mixed instead?

Quoted 6 lines
Quoted 2 lines

They should, but it will do an inesrt_pfn in some cases, won't it?

Yes. It does. But, it calls a lower level insert_pfn() function. The lower level insert_pfn() does not have any bug checks. But the higher level vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Yes, but is there anything extra you need to check for cache aliases in MIXEDMAP mappings?

author
#8 lore raw

RE: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 13 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Thursday, November 13, 2008 6:06 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 1 line · Venkatesh
Quoted 8 lines
Quoted 1 line

Yes. It does. But, it calls a lower level insert_pfn()

function. The lower

Quoted 2 lines

level insert_pfn() does not have any bug checks. But the higher level vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Yes, but is there anything extra you need to check for cache aliases in MIXEDMAP mappings?

Yes. We need additional things to track MIXEDMAP and we are looking at that. But, that is slightly more trickier than the general PFNMAP case. And only in-tree user of MIXEDMAP is xip and that too it only uses it for regular WB mapping. So, we thought we should fix the more common case first here.

With MIXEDMAP there is no way whether to distinguish whether insert_pfn Or insert_page was used while looking at VMA. We can probably use PFNMAP in addition to MIXEDMAP to indicate that, which will make things easier. But, we are still looking at that and trying to understand the change implication.

Thanks,
Venki

#13 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

In reply to Nick Piggin
Quoted 11 lines · Nick Piggin

You have to be careful of this, because it can be called with mmap_sem held for read only. Hmm, I guess vm_insert_page is doing the same thing. Probably mostly works because all other modifiers of vm_flags are holding mmap_sem.

However, in some cases, code can do vm_insert_pfn and vm_insert_page (actually hmm, no vm_insert_mixed actually should cover most of those cases).

Still, I'd be much happier if we could make these into BUG_ON, and then teach callers to set it in their .mmap routines.

Agreed. I don't think vm_insert_* is the right place to muck with that ..

Ben.

Quoted 31 lines · Venkatesh Pallipadi
Quoted 24 lines

vm_insert_pfn() is not setting the VM_PFNMAP flag in vma. Fix that.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 mm/memory.c |    2 ++
 1 file changed, 2 insertions(+)

Index: tip/mm/memory.c
===================================================================

mm/memory.c +2 -0
@@ -1444,6 +1444,8 @@ int vm_insert_pfn(struct vm_area_struct 
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return -EFAULT;
+
+	vma->vm_flags |= VM_PFNMAP;
 	return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
 }
 EXPORT_SYMBOL(vm_insert_pfn);
Signature · 1 line
--

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#9 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

In reply to Pallipadi, Venkatesh
Quoted 28 lines · Venkatesh
Quoted 13 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Thursday, November 13, 2008 6:06 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 1 line · Venkatesh
Quoted 8 lines
Quoted 1 line

Yes. It does. But, it calls a lower level insert_pfn()

function. The lower

Quoted 2 lines

level insert_pfn() does not have any bug checks. But the higher level vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Yes, but is there anything extra you need to check for cache aliases in MIXEDMAP mappings?

Yes. We need additional things to track MIXEDMAP and we are looking at that. But, that is slightly more trickier than the general PFNMAP case. And only in-tree user of MIXEDMAP is xip and that too it only uses it for regular WB mapping. So, we thought we should fix the more common case first here.

Quoted 3 lines

With MIXEDMAP there is no way whether to distinguish whether insert_pfn Or insert_page was used while looking at VMA. We can probably use PFNMAP in addition to MIXEDMAP to indicate that, which will make things easier.

It's difficult because it can have either method for a single VMA, and a given address in the vma may even change over time (not with current code in kernel AFAIKS, but AXFS eventually might get to that point).

Quoted 2 lines

But, we are still looking at that and trying to understand the change implication.

OK: now I understand correctly. Getting PFNMAP working is an important first step. I agree.

Thanks,
Nick

#10 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 38 lines · Nick Piggin
Quoted 28 lines · Venkatesh
Quoted 13 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Thursday, November 13, 2008 6:06 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 1 line · Venkatesh
Quoted 8 lines
Quoted 1 line

Yes. It does. But, it calls a lower level insert_pfn()

function. The lower

Quoted 2 lines

level insert_pfn() does not have any bug checks. But the higher level vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Yes, but is there anything extra you need to check for cache aliases in MIXEDMAP mappings?

Yes. We need additional things to track MIXEDMAP and we are looking at that. But, that is slightly more trickier than the general PFNMAP case. And only in-tree user of MIXEDMAP is xip and that too it only uses it for regular WB mapping. So, we thought we should fix the more common case first here.

Quoted 3 lines

With MIXEDMAP there is no way whether to distinguish whether insert_pfn Or insert_page was used while looking at VMA. We can probably use PFNMAP in addition to MIXEDMAP to indicate that, which will make things easier.

It's difficult because it can have either method for a single VMA, and a given address in the vma may even change over time (not with current code in kernel AFAIKS, but AXFS eventually might get to that point).

Quoted 5 lines
Quoted 2 lines

But, we are still looking at that and trying to understand the change implication.

OK: now I understand correctly. Getting PFNMAP working is an important first step. I agree.

Venki, a patch logistics sidenote: the final mm/* bits of this patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot just queue them up in the x86/pat tree without agreement from MM maintainers.

Ingo

author
#11 lore raw

RE: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 70 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Ingo Molnar
Sent: Tuesday, November 18, 2008 1:38 PM
To: Nick Piggin
Cc: Pallipadi, Venkatesh; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 11 lines · Nick Piggin
Quoted 10 lines · Venkatesh
Quoted 10 lines

-----Original Message-----
From: Nick Piggin [mailto:[email protected]]
Sent: Thursday, November 13, 2008 6:06 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in

vm_insert_pfn

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

On Thu, Nov 13, 2008 at 10:47:23AM -0800, Pallipadi,

Quoted 4 lines · Venkatesh
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

Yes. It does. But, it calls a lower level insert_pfn()

function. The lower

Quoted 1 line

level insert_pfn() does not have any bug checks. But

the higher level

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

vm_insert_pfn() checks for PFNMAP or MIXEDMAP.

Yes, but is there anything extra you need to check for

cache aliases in

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

MIXEDMAP mappings?

Yes. We need additional things to track MIXEDMAP and we

are looking at that.

Quoted 1 line
Quoted 1 line

But, that is slightly more trickier than the general

PFNMAP case. And

Quoted 1 line
Quoted 1 line

only in-tree user of MIXEDMAP is xip and that too it only

uses it for

Quoted 1 line
Quoted 1 line

regular WB mapping. So, we thought we should fix the more

common case

Quoted 3 lines
Quoted 3 lines

first here.

With MIXEDMAP there is no way whether to distinguish

whether insert_pfn

Quoted 1 line
Quoted 1 line

Or insert_page was used while looking at VMA. We can

probably use PFNMAP

Quoted 1 line
Quoted 1 line

in addition to MIXEDMAP to indicate that, which will make

things easier.

Quoted 2 lines

It's difficult because it can have either method for a

single VMA, and

Quoted 1 line

a given address in the vma may even change over time (not

with current

Quoted 4 lines

code in kernel AFAIKS, but AXFS eventually might get to that point).

Quoted 1 line

But, we are still looking at that and trying to understand

the change

Quoted 2 lines
Quoted 1 line

implication.

Quoted 9 lines
Quoted 1 line

OK: now I understand correctly. Getting PFNMAP working is an

important

Quoted 1 line

first step. I agree.

Venki, a patch logistics sidenote: the final mm/* bits of this patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot just queue them up in the x86/pat tree without agreement from MM maintainers.

OK. Feedback based on the discussions on this patchset, this particular patch set VM_PFNMAP flag in vm_insert_pfn
is not really needed and we should rather BUG_ON on that not being set. And there were few more comments on one other generic vm patch from Nick.

I will resend the entire patchset with changes soon and then we can work out the logistics.

Thanks,
Venki

#12 lore raw

Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 18 lines · Venkatesh
Quoted 13 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Ingo Molnar
Sent: Tuesday, November 18, 2008 1:38 PM
To: Nick Piggin
Cc: Pallipadi, Venkatesh; Thomas Gleixner; H Peter Anvin; Hugh Dickins; Roland Dreier; Jesse Barnes; Jeremy Fitzhardinge; Arjan van de Ven; [email protected]; Siddha, Suresh B
Subject: Re: [patch 2/8] x86 PAT: set VM_PFNMAP flag in vm_insert_pfn

Quoted 4 lines

patchset need acks from MM folks - Andrew, Nick or Hugh - we cannot just queue them up in the x86/pat tree without agreement from MM maintainers.

Quoted 7 lines

OK. Feedback based on the discussions on this patchset, this particular patch set VM_PFNMAP flag in vm_insert_pfn
is not really needed and we should rather BUG_ON on that not being set. And there were few more comments on one other generic vm patch from Nick.

I will resend the entire patchset with changes soon and then we can work out the logistics.

So long as it has been reviewed by mm people (cc'ing linux-mm might be nice), I don't care so much how it makes its way upstream. If it was a lot of code, it might make sense to slowly work dependencies though various subsystems... but this isn't too much on the mm side, and it is primarily support code for x86 subsystem feature so I don't mind myself if it went via the x86 tree, with appropriate acks.

maintainer
#18 lore raw

Re: [patch 4/8] x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

In reply to Venkatesh Pallipadi
Quoted 17 lines · On Wed, 12 Nov 2008 13:26:51 -0800 Venkatesh Pallipadi…
mm/memory.c +11 -0
@@ -99,6 +99,28 @@ int randomize_va_space __read_mostly =
 					2;
 #endif
 
+#ifndef track_pfn_vma_new
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	return 0;
+}
+#endif
+
+#ifndef track_pfn_vma_copy
+int track_pfn_vma_copy(struct vm_area_struct *vma)
+{

mm/memory.c int randomize_va_space __read_mostly = · line 122

Quoted 10 lines
mm/memory.c +10 -0
+	return 0;
+}
+#endif
+
+#ifndef untrack_pfn_vma
+void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+			unsigned long size)
+{
+}
+#endif

Using __weak would provide a somewhat neater result here.

author
#19 lore raw

RE: [patch 4/8] x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

Quoted 38 lines

-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Tuesday, December 16, 2008 11:57 AM
To: Pallipadi, Venkatesh
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Pallipadi, Venkatesh; Siddha, Suresh B
Subject: Re: [patch 4/8] x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

Quoted 21 lines · On Wed, 12 Nov 2008 13:26:51 -0800 Venkatesh Pallipadi…
mm/memory.c +15 -0
@@ -99,6 +99,28 @@ int randomize_va_space __read_mostly =
 					2;
 #endif
 
+#ifndef track_pfn_vma_new
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	return 0;
+}
+#endif
+
+#ifndef track_pfn_vma_copy
+int track_pfn_vma_copy(struct vm_area_struct *vma)
+{
+	return 0;
+}
+#endif
+
Quoted 9 lines

mm/memory.c int randomize_va_space __read_mostly = · line 122

Quoted 6 lines
mm/memory.c +6 -0
+#ifndef untrack_pfn_vma
+void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+			unsigned long size)
+{
+}
+#endif

Using __weak would provide a somewhat neater result here.

Thought about that. But, then remembered the issues with gcc versions and __weak, as in here
http://lkml.org/lkml/2008/5/1/368

and decided to take the safer approach.

Thanks,
Venki

maintainer
#22 lore raw

Re: [patch 5/8] x86 PAT: Implement track/untrack of pfnmap regions for x86

In reply to Venkatesh Pallipadi
Quoted 21 lines · On Wed, 12 Nov 2008 13:26:52 -0800 Venkatesh Pallipadi…

Hookup remap_pfn_range and vm_insert_pfn and corresponding copy and free routines with reserve and free tracking.

reserve and free here only takes care of non RAM region mapping. For RAM region, driver should use set_memory_[uc|wc|wb] to set the cache type and then setup the mapping for user pte. We can bypass below reserve/free in that case.

Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Suresh Siddha <[email protected]>
---
 arch/x86/include/asm/pgtable.h |   10 ++
 arch/x86/mm/pat.c              |  201 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 211 insertions(+)

Index: tip/arch/x86/mm/pat.c
===================================================================

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 599

Quoted 4 lines
arch/x86/mm/pat.c +1 -0
@@ -596,6 +596,207 @@ void unmap_devmem(unsigned long pfn, uns
 	free_memtype(addr, addr + size);
 }
 
+int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t vma_prot)

I don't think the kernel is improved by leaving all these things undocumented.

These are interfaces which other architectures might choose to implement, so a few words explaining why they exist, what they are supposed to do, etc wouldn't hurt.

It would certainly help code reviewers.

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 605

Quoted 6 lines
arch/x86/mm/pat.c +6 -0
+{
+	unsigned long flags;
+	unsigned long want_flags = (pgprot_val(vma_prot) & _PAGE_CACHE_MASK);
+
+	int is_ram = 0;
+	int id_sz, ret;

The newline in the middle of the definition of locals makes no sense.

Quoted 9 lines
arch/x86/mm/pat.c +9 -0
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+
+	if (is_ram != 0) {
+		/*
+		 * For mapping RAM pages, drivers need to call
+		 * set_memory_[uc|wc|wb] directly, for reserve and free, before
+		 * setting up the PTE.
+		 */
+		WARN_ON_ONCE(1);

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 625

Quoted 10 lines
arch/x86/mm/pat.c +10 -0
+		return 0;
+	}
+
+	ret = reserve_memtype(paddr, paddr + size, want_flags, &flags);
+	if (ret)
+		return ret;
+
+	if (flags != want_flags) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO

Would KERN_ERR be more appropriate?

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 626

Quoted 2 lines
arch/x86/mm/pat.c +2 -0
+		"%s:%d map pfn expected mapping type %s for %Lx-%Lx, got %s\n",
+			current->comm, current->pid,

get_task_comm() is more reliable, but it hardly matters here.

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 629

Quoted 2 lines
arch/x86/mm/pat.c +2 -0
+			cattr_name(want_flags),
+			paddr, (unsigned long long)(paddr + size),

Printing the u64 paddr without a cast is OK in arch code (we know that u64 is implemented as unsigned long long). But one might choose to cast it anyway, to set a good example.

Or one could not bother. But this code does it both ways!

Quoted 5 lines
arch/x86/mm/pat.c +5 -0
+			cattr_name(flags));
+		return -EINVAL;
+	}
+
+	/* Need to keep identity mapping in sync */

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 644

Quoted 10 lines
arch/x86/mm/pat.c +10 -0
+	if (paddr >= __pa(high_memory))
+		return 0;
+
+	id_sz = (__pa(high_memory) < paddr + size) ?
+				__pa(high_memory) - paddr :
+				size;
+
+	if (ioremap_change_attr((unsigned long)__va(paddr), id_sz, flags) < 0) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO

KERN_ERR?

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 649

Quoted 5 lines
arch/x86/mm/pat.c +5 -0
+			"%s:%d reserve_pfn_range ioremap_change_attr failed %s "
+			"for %Lx-%Lx\n",
+			current->comm, current->pid,
+			cattr_name(flags),
+			paddr, (unsigned long long)(paddr + size));

ditto

Quoted 41 lines
arch/x86/mm/pat.c +38 -0
+		return -EINVAL;
+	}
+	return 0;
+}
+
+void free_pfn_range(u64 paddr, unsigned long size)
+{
+	int is_ram;
+
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+	if (is_ram == 0)
+		free_memtype(paddr, paddr + size);
+}
 
...
 
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	int retval = 0;
+	unsigned long i, j;
+	u64 base_paddr;
+	u64 paddr;
+	unsigned long vma_start = vma->vm_start;
+	unsigned long vma_end = vma->vm_end;
+	unsigned long vma_size = vma_end - vma_start;
+
+	if (!pat_enabled)
+		return 0;
+
+	if (is_linear_pfn_mapping(vma)) {
+		/* reserve the whole chunk starting from vm_pgoff */
+		paddr = (u64)vma->vm_pgoff << PAGE_SHIFT;
+		return reserve_pfn_range(paddr, vma_size, prot);
+	}
+
+	/* reserve page by page using pfn and size */
+	base_paddr = (u64)pfn << PAGE_SHIFT;
+	for (i = 0; i < size; i += PAGE_SIZE) {
+		paddr = base_paddr + i;
+		retval = reserve_pfn_range(paddr, PAGE_SIZE, prot);

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 752

Quoted 10 lines
arch/x86/mm/pat.c +10 -0
+		if (retval)
+			goto cleanup_ret;
+	}
+	return 0;
+
+cleanup_ret:
+	/* Reserve error: Cleanup partial reservation and return error */
+	for (j = 0; j < i; j += PAGE_SIZE) {
+		paddr = base_paddr + j;
+		free_pfn_range(paddr, PAGE_SIZE);

Could we simply do

	free_pfn_range(base_paddr, i * PAGE_SIZE);

here?

If not, then perhaps add a helper function to do this, because that loop gets repeated in several places.

Quoted 8 lines
arch/x86/mm/pat.c +5 -0
+	}
+
+	return retval;
+}
+
 
...
maintainer
#20 lore raw

Re: [patch 4/8] x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

In reply to Pallipadi, Venkatesh
Quoted 13 lines · Venkatesh
Quoted 13 lines

-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Tuesday, December 16, 2008 11:57 AM
To: Pallipadi, Venkatesh
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Pallipadi, Venkatesh; Siddha, Suresh B
Subject: Re: [patch 4/8] x86 PAT: hooks in generic vm code to help archs to track pfnmap regions

Your emails are getting mucked up.

Quoted 34 lines
Quoted 24 lines
Quoted 21 lines · On Wed, 12 Nov 2008 13:26:51 -0800 Venkatesh Pallipadi…
mm/memory.c +15 -0
@@ -99,6 +99,28 @@ int randomize_va_space __read_mostly =
 					2;
 #endif
 
+#ifndef track_pfn_vma_new
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	return 0;
+}
+#endif
+
+#ifndef track_pfn_vma_copy
+int track_pfn_vma_copy(struct vm_area_struct *vma)
+{
+	return 0;
+}
+#endif
+
Quoted 9 lines

mm/memory.c int randomize_va_space __read_mostly = · line 122

Quoted 6 lines
mm/memory.c +6 -0
+#ifndef untrack_pfn_vma
+void untrack_pfn_vma(struct vm_area_struct *vma, unsigned long pfn,
+			unsigned long size)
+{
+}
+#endif

Using __weak would provide a somewhat neater result here.

Quoted 5 lines

Thought about that. But, then remembered the issues with gcc versions and __weak, as in here
http://lkml.org/lkml/2008/5/1/368

and decided to take the safer approach.

Yes, it is a concern.

author
#23 lore raw

Re: [patch 5/8] x86 PAT: Implement track/untrack of pfnmap regions for x86

In reply to Andrew Morton
Quoted 17 lines · Andrew Morton

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 599

Quoted 11 lines · Venkatesh Pallipadi
 2 files changed, 211 insertions(+)

Index: tip/arch/x86/mm/pat.c
===================================================================

arch/x86/mm/pat.c +1 -0
@@ -596,6 +596,207 @@ void unmap_devmem(unsigned long pfn, uns
 	free_memtype(addr, addr + size);
 }
 
+int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t vma_prot)

I don't think the kernel is improved by leaving all these things undocumented.

Quoted 5 lines

These are interfaces which other architectures might choose to implement, so a few words explaining why they exist, what they are supposed to do, etc wouldn't hurt.

It would certainly help code reviewers.

Agreed. Will add documentation to the new functions here and update the patch.

Quoted 9 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 605

Quoted 7 lines
arch/x86/mm/pat.c +6 -0
+{
+	unsigned long flags;
+	unsigned long want_flags = (pgprot_val(vma_prot) & _PAGE_CACHE_MASK);
+
+	int is_ram = 0;
+	int id_sz, ret;

The newline in the middle of the definition of locals makes no sense.

Will change.

Quoted 13 lines
Quoted 13 lines
arch/x86/mm/pat.c +13 -0
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+
+	if (is_ram != 0) {
+		/*
+		 * For mapping RAM pages, drivers need to call
+		 * set_memory_[uc|wc|wb] directly, for reserve and free, before
+		 * setting up the PTE.
+		 */
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	ret = reserve_memtype(paddr, paddr + size, want_flags, &flags);
Quoted 8 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 625

Quoted 6 lines
arch/x86/mm/pat.c +6 -0
+	if (ret)
+		return ret;
+
+	if (flags != want_flags) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO

Would KERN_ERR be more appropriate?

OK.

Quoted 5 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 626

Quoted 3 lines
arch/x86/mm/pat.c +2 -0
+		"%s:%d map pfn expected mapping type %s for %Lx-%Lx, got %s\n",
+			current->comm, current->pid,

get_task_comm() is more reliable, but it hardly matters here.

current->comm was used at other places in this file. I should probably change all of them as a separate cleanup patch.

Quoted 10 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 629

Quoted 3 lines
arch/x86/mm/pat.c +2 -0
+			cattr_name(want_flags),
+			paddr, (unsigned long long)(paddr + size),

Printing the u64 paddr without a cast is OK in arch code (we know that u64 is implemented as unsigned long long). But one might choose to cast it anyway, to set a good example.

Or one could not bother. But this code does it both ways!

Will change.

Quoted 9 lines
Quoted 9 lines
arch/x86/mm/pat.c +9 -0
+			cattr_name(flags));
+		return -EINVAL;
+	}
+
+	/* Need to keep identity mapping in sync */
+	if (paddr >= __pa(high_memory))
+		return 0;
+
+	id_sz = (__pa(high_memory) < paddr + size) ?
Quoted 8 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 644

Quoted 6 lines
arch/x86/mm/pat.c +6 -0
+				__pa(high_memory) - paddr :
+				size;
+
+	if (ioremap_change_attr((unsigned long)__va(paddr), id_sz, flags) < 0) {
+		free_memtype(paddr, paddr + size);
+		printk(KERN_INFO

KERN_ERR?

OK.

Quoted 8 lines

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 649

Quoted 6 lines
arch/x86/mm/pat.c +5 -0
+			"%s:%d reserve_pfn_range ioremap_change_attr failed %s "
+			"for %Lx-%Lx\n",
+			current->comm, current->pid,
+			cattr_name(flags),
+			paddr, (unsigned long long)(paddr + size));

ditto

Will do.

Quoted 55 lines
Quoted 42 lines
arch/x86/mm/pat.c +38 -0
+		return -EINVAL;
+	}
+	return 0;
+}
+
+void free_pfn_range(u64 paddr, unsigned long size)
+{
+	int is_ram;
+
+	is_ram = pagerange_is_ram(paddr, paddr + size);
+	if (is_ram == 0)
+		free_memtype(paddr, paddr + size);
+}
 
...
 
+int track_pfn_vma_new(struct vm_area_struct *vma, pgprot_t prot,
+			unsigned long pfn, unsigned long size)
+{
+	int retval = 0;
+	unsigned long i, j;
+	u64 base_paddr;
+	u64 paddr;
+	unsigned long vma_start = vma->vm_start;
+	unsigned long vma_end = vma->vm_end;
+	unsigned long vma_size = vma_end - vma_start;
+
+	if (!pat_enabled)
+		return 0;
+
+	if (is_linear_pfn_mapping(vma)) {
+		/* reserve the whole chunk starting from vm_pgoff */
+		paddr = (u64)vma->vm_pgoff << PAGE_SHIFT;
+		return reserve_pfn_range(paddr, vma_size, prot);
+	}
+
+	/* reserve page by page using pfn and size */
+	base_paddr = (u64)pfn << PAGE_SHIFT;
+	for (i = 0; i < size; i += PAGE_SIZE) {
+		paddr = base_paddr + i;
+		retval = reserve_pfn_range(paddr, PAGE_SIZE, prot);

arch/x86/mm/pat.c void unmap_devmem(unsigned long pfn, uns · line 752

Quoted 10 lines
arch/x86/mm/pat.c +10 -0
+		if (retval)
+			goto cleanup_ret;
+	}
+	return 0;
+
+cleanup_ret:
+	/* Reserve error: Cleanup partial reservation and return error */
+	for (j = 0; j < i; j += PAGE_SIZE) {
+		paddr = base_paddr + j;
+		free_pfn_range(paddr, PAGE_SIZE);

Could we simply do

Quoted 7 lines
	free_pfn_range(base_paddr, i * PAGE_SIZE);

here?

If not, then perhaps add a helper function to do this, because that loop gets repeated in several places.

free_pfn_range(base_paddr, i * PAGE_SIZE);

will not work as free should correspond to reserve granularity. I agree making this a function should make this cleaner. Will do that in the refresh of this patch.

Thanks,
Venki