[PATCH] Xen/i386 cleanups - io_remap_pfn_range

[PATCH] Xen/i386 cleanups - io_remap_pfn_range

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

This patch introduces a new interface function for mapping bus/device
memory: io_remap_pfn_range. This accepts the same parameters as
remap_pfn_range (indeed, by default it is implemented by this existing
function) but should be used in any situation where the caller is not
simply remapping ordinary RAM. For example, when mapping device
registers the new function should be used. 

The distinction between remapping device memory and ordinary RAM is
critical for the Xen hypervisor. This may also serve as a starting
point for cleaning up remaining users of io_remap_page_range (in
particular, the several sparc-specific sections in various drivers
that use a special form of io_remap_page_range).

I have audited the drivers/ and sound/ directories. Most uses of
remap_pfn_range are okay, but there are a small handful that are
remapping device memory (mostly AGP and DRM drivers).

Of particular driver is the HPET driver, whose mmap function is broken
even for native (non-Xen) builds. If nothing else, vmalloc_to_phys
should be used instead of __pa to convert an ioremapped virtual
address to a valid physical address. The fix in this patch is to
remember the original bus address as probed at boot time and to pass
this to io_remap_pfn_range.

Signed-off-by: Keir Fraser <keir@xensource.com>


[-- Attachment #2: iomap patch --]
[-- Type: text/plain, Size: 4291 bytes --]

--- linux-2.6-old/drivers/char/agp/frontend.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/frontend.c	2005-03-16 10:34:58 +00:00
@@ -628,7 +628,7 @@
 		DBG("client vm_ops=%p", kerninfo.vm_ops);
 		if (kerninfo.vm_ops) {
 			vma->vm_ops = kerninfo.vm_ops;
-		} else if (remap_pfn_range(vma, vma->vm_start,
+		} else if (io_remap_pfn_range(vma, vma->vm_start,
 				(kerninfo.aper_base + offset) >> PAGE_SHIFT,
 					    size, vma->vm_page_prot)) {
 			goto out_again;
@@ -644,7 +644,7 @@
 		DBG("controller vm_ops=%p", kerninfo.vm_ops);
 		if (kerninfo.vm_ops) {
 			vma->vm_ops = kerninfo.vm_ops;
-		} else if (remap_pfn_range(vma, vma->vm_start,
+		} else if (io_remap_pfn_range(vma, vma->vm_start,
 					    kerninfo.aper_base >> PAGE_SHIFT,
 					    size, vma->vm_page_prot)) {
 			goto out_again;
--- linux-2.6-old/drivers/char/drm/drm_vm.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/drm/drm_vm.c	2005-03-16 10:34:58 +00:00
@@ -630,7 +630,7 @@
 					vma->vm_end - vma->vm_start,
 					vma->vm_page_prot, 0))
 #else
-		if (remap_pfn_range(DRM_RPR_ARG(vma) vma->vm_start,
+		if (io_remap_pfn_range(vma, vma->vm_start,
 				     (VM_OFFSET(vma) + offset) >> PAGE_SHIFT,
 				     vma->vm_end - vma->vm_start,
 				     vma->vm_page_prot))
--- linux-2.6-old/drivers/char/drm/i810_dma.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/drm/i810_dma.c	2005-03-16 10:34:58 +00:00
@@ -119,7 +119,7 @@
    	buf_priv->currently_mapped = I810_BUF_MAPPED;
 	unlock_kernel();
 
-	if (remap_pfn_range(DRM_RPR_ARG(vma) vma->vm_start,
+	if (io_remap_pfn_range(vma, vma->vm_start,
 			     VM_OFFSET(vma) >> PAGE_SHIFT,
 			     vma->vm_end - vma->vm_start,
 			     vma->vm_page_prot)) return -EAGAIN;
--- linux-2.6-old/drivers/char/drm/i830_dma.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/drm/i830_dma.c	2005-03-16 10:34:58 +00:00
@@ -121,7 +121,7 @@
    	buf_priv->currently_mapped = I830_BUF_MAPPED;
 	unlock_kernel();
 
-	if (remap_pfn_range(DRM_RPR_ARG(vma) vma->vm_start,
+	if (io_remap_pfn_range(vma, vma->vm_start,
 			     VM_OFFSET(vma) >> PAGE_SHIFT,
 			     vma->vm_end - vma->vm_start,
 			     vma->vm_page_prot)) return -EAGAIN;
--- linux-2.6-old/drivers/char/hpet.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/hpet.c	2005-03-16 10:34:58 +00:00
@@ -76,6 +76,7 @@
 struct hpets {
 	struct hpets *hp_next;
 	struct hpet __iomem *hp_hpet;
+	unsigned long hp_hpet_phys;
 	struct time_interpolator *hp_interpolator;
 	unsigned long hp_period;
 	unsigned long hp_delta;
@@ -265,7 +266,7 @@
 		return -EINVAL;
 
 	devp = file->private_data;
-	addr = (unsigned long)devp->hd_hpet;
+	addr = devp->hd_hpets->hp_hpet_phys;
 
 	if (addr & (PAGE_SIZE - 1))
 		return -ENOSYS;
@@ -274,7 +275,7 @@
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	addr = __pa(addr);
 
-	if (remap_pfn_range(vma, vma->vm_start, addr >> PAGE_SHIFT,
+	if (io_remap_pfn_range(vma, vma->vm_start, addr >> PAGE_SHIFT,
 					PAGE_SIZE, vma->vm_page_prot)) {
 		printk(KERN_ERR "remap_pfn_range failed in hpet.c\n");
 		return -EAGAIN;
@@ -795,6 +796,7 @@
 
 	hpetp->hp_which = hpet_nhpet++;
 	hpetp->hp_hpet = hdp->hd_address;
+	hpetp->hp_hpet_phys = hdp->hd_phys_address;
 
 	hpetp->hp_ntimer = hdp->hd_nirqs;
 
--- linux-2.6-old/drivers/sbus/char/flash.c	2005-03-16 10:30:37 +00:00
+++ linux-2.6-new/drivers/sbus/char/flash.c	2005-03-16 10:34:58 +00:00
@@ -75,7 +75,7 @@
 	pgprot_val(vma->vm_page_prot) |= _PAGE_E;
 	vma->vm_flags |= (VM_SHM | VM_LOCKED);
 
-	if (remap_pfn_range(vma, vma->vm_start, addr, size, vma->vm_page_prot))
+	if (io_remap_pfn_range(vma, vma->vm_start, addr, size, vma->vm_page_prot))
 		return -EAGAIN;
 		
 	return 0;
--- linux-2.6-old/include/linux/mm.h	2005-03-16 10:31:07 +00:00
+++ linux-2.6-new/include/linux/mm.h	2005-03-16 10:34:58 +00:00
@@ -815,6 +815,10 @@
 extern int check_user_page_readable(struct mm_struct *mm, unsigned long address);
 int remap_pfn_range(struct vm_area_struct *, unsigned long,
 		unsigned long, unsigned long, pgprot_t);
+/* Allow arch override for mapping of device and I/O (non-RAM) pages. */
+#ifndef io_remap_pfn_range
+#define io_remap_pfn_range remap_pfn_range
+#endif
 
 #ifdef CONFIG_PROC_FS
 void __vm_stat_account(struct mm_struct *, unsigned long, struct file *, long);

Re: [PATCH] Xen/i386 cleanups - io_remap_pfn_range

[Randy.Dunlap]

Keir Fraser wrote:
> This patch introduces a new interface function for mapping bus/device
> memory: io_remap_pfn_range. This accepts the same parameters as
> remap_pfn_range (indeed, by default it is implemented by this existing
> function) but should be used in any situation where the caller is not
> simply remapping ordinary RAM. For example, when mapping device
> registers the new function should be used. 
> 
> The distinction between remapping device memory and ordinary RAM is
> critical for the Xen hypervisor. This may also serve as a starting
> point for cleaning up remaining users of io_remap_page_range (in
> particular, the several sparc-specific sections in various drivers
> that use a special form of io_remap_page_range).
> 
> I have audited the drivers/ and sound/ directories. Most uses of
> remap_pfn_range are okay, but there are a small handful that are
> remapping device memory (mostly AGP and DRM drivers).
> 
> Of particular driver is the HPET driver, whose mmap function is broken
> even for native (non-Xen) builds. If nothing else, vmalloc_to_phys
> should be used instead of __pa to convert an ioremapped virtual
> address to a valid physical address. The fix in this patch is to
> remember the original bus address as probed at boot time and to pass
> this to io_remap_pfn_range.

I'll look over this shortly.  I posted an io_remap_pfn_range() patch
to the linux-mm mailing list last week:
http://marc.theaimsgroup.com/?l=linux-ultrasparc&m=111049550001120&w=2

One of the things that it needs to take into account is that
io_remap_page_range() on sparc/sparc64 has 6 parameters instead of 5.
My patch tries to remedy that...

-- 
~Randy

Re: [PATCH] Xen/i386 cleanups - io_remap_pfn_range

[Randy.Dunlap]

Keir Fraser wrote:
> This patch introduces a new interface function for mapping bus/device
> memory: io_remap_pfn_range. This accepts the same parameters as
> remap_pfn_range (indeed, by default it is implemented by this existing
> function) but should be used in any situation where the caller is not
> simply remapping ordinary RAM. For example, when mapping device
> registers the new function should be used. 
> 
> The distinction between remapping device memory and ordinary RAM is
> critical for the Xen hypervisor. This may also serve as a starting
> point for cleaning up remaining users of io_remap_page_range (in
> particular, the several sparc-specific sections in various drivers
> that use a special form of io_remap_page_range).
> 
> I have audited the drivers/ and sound/ directories. Most uses of
> remap_pfn_range are okay, but there are a small handful that are
> remapping device memory (mostly AGP and DRM drivers).
> 
> Of particular driver is the HPET driver, whose mmap function is broken
> even for native (non-Xen) builds. If nothing else, vmalloc_to_phys
> should be used instead of __pa to convert an ioremapped virtual
> address to a valid physical address. The fix in this patch is to
> remember the original bus address as probed at boot time and to pass
> this to io_remap_pfn_range.
> 
> Signed-off-by: Keir Fraser <keir@xensource.com>

Hi-

Our io_remap_pfn_range() patches don't contain many collisions.
My first patch [adding io_remap_pfn_range() to all arches]
<http://marc.theaimsgroup.com/?l=linux-mm&m=111049473410099&w=2>
does go a little further than yours in that regard.

Also, I was under the impression (only, so this is a question)
that this type of construct (from your patch):

+#ifndef io_remap_pfn_range
+#define io_remap_pfn_range remap_pfn_range
+#endif

only works for #defines (macros), while in some arches
io_remap_page_range() (and presumably io_remap_pfn_range)
is a function [sparc32/64] or inline function [mips].

My first patch referenced a future patch to convert
all callers of io_remap_page_range() to io_remap_pfn_range(),
which I have now done and built succesfully on 8 arches.
I'll post it now.

-- 
~Randy

Re: [PATCH] Xen/i386 cleanups - io_remap_pfn_range

[Keir Fraser]

On 17 Mar 2005, at 04:41, Randy.Dunlap wrote:

> Our io_remap_pfn_range() patches don't contain many collisions.
> My first patch [adding io_remap_pfn_range() to all arches]
> <http://marc.theaimsgroup.com/?l=linux-mm&m=111049473410099&w=2>
> does go a little further than yours in that regard.
>
> Also, I was under the impression (only, so this is a question)
> that this type of construct (from your patch):
>
> +#ifndef io_remap_pfn_range
> +#define io_remap_pfn_range remap_pfn_range
> +#endif
>
> only works for #defines (macros), while in some arches
> io_remap_page_range() (and presumably io_remap_pfn_range)
> is a function [sparc32/64] or inline function [mips].
>
> My first patch referenced a future patch to convert
> all callers of io_remap_page_range() to io_remap_pfn_range(),
> which I have now done and built succesfully on 8 arches.
> I'll post it now.

The way in which you introduce io_remap_pfn_range() into all 
architectures is much better than my method, and doesn't depend on 
io_remap_pfn_range being a macro.
Apart from that, yes: our driver patches are quite disjoint and 
complement each other.
Hopefully a combined patch could eliminate some of the 'ifdef sparc's 
that are scattered around. :-)

  -- Keir