Skip to contentSkip to conversation

sh: trapped io support

PATCH In review Magnus Damm 2 participants 4 messages Feb 6, 2008 – Feb 7, 2008 SuperH

lore
Share & tools
lore

Magnus Damm

author
#0 lore raw

PATCH 1/1 sh: trapped io support

This patch adds trapped io support.

The idea is that we want to get rid of the in/out/readb/writeb callbacks from the machvec and replace that with simple inline read and write operations to memory. Fast and simple for most hardware devices (think pci).

Some devices require special treatment though - like 16-bit only CF devices - so we need to have some method to hook in callbacks.

This patch makes it possible to add a per-device trap generating filter. This way we can get maximum performance of sane hardware - which doesn't need this filter - and crappy hardware works but gets punished by a performance hit.

So build better hardware or use register_trapped_io_handler() if you must.

Signed-off-by Magnus Damm <[email protected]>

---

 arch/sh/Kconfig             |    3 
 arch/sh/kernel/Makefile_32  |    1 
 arch/sh/kernel/Makefile_64  |    1 
 arch/sh/kernel/io.c         |    8 +
 arch/sh/kernel/io_trapped.c |  274 +++++++++++++++++++++++++++++++++++++++++++
 arch/sh/kernel/traps_32.c   |   81 ++++++++----
 arch/sh/mm/fault_32.c       |    3 
 include/asm-sh/io.h         |    9 +
 include/asm-sh/io_trapped.h |   59 +++++++++
 include/asm-sh/system.h     |    5 
 include/asm-sh/system_32.h  |    3 
 11 files changed, 420 insertions(+), 27 deletions(-)
arch/sh/Kconfig +3 -0
@@ -324,6 +324,9 @@ source "arch/sh/Kconfig.cpu"
 
 menu "Board support"
 
+config IO_TRAPPED
+	bool
+
 config SOLUTION_ENGINE
 	bool
arch/sh/kernel/Makefile_32 +1 -0
@@ -22,5 +22,6 @@ obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_BINFMT_ELF)	+= dump_task.o
+obj-$(CONFIG_IO_TRAPPED)	+= io_trapped.o
 
 EXTRA_CFLAGS += -Werror
arch/sh/kernel/Makefile_64 +1 -0
@@ -18,5 +18,6 @@ obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_BINFMT_ELF)	+= dump_task.o
+obj-$(CONFIG_IO_TRAPPED)	+= io_trapped.o
 
 EXTRA_CFLAGS += -Werror
arch/sh/kernel/io.c +8 -0
@@ -63,6 +63,14 @@ EXPORT_SYMBOL(memset_io);
 
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
+	void __iomem *ret;
+
+	if (nr > 4) {
+		ret = ioremap_trapped(port, nr, IORESOURCE_IO);
+		if (ret)
+			return ret;
+	}
+
 	return sh_mv.mv_ioport_map(port, nr);
 }
 EXPORT_SYMBOL(ioport_map);
arch/sh/kernel/io_trapped.c new file +274 -0

274 lines not on this page · raw message

arch/sh/kernel/traps_32.c +54 -27

244 lines not on this page · raw message

arch/sh/mm/fault_32.c +3 -0
@@ -15,6 +15,7 @@
 #include <linux/mm.h>
 #include <linux/hardirq.h>
 #include <linux/kprobes.h>
+#include <asm/io_trapped.h>
 #include <asm/system.h>
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>

@@ -163,6 +164,8 @@ no_context:
 	if (fixup_exception(regs))
 		return;
 
+	if (handle_trapped_io(regs, address))
+		return;
 /*
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.
include/asm-sh/io.h +9 -0
@@ -38,6 +38,7 @@
  */
 #define __IO_PREFIX	generic
 #include <asm/io_generic.h>
