Messages in this thread | | | Date | Tue, 16 Dec 2008 12:07:59 -0800 | From | Andrew Morton <> | Subject | Re: [patch 5/8] x86 PAT: Implement track/untrack of pfnmap regions for x86 |
| |
On Wed, 12 Nov 2008 13:26:52 -0800 Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> wrote:
> 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 <venkatesh.pallipadi@intel.com> > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> > > --- > 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 > =================================================================== > --- tip.orig/arch/x86/mm/pat.c 2008-11-12 09:55:55.000000000 -0800 > +++ tip/arch/x86/mm/pat.c 2008-11-12 12:06:50.000000000 -0800 > @@ -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.
> +{ > + 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.
> + 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
Would KERN_ERR be more appropriate?
> + "%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.
> + 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!
> + 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
KERN_ERR?
> + "%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
> + 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); > + 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.
> + } > + > + return retval; > +} > + > > ... >
| |