Status on ioremap patch

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Steffen Persvold

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines · Steffen Persvold

Steffen> So, how about that patch guys ? Any chance you'll put it Steffen> into the mainstream IA64 kernel ???

I haven't looked at it yet, but am planning to do so as part of the 2.4.10 patch.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Steffen" = Steffen Persvold <[email protected]> writes:

Quoted 2 lines

I haven't looked at it yet, but am planning to do so as part of
the 2.4.10 patch.

Steffen,

I looked at your patch and have a comment and a suggestion. First, I do not think it's safe to define ioremap() as a way to install write-cached mappings. There are plenty of drivers left that use ioremap() instead of ioremap_nocache() and we can't change the semantics of ioremap() underneath them.

Second, it seems to me it would be cleaner to extend remap_page_range() to allow remapping into the kernel mapped segment (region 5 in the case of ia64 linux). This should work as is, except that in the kernel case, you need to use pgd_offset_k(). I think you could hack mm/memory.c to check whether "from" is in the range from VMALLOC_START to VMALLOC_END and, if so, use pgd_offset_k() instead of pgd_offset(). Can you try this and let me know if it works for your needs?

mm/memory.c Sat Jan 5 13:13:26 2002 line 796

Quoted 9 lines · Steffen Persvold

mm/memory.c Sat Jan 5 13:13:26 2002 renamed from mm/memory.c.orig Fri Dec 21 18:42:05 2001 +3 -0

@@ -791,6 +791,9 @@
 * maps a range of physical memory into the requested pages. the old
 * mappings are removed. any references to nonexistent pages results
 * in null mappings (currently treated as "copy-on-access")
+ *
+ * For physical (or I/O) memory mapped into the kernel virtual space,
+ * the old mappings will not be removed.

That comment worries me. If you have multiple mappings for the same page then you may have problems on hardware that uses virtually indexed caches. Two virtual addresses could map to the same physical page but index to different cache lines, destroying cache coherency. How do you prevent that?

Quoted 19 lines · Keith Owens

mm/memory.c Sat Jan 5 13:13:26 2002 line 796

Quoted 9 lines · Steffen Persvold

mm/memory.c Sat Jan 5 13:13:26 2002 renamed from mm/memory.c.orig Fri Dec 21 18:42:05 2001 +3 -0

@@ -791,6 +791,9 @@
 * maps a range of physical memory into the requested pages. the old
 * mappings are removed. any references to nonexistent pages results
 * in null mappings (currently treated as "copy-on-access")
+ *
+ * For physical (or I/O) memory mapped into the kernel virtual space,
+ * the old mappings will not be removed.

That comment worries me. If you have multiple mappings for the same page then you may have problems on hardware that uses virtually indexed caches. Two virtual addresses could map to the same physical page but index to different cache lines, destroying cache coherency. How do you prevent that?

Well, actually multiple mappings in kernel space is denied :

(quoted diff) +6 -0

+               if (address > VMALLOC_START && address < VMALLOC_END) {
+                       if (!pte_none(*pte)) {
+                               printk("remap_area_pte: page already exists\n");
+                               BUG();
+                       }
+                       set_pte(pte, mk_pte_phys(phys_addr, prot));

Quoted 29 lines · Steffen Persvold

Quoted 19 lines · Keith Owens

mm/memory.c Sat Jan 5 13:13:26 2002 line 796

Quoted 9 lines · Steffen Persvold

mm/memory.c Sat Jan 5 13:13:26 2002 renamed from mm/memory.c.orig Fri Dec 21 18:42:05 2001 +3 -0

@@ -791,6 +791,9 @@
 * maps a range of physical memory into the requested pages. the old
 * mappings are removed. any references to nonexistent pages results
 * in null mappings (currently treated as "copy-on-access")
+ *
+ * For physical (or I/O) memory mapped into the kernel virtual space,
+ * the old mappings will not be removed.

That comment worries me. If you have multiple mappings for the same page then you may have problems on hardware that uses virtually indexed caches. Two virtual addresses could map to the same physical page but index to different cache lines, destroying cache coherency. How do you prevent that?

Well, actually multiple mappings in kernel space is denied :

(quoted diff) +6 -0

+               if (address > VMALLOC_START && address < VMALLOC_END) {
+                       if (!pte_none(*pte)) {
+                               printk("remap_area_pte: page already exists\n");
+                               BUG();
+                       }
+                       set_pte(pte, mk_pte_phys(phys_addr, prot));

Review

Quoted 10 lines

(quoted diff) +10 -0

+               } else {
+                       struct page *page;
+                       pte_t oldpage;
+                       oldpage = ptep_get_and_clear(pte);
+
+                       page = virt_to_page(__va(phys_addr));
+                       if ((!VALID_PAGE(page)) || PageReserved(page))
+                               set_pte(pte, mk_pte_phys(phys_addr, prot));
+                       forget_pte(oldpage);
+               }

What about one mapping in kernel space and one in user space, or two mappings in user space? As long as you have more than one mapping to the same physical page you must handle virtual cache aliasing.

Quoted 9 lines · David Mosberger

Steffen,

I looked at your patch and have a comment and a suggestion. First, I do not think it's safe to define ioremap() as a way to install write-cached mappings. There are plenty of drivers left that use ioremap() instead of ioremap_nocache() and we can't change the semantics of ioremap() underneath them.

Quoted 9 lines

Second, it seems to me it would be cleaner to extend remap_page_range() to allow remapping into the kernel mapped segment (region 5 in the case of ia64 linux). This should work as is, except that in the kernel case, you need to use pgd_offset_k(). I think you could hack mm/memory.c to check whether "from" is in the range from VMALLOC_START to VMALLOC_END and, if so, use pgd_offset_k() instead of pgd_offset(). Can you try this and let me know if it works for your needs?

I've had some time to look at it, and I decided to do what you said. I also changed ioremap on all platforms that implements it so that they use remap_page_range()). I've tested the patch on IA64 and i386.

I've attached the patch (I didn't post it on the lkml even though it affects more that the IA64 platform), it's against a vanilla 2.4.17 tree.

Please take a look and tell me what you think.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Steffen Persvold