+#include <asm/io_trapped.h>
 
 #define maybebadio(port) \
   printk(KERN_ERR "bad PC-like io %s:%u for port 0x%lx at 0x%08x\n", \

@@ -309,7 +310,15 @@ __ioremap_mode(unsigned long offset, uns
 {
 #ifdef CONFIG_SUPERH32
 	unsigned long last_addr = offset + size - 1;
+#endif
+	void __iomem *ret = NULL;
+
+	if (!(flags & _PAGE_CACHABLE))
+		ret = ioremap_trapped(offset, size, IORESOURCE_MEM);
+	if (ret)
+		return ret;
 
+#ifdef CONFIG_SUPERH32
 	/*
 	 * For P1 and P2 space this is trivial, as everything is already
 	 * mapped. Uncached access for P1 addresses are done through P2.
include/asm-sh/io_trapped.h new file +59 -0

59 lines not on this page · raw message

include/asm-sh/system.h +5 -0
@@ -182,6 +182,11 @@ BUILD_TRAP_HANDLER(fpu_state_restore);
 
 #define arch_align_stack(x) (x)
 
+struct mem_access {
+	int (*from)(void *dst, void *src, int cnt);
+	int (*to)(void *dst, void *src, int cnt);
+};
+
 #ifdef CONFIG_SUPERH32
 # include "system_32.h"
 #else
include/asm-sh/system_32.h +3 -0
@@ -96,4 +96,7 @@ do {							\
 		: "=&r" (__dummy));			\
 } while (0)
 
+int handle_unaligned_access(u16 instruction, struct pt_regs *regs,
+			    struct mem_access *ma);
+
 #endif /* __ASM_SH_SYSTEM_32_H */
#1 lore raw

arch/sh/Kconfig source "arch/sh/Kconfig.cpu" · line 332

Quoted 12 lines · Magnus Damm
arch/sh/Kconfig +3 -0
@@ -324,6 +324,9 @@ source "arch/sh/Kconfig.cpu"
 
 menu "Board support"
 
+config IO_TRAPPED
+	bool
+
 config SOLUTION_ENGINE
 	bool

This is a pretty bizarre place to put this symbol.

Quoted 7 lines
arch/sh/kernel/io.c +1 -0
@@ -63,6 +63,14 @@ EXPORT_SYMBOL(memset_io);
 
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
+	void __iomem *ret;

arch/sh/kernel/io.c EXPORT_SYMBOL(memset_io); · line 76

Quoted 10 lines
arch/sh/kernel/io.c +7 -0
+
+	if (nr > 4) {
+		ret = ioremap_trapped(port, nr, IORESOURCE_IO);
+		if (ret)
+			return ret;
+	}
+
 	return sh_mv.mv_ioport_map(port, nr);
 }
 EXPORT_SYMBOL(ioport_map);

SH-5 needs 64-bit I/O, as do some of the SH-5 blocks that have showed up on SH-4 parts (ie, the SH4-202 SuperHyway CVRs and so on). I don't know that there's any point in tying to hook this in here for unhandled sizes.

Quoted 20 lines
arch/sh/kernel/io.c +20 -0
+unsigned int trapped_ioread8(void __iomem *addr)
+{
+	return ctrl_inb((unsigned long __force)addr);
+}
+
+unsigned int trapped_ioread16(void __iomem *addr)
+{
+	return ctrl_inw((unsigned long __force)addr);
+}
+
+unsigned int trapped_ioread32(void __iomem *addr)
+{
+	return ctrl_inl((unsigned long __force)addr);
+}
+
+void trapped_iowrite8(unsigned int data, void __iomem *addr)
+{
+	ctrl_outb(data, (unsigned long __force)addr);
+}
+

arch/sh/kernel/io.c

Quoted 10 lines
arch/sh/kernel/io.c +10 -0
+void trapped_iowrite16(unsigned int data, void __iomem *addr)
+{
+	ctrl_outw(data, (unsigned long __force)addr);
+}
+
+void trapped_iowrite32(unsigned int data, void __iomem *addr)
+{
+	ctrl_outl(data, (unsigned long __force)addr);
+}
+

It would be nice to ioport_map() these directly and do away with the __force casting, as per generic I/O.

arch/sh/kernel/io.c

Quoted 2 lines
arch/sh/kernel/io.c +2 -0
+LIST_HEAD(trapped_io);
+LIST_HEAD(trapped_mem);

These should probably be conditionalized on CONFIG_HAS_IOPORT and CONFIG_HAS_IOMEM, so at least boards that don't have any use for the ioport stuff can save some space here.

arch/sh/kernel/io.c

Quoted 6 lines
arch/sh/kernel/io.c +6 -0
+static DEFINE_SPINLOCK(trapped_lock);
+
+#define TRAPPED_PAGES_MAX 16
+
+void __init register_trapped_io_handler(struct trapped_io *tiop)
+{

There are a lot of things that can fail in here, please make this an int and actually pass back the return value as opposed to just BUG'ing out at the first sign of trouble.

Perhaps it's worth splitting out the platform devices in to a set of trapped devices and non-trapped, then only registering the trapped ones when the trapped page setup succeeds. There's no reason to bring down the rest of the system because you can't trap non-standard writes to a mostly broken SuperIO.

arch/sh/kernel/io.c

Quoted 10 lines
arch/sh/kernel/io.c +10 -0
+static int from_device(void *dst, void *src, int cnt)
+{
+	struct trapped_io *tiop;
+	unsigned long addr = (unsigned long)src;
+	unsigned int tmp = 0;
+
+	tiop = lookup_tiop(addr);
+	BUG_ON(!tiop);
+	BUG_ON(tiop->magic != IO_TRAPPED_MAGIC);
+

Same deal here. You can in fact pass back an error code without BUG()'ing.

arch/sh/kernel/io.c

Quoted 5 lines
arch/sh/kernel/io.c +5 -0
+	addr = lookup_address(tiop, addr);
+#ifdef DEBUG
+	printk(KERN_DEBUG "trapped io read! 0x%08x, %d",
+	       (unsigned int)src, cnt);
+#endif

pr_debug().

Quoted 6 lines
arch/sh/kernel/io.c +6 -0
+	switch (cnt) {
+	case 1:
+		tmp = tiop->read8((void __iomem *)addr);
+		*(unsigned char *)dst = tmp & 0xff;
+		break;
+	case 2:

arch/sh/kernel/io.c

Quoted 10 lines
arch/sh/kernel/io.c +10 -0
+		tmp = tiop->read16((void __iomem *)addr);
+		*(unsigned short *)dst = tmp & 0xffff;
+		break;
+	case 4:
+		tmp = tiop->read32((void __iomem *)addr);
+		*(unsigned int *)dst = tmp;
+		break;
+	default:
+		BUG();
+	}

ctrl_outX() for these. The __iomem cast back and forth is also fairly ugly. Since you force an unsigned long to a void __iomem * here and then back to an unsigned long in the routines themselves. Grr.

arch/sh/kernel/io.c

Quoted 3 lines
arch/sh/kernel/io.c +3 -0
+#ifdef DEBUG
+	printk(" -> 0x%08x 0x%08x\n", (unsigned int)addr, (unsigned int)tmp);
+#endif

pr_debug().

Quoted 21 lines
arch/sh/kernel/io.c +21 -0
+static int to_device(void *dst, void *src, int cnt)
+{
+	struct trapped_io *tiop;
+	unsigned long addr = (unsigned long)dst;
+	unsigned int tmp = 0;
+
+	tiop = lookup_tiop(addr);
+	BUG_ON(!tiop);
+	BUG_ON(tiop->magic != IO_TRAPPED_MAGIC);
+
+	addr = lookup_address(tiop, addr);
+#ifdef DEBUG
+	printk(KERN_DEBUG "trapped io write! 0x%08x, %d",
+	       (unsigned int)dst, cnt);
+#endif
+	switch (cnt) {
+	case 1:
+		tmp = *(unsigned char *)src;
+		tiop->write8(tmp, (void __iomem *)addr);
+		break;
+	case 2:

arch/sh/kernel/io.c

Quoted 10 lines
arch/sh/kernel/io.c +10 -0
+		tmp = *(unsigned short *)src;
+		tiop->write16(tmp, (void __iomem *)addr);
+		break;
+	case 4:
+		tmp = *(unsigned int *)src;
+		tiop->write32(tmp, (void __iomem *)addr);
+		break;
+	default:
+		BUG();
+	}

ctrl_inX() here.

arch/sh/kernel/io.c

Quoted 3 lines
arch/sh/kernel/io.c +3 -0
+#ifdef DEBUG
+	printk("-> 0x%08x 0x%08x\n", (unsigned int)addr, (unsigned int)tmp);
+#endif

pr_debug().

arch/sh/kernel/io.c

Quoted 6 lines
arch/sh/kernel/io.c +6 -0
+int handle_trapped_io(struct pt_regs *regs, unsigned long address)
+{
+	mm_segment_t oldfs;
+	u16 instruction;
+	int tmp;
+

opcode_size_t.

arch/sh/kernel/io.c

Quoted 13 lines
arch/sh/kernel/io.c +13 -0
+	if (!lookup_tiop(address))
+		return 0;
+
+	BUG_ON(user_mode(regs));
+
+	oldfs = get_fs();
+	set_fs(KERNEL_DS);
+	if (copy_from_user(&instruction, (u16 *)(regs->pc), 2)) {
+		set_fs(oldfs);
+		BUG();
+		return 0;
+	}
+

opcode_size_t and instruction_size(). The BUG()'s here are also far too aggressive, neither one looks helpful.

arch/sh/kernel/io.c

Quoted 3 lines
arch/sh/kernel/io.c +3 -0
+	tmp = handle_unaligned_access(instruction, regs, &trapped_mem_access);
+	set_fs(oldfs);
+	return !tmp;

return tmp = 0 is a bit less ugly in these sorts of cases.

Quoted 19 lines
arch/sh/kernel/traps_32.c +7 -1
@@ -150,18 +150,43 @@ static int die_if_no_fixup(const char * 
 static inline void sign_extend(unsigned int count, unsigned char *dst)
 {
 #ifdef __LITTLE_ENDIAN__
+	if ((count = 1) && dst[0] & 0x80) {
+		dst[1] = 0xff;
+		dst[2] = 0xff;
+		dst[3] = 0xff;
+	}
 	if ((count = 2) && dst[1] & 0x80) {
 		dst[2] = 0xff;
 		dst[3] = 0xff;
 	}
 #else
-	if ((count = 2) && dst[2] & 0x80) {
+	if ((count = 1) && dst[3] & 0x80) {
+		dst[2] = 0xff;

arch/sh/kernel/traps_32.c static int die_if_no_fixup(const char * · line 174

Quoted 10 lines
arch/sh/kernel/traps_32.c +4 -0
+		dst[1] = 0xff;
 		dst[0] = 0xff;
+	}
+	if ((count = 2) && dst[2] & 0x80) {
 		dst[1] = 0xff;
+		dst[0] = 0xff;
 	}
 #endif
 }

Shouldn't this be a separate patch?

Quoted 5 lines
arch/sh/kernel/traps_32.c +5 -0
+static int from_user(void *dst, void *src, int cnt)
+{
+	return copy_from_user(dst, src, cnt);
+}
+

arch/sh/kernel/traps_32.c static int die_if_no_fixup(const char * · line 189

Quoted 10 lines
arch/sh/kernel/traps_32.c +10 -0
+static int to_user(void *dst, void *src, int cnt)
+{
+	return copy_to_user(dst, src, cnt);
+}
+
+static struct mem_access user_mem_access = {
+	from_user,
+	to_user,
+};
+

The argument ordering and everything in these is exactly the same, why don't you use copy_to_user()/copy_from_user() in your structure here instead of these no-op wrappers?

Quoted 11 lines
arch/sh/mm/fault_32.c +1 -0
@@ -15,6 +15,7 @@
 #include <linux/mm.h>
 #include <linux/hardirq.h>
 #include <linux/kprobes.h>
+#include <asm/io_trapped.h>
 #include <asm/system.h>
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>

arch/sh/mm/fault_32.c no_context: · line 171

Quoted 8 lines
arch/sh/mm/fault_32.c +2 -0
@@ -163,6 +164,8 @@ no_context:
 	if (fixup_exception(regs))
 		return;
 
+	if (handle_trapped_io(regs, address))
+		return;
 /*
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.

This looks like a good candidate for page fault notifiers..

Quoted 16 lines
include/asm-sh/io.h +3 -0
@@ -38,6 +38,7 @@
  */
 #define __IO_PREFIX	generic
 #include <asm/io_generic.h>
+#include <asm/io_trapped.h>
 
 #define maybebadio(port) \
   printk(KERN_ERR "bad PC-like io %s:%u for port 0x%lx at 0x%08x\n", \

@@ -309,7 +310,15 @@ __ioremap_mode(unsigned long offset, uns
 {
 #ifdef CONFIG_SUPERH32
 	unsigned long last_addr = offset + size - 1;
+#endif
+	void __iomem *ret = NULL;

include/asm-sh/io.h __ioremap_mode(unsigned long offset, uns · line 324

Quoted 10 lines
include/asm-sh/io.h +6 -0
+
+	if (!(flags & _PAGE_CACHABLE))
+		ret = ioremap_trapped(offset, size, IORESOURCE_MEM);
+	if (ret)
+		return ret;
 
+#ifdef CONFIG_SUPERH32
 	/*
 	 * For P1 and P2 space this is trivial, as everything is already
 	 * mapped. Uncached access for P1 addresses are done through P2.

This is also a bit of a mess. Why are you handing off IORESOURCE_MEM here when y ou don't know whether it's an IORESOURCE_MEM or IORESOURCE_IO? The pgprot validation here is also non-obvious, you may as well just unconditionally call ioremap_trapped() (or __ioremap_trapped(), as it shouuld be considered an internal API), and bail out early if it's happy.

Quoted 16 lines
include/asm-sh/io_trapped.h new file +13 -0
@@ -0,0 +1,59 @@
+#ifndef __ASM_SH_IO_TRAPPED_H
+#define __ASM_SH_IO_TRAPPED_H
+
+#include <linux/list.h>
+#include <linux/ioport.h>
+#include <asm/page.h>
+
+#define IO_TRAPPED_MAGIC 0xfeedbeef
+
+struct trapped_io {
+	unsigned int magic;
+	struct resource *resource;
+	unsigned int num_resources;

include/asm-sh/io_trapped.h line 23

Quoted 10 lines
include/asm-sh/io_trapped.h new file +10 -0
+	struct list_head list;
+	void __iomem *virt_base;
+	unsigned int (*read8)(void __iomem *addr);
+	unsigned int (*read16)(void __iomem *addr);
+	unsigned int (*read32)(void __iomem *addr);
+	void (*write8)(unsigned int data, void __iomem *addr);
+	void (*write16)(unsigned int data, void __iomem *addr);
+	void (*write32)(unsigned int data, void __iomem *addr);
+} __attribute__ ((aligned(PAGE_SIZE)));;
+

Should probably add 64-bit versions here also. Double ;'s, and this really wants a __page_aligned.

include/asm-sh/io_trapped.h line 35

Quoted 5 lines
include/asm-sh/io_trapped.h new file +5 -0
+#ifdef CONFIG_IO_TRAPPED
+void __init register_trapped_io_handler(struct trapped_io *tiop);
+int handle_trapped_io(struct pt_regs *regs, unsigned long address);
+#else
+#define register_trapped_io_handler(tiop) while (0)

do { } while (0) might even work!

include/asm-sh/io_trapped.h line 31

Quoted 3 lines
include/asm-sh/io_trapped.h new file +3 -0
+int handle_unaligned_access(u16 instruction, struct pt_regs *regs,
+			    struct mem_access *ma);
+

This too should use opcode_size_t. New code has to live with the fact that we have mixed instruction sizes these days.

#2 lore raw
In reply to Magnus Damm
Quoted 11 lines · Paul Mundt
Quoted 10 lines · Magnus Damm
arch/sh/kernel/traps_32.c +10 -0
+static int from_user(void *dst, void *src, int cnt)
+{
+	return copy_from_user(dst, src, cnt);
+}
+
+static int to_user(void *dst, void *src, int cnt)
+{
+	return copy_to_user(dst, src, cnt);
+}
+
Quoted 9 lines

arch/sh/kernel/traps_32.c static int die_if_no_fixup(const char * · line 189

Quoted 5 lines
arch/sh/kernel/traps_32.c +5 -0
+static struct mem_access user_mem_access = {
+	from_user,
+	to_user,
+};
+

The argument ordering and everything in these is exactly the same, why don't you use copy_to_user()/copy_from_user() in your structure here instead of these no-op wrappers?

Or rather, just make copy_to_user()/copy_from_user() inlines rather than macros so they can be used here. If you're already certain about the access, you can also use the __copy_to/from_user() variants which can be dropped in here assuming you can live without the access_ok() verification.

author
#3 lore raw
In reply to Magnus Damm

Hi Paul,

Sorry about the rough quality, but the code started to rot so I had to get it out... =)

Quoted 7 lines · Paul Mundt
Quoted 6 lines · Magnus Damm
arch/sh/Kconfig 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/Kconfig +0 -0
@@ -324,6 +324,9 @@ source "arch/sh/Kconfig.cpu"
 
 menu "Board support"
Quoted 7 lines

arch/sh/Kconfig 2008-02-06 22:57:22.000000000 +0900 source "arch/sh/Kconfig.cpu" · line 332

Quoted 6 lines
arch/sh/Kconfig 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/Kconfig +3 -0
+config IO_TRAPPED
+     bool
+
 config SOLUTION_ENGINE
      bool

This is a pretty bizarre place to put this symbol.

Yep, will fix.

Quoted 13 lines
Quoted 13 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +7 -0
@@ -63,6 +63,14 @@ EXPORT_SYMBOL(memset_io);
 
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
+     void __iomem *ret;
+
+     if (nr > 4) {
+             ret = ioremap_trapped(port, nr, IORESOURCE_IO);
+             if (ret)
+                     return ret;
+     }
Quoted 8 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 EXPORT_SYMBOL(memset_io); · line 76

Quoted 4 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +1 -0
+
      return sh_mv.mv_ioport_map(port, nr);
 }
 EXPORT_SYMBOL(ioport_map);

SH-5 needs 64-bit I/O, as do some of the SH-5 blocks that have showed up on SH-4 parts (ie, the SH4-202 SuperHyway CVRs and so on). I don't know that there's any point in tying to hook this in here for unhandled sizes.

Ok, I added that ugly (nr > 4) hack to avoid getting called by the io routines in io_generic.c. They call ioport_map() on every memory access. I'll make them use __ioport_map instead which won't include the call to ioremap_trapped().

Quoted 24 lines
Quoted 24 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +24 -0
+unsigned int trapped_ioread8(void __iomem *addr)
+{
+     return ctrl_inb((unsigned long __force)addr);
+}
+
+unsigned int trapped_ioread16(void __iomem *addr)
+{
+     return ctrl_inw((unsigned long __force)addr);
+}
+
+unsigned int trapped_ioread32(void __iomem *addr)
+{
+     return ctrl_inl((unsigned long __force)addr);
+}
+
+void trapped_iowrite8(unsigned int data, void __iomem *addr)
+{
+     ctrl_outb(data, (unsigned long __force)addr);
+}
+
+void trapped_iowrite16(unsigned int data, void __iomem *addr)
+{
+     ctrl_outw(data, (unsigned long __force)addr);
+}
Quoted 8 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+
+void trapped_iowrite32(unsigned int data, void __iomem *addr)
+{
+     ctrl_outl(data, (unsigned long __force)addr);
+}
+

It would be nice to ioport_map() these directly and do away with the __force casting, as per generic I/O.

Yep, good idea.

Quoted 6 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 2 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +2 -0
+LIST_HEAD(trapped_io);
+LIST_HEAD(trapped_mem);

These should probably be conditionalized on CONFIG_HAS_IOPORT and CONFIG_HAS_IOMEM, so at least boards that don't have any use for the ioport stuff can save some space here.

Sure, will do.

Quoted 10 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+static DEFINE_SPINLOCK(trapped_lock);
+
+#define TRAPPED_PAGES_MAX 16
+
+void __init register_trapped_io_handler(struct trapped_io *tiop)
+{

There are a lot of things that can fail in here, please make this an int and actually pass back the return value as opposed to just BUG'ing out at the first sign of trouble.

Quoted 5 lines

Perhaps it's worth splitting out the platform devices in to a set of trapped devices and non-trapped, then only registering the trapped ones when the trapped page setup succeeds. There's no reason to bring down the rest of the system because you can't trap non-standard writes to a mostly broken SuperIO.

It's better to let the register_trapped_io_handler() function return an int.

Quoted 4 lines
Quoted 4 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +4 -0
+static int from_device(void *dst, void *src, int cnt)
+{
+     struct trapped_io *tiop;
+     unsigned long addr = (unsigned long)src;
Quoted 8 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+     unsigned int tmp = 0;
+
+     tiop = lookup_tiop(addr);
+     BUG_ON(!tiop);
+     BUG_ON(tiop->magic != IO_TRAPPED_MAGIC);
+

Same deal here. You can in fact pass back an error code without BUG()'ing.

I'll replace those with WARN_ON().

Quoted 7 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 5 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +5 -0
+     addr = lookup_address(tiop, addr);
+#ifdef DEBUG
+     printk(KERN_DEBUG "trapped io read! 0x%08x, %d",
+            (unsigned int)src, cnt);
+#endif

pr_debug().

Gotcha.

Quoted 16 lines
Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+     switch (cnt) {
+     case 1:
+             tmp = tiop->read8((void __iomem *)addr);
+             *(unsigned char *)dst = tmp & 0xff;
+             break;
+     case 2:

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 10 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +10 -0
+             tmp = tiop->read16((void __iomem *)addr);
+             *(unsigned short *)dst = tmp & 0xffff;
+             break;
+     case 4:
+             tmp = tiop->read32((void __iomem *)addr);
+             *(unsigned int *)dst = tmp;
+             break;
+     default:
+             BUG();
+     }
Quoted 3 lines

ctrl_outX() for these. The __iomem cast back and forth is also fairly ugly. Since you force an unsigned long to a void __iomem * here and then back to an unsigned long in the routines themselves. Grr.

I'll get rid of the ugly casting and I'll replace with *dst/*src code with ctrl_outX/inX().

Quoted 7 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+int handle_trapped_io(struct pt_regs *regs, unsigned long address)
+{
+     mm_segment_t oldfs;
+     u16 instruction;
+     int tmp;
+

opcode_size_t.

Yeah, i'll use that one instead.

Quoted 7 lines
Quoted 7 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +7 -0
+     if (!lookup_tiop(address))
+             return 0;
+
+     BUG_ON(user_mode(regs));
+
+     oldfs = get_fs();
+     set_fs(KERNEL_DS);
Quoted 8 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 6 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +6 -0
+     if (copy_from_user(&instruction, (u16 *)(regs->pc), 2)) {
+             set_fs(oldfs);
+             BUG();
+             return 0;
+     }
+

opcode_size_t and instruction_size(). The BUG()'s here are also far too aggressive, neither one looks helpful.

I'll replace the BUG()'s with WARN().

Quoted 5 lines

arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900

Quoted 3 lines
arch/sh/kernel/io.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/io.c +3 -0
+     tmp = handle_unaligned_access(instruction, regs, &trapped_mem_access);
+     set_fs(oldfs);
+     return !tmp;

return tmp = 0 is a bit less ugly in these sorts of cases.

Whatever makes you happy. =)

Quoted 22 lines
Quoted 22 lines
arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/traps_32.c +9 -1
@@ -150,18 +150,43 @@ static int die_if_no_fixup(const char *
 static inline void sign_extend(unsigned int count, unsigned char *dst)
 {
 #ifdef __LITTLE_ENDIAN__
+     if ((count = 1) && dst[0] & 0x80) {
+             dst[1] = 0xff;
+             dst[2] = 0xff;
+             dst[3] = 0xff;
+     }
      if ((count = 2) && dst[1] & 0x80) {
              dst[2] = 0xff;
              dst[3] = 0xff;
      }
 #else
-     if ((count = 2) && dst[2] & 0x80) {
+     if ((count = 1) && dst[3] & 0x80) {
+             dst[2] = 0xff;
+             dst[1] = 0xff;
              dst[0] = 0xff;
+     }
Quoted 8 lines

arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 static int die_if_no_fixup(const char * · line 174

Quoted 7 lines
arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/traps_32.c +2 -0
+     if ((count = 2) && dst[2] & 0x80) {
              dst[1] = 0xff;
+             dst[0] = 0xff;
      }
 #endif
 }

Shouldn't this be a separate patch?

It could be, but the regular unaligned code won't need byte sized sign extension. =)

Quoted 10 lines
Quoted 10 lines
arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/traps_32.c +10 -0
+static int from_user(void *dst, void *src, int cnt)
+{
+     return copy_from_user(dst, src, cnt);
+}
+
+static int to_user(void *dst, void *src, int cnt)
+{
+     return copy_to_user(dst, src, cnt);
+}
+
Quoted 8 lines

arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 static int die_if_no_fixup(const char * · line 189

Quoted 5 lines
arch/sh/kernel/traps_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/kernel/traps_32.c +5 -0
+static struct mem_access user_mem_access = {
+     from_user,
+     to_user,
+};
+

The argument ordering and everything in these is exactly the same, why don't you use copy_to_user()/copy_from_user() in your structure here instead of these no-op wrappers?

I'll make copy_from_user() and copy_to_user() inlines and use them directly.

Quoted 14 lines
Quoted 14 lines
arch/sh/mm/fault_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/mm/fault_32.c +1 -0
@@ -15,6 +15,7 @@
 #include <linux/mm.h>
 #include <linux/hardirq.h>
 #include <linux/kprobes.h>
+#include <asm/io_trapped.h>
 #include <asm/system.h>
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>

@@ -163,6 +164,8 @@ no_context:
      if (fixup_exception(regs))
              return;
Quoted 7 lines

arch/sh/mm/fault_32.c 2008-02-06 22:57:22.000000000 +0900 no_context: · line 171

Quoted 5 lines
arch/sh/mm/fault_32.c 2008-02-06 22:57:22.000000000 +0900 renamed from arch/sh/mm/fault_32.c +2 -0
+     if (handle_trapped_io(regs, address))
+             return;
 /*
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.

This looks like a good candidate for page fault notifiers..

Yeah, maybe we can use that framework later on.

Quoted 22 lines
Quoted 22 lines
include/asm-sh/io.h 2008-02-06 22:57:22.000000000 +0900 renamed from include/asm-sh/io.h +8 -0
@@ -38,6 +38,7 @@
  */
 #define __IO_PREFIX  generic
 #include <asm/io_generic.h>
+#include <asm/io_trapped.h>
 
 #define maybebadio(port) \
   printk(KERN_ERR "bad PC-like io %s:%u for port 0x%lx at 0x%08x\n", \

@@ -309,7 +310,15 @@ __ioremap_mode(unsigned long offset, uns
 {
 #ifdef CONFIG_SUPERH32
      unsigned long last_addr = offset + size - 1;
+#endif
+     void __iomem *ret = NULL;
+
+     if (!(flags & _PAGE_CACHABLE))
+             ret = ioremap_trapped(offset, size, IORESOURCE_MEM);
+     if (ret)
+             return ret;
Quoted 10 lines

include/asm-sh/io.h 2008-02-06 22:57:22.000000000 +0900 __ioremap_mode(unsigned long offset, uns · line 324

Quoted 4 lines
include/asm-sh/io.h 2008-02-06 22:57:22.000000000 +0900 renamed from include/asm-sh/io.h +1 -0
+#ifdef CONFIG_SUPERH32
      /*
       * For P1 and P2 space this is trivial, as everything is already
       * mapped. Uncached access for P1 addresses are done through P2.

This is also a bit of a mess. Why are you handing off IORESOURCE_MEM here when y ou don't know whether it's an IORESOURCE_MEM or IORESOURCE_IO? The pgprot validation here is also non-obvious, you may as well just unconditionally call ioremap_trapped() (or __ioremap_trapped(), as it shouuld be considered an internal API), and bail out early if it's happy.

I'll call __ioremap_trapped() all the time then. But I'll keep IORESOURCE_MEM since IORESOURCE_IO should be mapped with ioport_map() instead of ioremap().

Quoted 20 lines
Quoted 20 lines
include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 new file +17 -0
@@ -0,0 +1,59 @@
+#ifndef __ASM_SH_IO_TRAPPED_H
+#define __ASM_SH_IO_TRAPPED_H
+
+#include <linux/list.h>
+#include <linux/ioport.h>
+#include <asm/page.h>
+
+#define IO_TRAPPED_MAGIC 0xfeedbeef
+
+struct trapped_io {
+     unsigned int magic;
+     struct resource *resource;
+     unsigned int num_resources;
+     struct list_head list;
+     void __iomem *virt_base;
+     unsigned int (*read8)(void __iomem *addr);
+     unsigned int (*read16)(void __iomem *addr);
Quoted 8 lines

include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 line 23

Quoted 6 lines
include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 new file +6 -0
+     unsigned int (*read32)(void __iomem *addr);
+     void (*write8)(unsigned int data, void __iomem *addr);
+     void (*write16)(unsigned int data, void __iomem *addr);
+     void (*write32)(unsigned int data, void __iomem *addr);
+} __attribute__ ((aligned(PAGE_SIZE)));;
+

Should probably add 64-bit versions here also. Double ;'s, and this really wants a __page_aligned.

I'll fix that. Hm...

Quoted 7 lines

include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 line 28

Quoted 5 lines
include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 new file +5 -0
+#ifdef CONFIG_IO_TRAPPED
+void __init register_trapped_io_handler(struct trapped_io *tiop);
+int handle_trapped_io(struct pt_regs *regs, unsigned long address);
+#else
+#define register_trapped_io_handler(tiop) while (0)

do { } while (0) might even work!

Yes !!

Quoted 5 lines

include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 line 31

Quoted 3 lines
include/asm-sh/io_trapped.h 2008-02-06 23:34:23.000000000 +0900 new file +3 -0
+int handle_unaligned_access(u16 instruction, struct pt_regs *regs,
+                         struct mem_access *ma);
+

This too should use opcode_size_t. New code has to live with the fact that we have mixed instruction sizes these days.

Sure, will do.

Thanks for the review!

/ magnus