LPAE fixes and extensions for Keystone

The original phys_to_virt/virt_to_phys patching implementation relied on early patching prior to MMU initialization. On PAE systems running out of >4G address space, this would have entailed an additional round of patching after switching over to the high address space.

The approach implemented here conceptually extends the original PHYS_OFFSET patching implementation with the introduction of "early" patch stubs. Early patch code is required to be functional out of the box, even before the patch is applied. This is implemented by inserting functional (but inefficient) load code into the .runtime.patch.code init section. Having functional code out of the box then allows us to defer the init time patch application until later in the init sequence.

In addition to fitting better with our need for physical address-space switch-over, this implementation should be somewhat more extensible by virtue of its more readable (and hackable) C implementation. This should prove useful for other similar init time specialization needs, especially in light of our multi-platform kernel initiative.

This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7 (Cortex-A8) device.

Note: the obtuse use of stringified symbols in patch_stub() and early_patch_stub() is intentional. Theoretically this should have been accomplished with formal operands passed into the asm block, but this requires the use of the 'c' modifier for instantiating the long (e.g. .long %c0). However, the 'c' modifier has been found to ICE certain versions of GCC, and therefore we resort to stringified symbols here.

Signed-off-by Cyril Chemparathy <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

arch/arm/include/asm/runtime-patch.h new file +24 -0

new file mode 100644
index 0000000..366444d
@@ -0,0 +1,208 @@
+/*
+ * arch/arm/include/asm/runtime-patch.h
+ * Note: this file should not be included by non-asm/.h files
+ *
+ * Copyright 2012 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_ARM_RUNTIME_PATCH_H
+#define __ASM_ARM_RUNTIME_PATCH_H
+
+#include <linux/stringify.h>
+
+#ifndef __ASSEMBLY__

Quoted 29 lines · Cyril Chemparathy

The original phys_to_virt/virt_to_phys patching implementation relied on early patching prior to MMU initialization. On PAE systems running out of >4G address space, this would have entailed an additional round of patching after switching over to the high address space.

The approach implemented here conceptually extends the original PHYS_OFFSET patching implementation with the introduction of "early" patch stubs. Early patch code is required to be functional out of the box, even before the patch is applied. This is implemented by inserting functional (but inefficient) load code into the .runtime.patch.code init section. Having functional code out of the box then allows us to defer the init time patch application until later in the init sequence.

In addition to fitting better with our need for physical address-space switch-over, this implementation should be somewhat more extensible by virtue of its more readable (and hackable) C implementation. This should prove useful for other similar init time specialization needs, especially in light of our multi-platform kernel initiative.

This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7 (Cortex-A8) device.

Note: the obtuse use of stringified symbols in patch_stub() and early_patch_stub() is intentional. Theoretically this should have been accomplished with formal operands passed into the asm block, but this requires the use of the 'c' modifier for instantiating the long (e.g. .long %c0). However, the 'c' modifier has been found to ICE certain versions of GCC, and therefore we resort to stringified symbols here.

Quoted 2 lines

Signed-off-by Cyril Chemparathy <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

There is another problem with this.

[...]

Quoted 30 lines

arch/arm/include/asm/runtime-patch.h new file +24 -0

new file mode 100644
index 0000000..366444d
@@ -0,0 +1,208 @@
+/*
+ * arch/arm/include/asm/runtime-patch.h
+ * Note: this file should not be included by non-asm/.h files
+ *
+ * Copyright 2012 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_ARM_RUNTIME_PATCH_H
+#define __ASM_ARM_RUNTIME_PATCH_H
+
+#include <linux/stringify.h>
+
+#ifndef __ASSEMBLY__

arch/arm/include/asm/runtime-patch.h line 34

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+
+#ifdef CONFIG_ARM_RUNTIME_PATCH
+
+struct patch_info {
+	void		*insn;
+	u16		 type;
+	u8		 insn_size;
+	u8		 data_size;
+	u32		 data[0];
+};

This causes the following compilation error:

CC sound/core/pcm.o
In file included from sound/core/pcm.c:293:0:
include/linux/soundcard.h:223:8: error: redefinition of 'struct patch_info' arch/arm/include/asm/runtime-patch.h:28:8: note: originally defined here make[2]: *** [sound/core/pcm.o] Error 1

The problem is that asm/runtime-patch.h gets included by asm/memory.h and asm/memory.h is included by almost the entire kernel. Something like "struct patch_info" is a bit too generic a name to be exported to the world as the likelihood of a name collision with some private definition in a driver or the like is rather high.

In that context it might be worth moving everything that is not required for the patch stub definitions out of asm/runtime-patch.h. For example, the definition of struct patch_info, struct patch_info_imm8, patch_next() and patch_data() could be moved to runtime-patch.c directly instead. And then patch_stub() should be renamed to runtime_patch_stub(), early_patch_stub() to early_runtime_patch_stub(), patch_imm8() to runtime_patch_imm8(), etc. Even the __IMM8 symbol name is rather weak for kernel wide scope.

The original phys_to_virt/virt_to_phys patching implementation relied on early patching prior to MMU initialization. On PAE systems running out of >4G address space, this would have entailed an additional round of patching after switching over to the high address space.

The approach implemented here conceptually extends the original PHYS_OFFSET patching implementation with the introduction of "early" patch stubs. Early patch code is required to be functional out of the box, even before the patch is applied. This is implemented by inserting functional (but inefficient) load code into the .runtime.patch.code init section. Having functional code out of the box then allows us to defer the init time patch application until later in the init sequence.

In addition to fitting better with our need for physical address-space switch-over, this implementation should be somewhat more extensible by virtue of its more readable (and hackable) C implementation. This should prove useful for other similar init time specialization needs, especially in light of our multi-platform kernel initiative.

This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7 (Cortex-A8) device.

Note: the obtuse use of stringified symbols in patch_stub() and early_patch_stub() is intentional. Theoretically this should have been accomplished with formal operands passed into the asm block, but this requires the use of the 'c' modifier for instantiating the long (e.g. .long %c0). However, the 'c' modifier has been found to ICE certain versions of GCC, and therefore we resort to stringified symbols here.

Signed-off-by Cyril Chemparathy <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

---
 arch/arm/Kconfig                     |    3 +
 arch/arm/include/asm/module.h        |    7 ++
 arch/arm/include/asm/runtime-patch.h |  208 ++++++++++++++++++++++++++++++++++
 arch/arm/kernel/Makefile             |    1 +
 arch/arm/kernel/module.c             |    9 +-
 arch/arm/kernel/runtime-patch.c      |  193 +++++++++++++++++++++++++++++++
 arch/arm/kernel/setup.c              |    3 +
 arch/arm/kernel/vmlinux.lds.S        |   10 ++
 8 files changed, 433 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/runtime-patch.h
 create mode 100644 arch/arm/kernel/runtime-patch.c

arch/arm/Kconfig +3 -0

index 2f88d8d..98a3a1a 100644
@@ -60,6 +60,9 @@ config ARM
 config ARM_HAS_SG_CHAIN
 	bool
 
+config ARM_RUNTIME_PATCH
+	bool
+
 config NEED_SG_DMA_LENGTH
 	bool
 

arch/arm/include/asm/module.h +7 -0

index 6c6809f..2090486 100644
@@ -43,9 +43,16 @@ struct mod_arch_specific {
 #define MODULE_ARCH_VERMAGIC_ARMTHUMB ""
 #endif
 
+#ifdef CONFIG_ARM_RUNTIME_PATCH
+#define MODULE_ARCH_VERMAGIC_RT_PATCH "rt-patch "
+#else
+#define MODULE_ARCH_VERMAGIC_RT_PATCH ""
+#endif
+
 #define MODULE_ARCH_VERMAGIC \
 	MODULE_ARCH_VERMAGIC_ARMVSN \
 	MODULE_ARCH_VERMAGIC_ARMTHUMB \
+	MODULE_ARCH_VERMAGIC_RT_PATCH \
 	MODULE_ARCH_VERMAGIC_P2V
 
 #endif /* _ASM_ARM_MODULE_H */

arch/arm/include/asm/runtime-patch.h new file +57 -0

new file mode 100644
index 0000000..366444d
@@ -0,0 +1,208 @@
+/*
+ * arch/arm/include/asm/runtime-patch.h
+ * Note: this file should not be included by non-asm/.h files
+ *
+ * Copyright 2012 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_ARM_RUNTIME_PATCH_H
+#define __ASM_ARM_RUNTIME_PATCH_H
+
+#include <linux/stringify.h>
+
+#ifndef __ASSEMBLY__
+
+#ifdef CONFIG_ARM_RUNTIME_PATCH
+
+struct patch_info {
+	void		*insn;
+	u16		 type;
+	u8		 insn_size;
+	u8		 data_size;
+	u32		 data[0];
+};
+
+#define PATCH_IMM8		0x0001
+struct patch_info_imm8 {
+	u32		*imm;
+	u32		 insn;
+};
+
+#define patch_next(p)	((void *)(p) + sizeof(*(p)) + (p)->data_size)
+#define patch_data(p)	((void *)&(p)->data[0])
+
+#define patch_stub(type, code, patch_data, ...)				\
+	__asm__("@ patch stub\n"					\
+		"1:\n"							\
+		code							\
+		"2:\n"							\
+		"	.pushsection .runtime.patch.table, \"a\"\n"	\
+		"3:\n"							\
+		"	.word 1b\n"					\
+		"	.hword (" __stringify(type) ")\n"		\
+		"	.byte (2b-1b)\n"				\
+		"	.byte (5f-4f)\n"				\
+		"4:\n"							\
+		patch_data						\

arch/arm/include/asm/runtime-patch.h new file +31 -0

+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* operands */						\
+			: [to]	 "=r" (_to)				\
+			: [from] "r"  (_from),				\
+			  [imm]	 "I"  (__IMM8),				\
+				 "i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * patch_imm8_mov() - same as patch_imm8(), but for mov/mvn instructions
+ */
+#define patch_imm8_mov(_insn, _to, _sym, _ofs)				\
+	patch_stub(							\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			_insn "	%[to], %[imm]\n",			\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn "	%[to], %[imm]\n",			\
+		/* operands */						\
+			: [to]	"=r" (_to)				\
+			: [imm]	"I"  (__IMM8),				\
+				"i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * early_patch_imm8() - early functional variant of patch_imm8() above.  The
+ *			same restrictions on the constant apply here.  This
+ *			version emits workable (albeit inefficient) code at
+ *			compile-time, and therefore functions even prior to

arch/arm/include/asm/runtime-patch.h new file +4 -0

+			"ldr	%[tmp], [%[tmp]]\n"			\
+			_insn "	%[to], %[from], %[tmp]\n",		\
+		/* pad */						\
+			0,						\

arch/arm/include/asm/runtime-patch.h new file +44 -0

+} while (0)
+
+#define early_patch_imm8_mov(_insn, _to, _sym, _ofs)			\
+do {									\
+	unsigned long __tmp;						\
+	early_patch_stub(						\
+		/* type */						\
+			PATCH_IMM8					\
+		/* code */						\
+			"ldr	%[tmp], =" __stringify(_sym + _ofs) "\n"\
+			"ldr	%[tmp], [%[tmp]]\n"			\
+			_insn "	%[to], %[tmp]\n",			\
+		/* pad */						\
+			0,						\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn " %[to], %[imm]\n",			\
+		/* operands */						\
+			: [to]	"=r"  (_to),				\
+			  [tmp]	"=&r" (__tmp)				\
+			: [imm]	"I"   (__IMM8),				\
+				"i"   (&(_sym))				\
+			: "cc");					\
+} while (0)
+
+int runtime_patch(const void *table, unsigned size);
+void runtime_patch_kernel(void);
+
+#else
+
+static inline int runtime_patch(const void *table, unsigned size)
+{
+	return 0;
+}
+
+static inline void runtime_patch_kernel(void)
+{
+}
+
+#endif /* CONFIG_ARM_RUNTIME_PATCH */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ASM_ARM_RUNTIME_PATCH_H */

arch/arm/kernel/Makefile +1 -0

index 7ad2d5c..0b16b94 100644
@@ -81,5 +81,6 @@ endif
 head-y			:= head$(MMUEXT).o
 obj-$(CONFIG_DEBUG_LL)	+= debug.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
+obj-$(CONFIG_ARM_RUNTIME_PATCH)	+= runtime-patch.o patch.o
 
 extra-y := $(head-y) vmlinux.lds

arch/arm/kernel/module.c +8 -1

index 1e9be5d..10a2922 100644
@@ -24,6 +24,7 @@
 #include <asm/sections.h>
 #include <asm/smp_plat.h>
 #include <asm/unwind.h>
+#include <asm/runtime-patch.h>
 
 #ifdef CONFIG_XIP_KERNEL
 /*

@@ -276,7 +277,7 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 	const Elf_Shdr *sechdrs_end = sechdrs + hdr->e_shnum;
 	struct mod_unwind_map maps[ARM_SEC_MAX];
-	int i;
+	int i, err;
 
 	memset(maps, 0, sizeof(maps));
 

@@ -321,6 +322,12 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	if (s)
 		fixup_pv_table((void *)s->sh_addr, s->sh_size);
 #endif
+	s = find_mod_section(hdr, sechdrs, ".runtime.patch.table");
+	if (s) {
+		err = runtime_patch((void *)s->sh_addr, s->sh_size);
+		if (err)
+			return err;
+	}
 	s = find_mod_section(hdr, sechdrs, ".alt.smp.init");
 	if (s && !is_smp())
 #ifdef CONFIG_SMP_ON_UP

arch/arm/kernel/runtime-patch.c new file +21 -0

new file mode 100644
index 0000000..28a6367
@@ -0,0 +1,193 @@
+/*
+ * arch/arm/kernel/runtime-patch.c
+ *
+ * Copyright 2012 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kernel.h>
+#include <linux/sched.h>
+
+#include <asm/opcodes.h>

arch/arm/kernel/runtime-patch.c new file +51 -0

+
+#ifdef CONFIG_THUMB2_KERNEL
+
+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)
+{
+	u32 op, rot, val;
+	const u32 supported_ops = (BIT(0)  | /* and */
+				   BIT(1)  | /* bic */
+				   BIT(2)  | /* orr/mov */
+				   BIT(3)  | /* orn/mvn */
+				   BIT(4)  | /* eor */
+				   BIT(8)  | /* add */
+				   BIT(10) | /* adc */
+				   BIT(11) | /* sbc */
+				   BIT(12) | /* sub */
+				   BIT(13)); /* rsb */
+
+	insn = __mem_to_opcode_thumb32(insn);
+
+	if (!__opcode_is_thumb32(insn)) {
+		pr_err("patch: invalid thumb2 insn %08x\n", insn);
+		return -EINVAL;
+	}
+
+	/* allow only data processing (immediate)
+	 * 1111 0x0x xxx0 xxxx 0xxx xxxx xxxx xxxx */
+	if ((insn & 0xfa008000) != 0xf0000000) {
+		pr_err("patch: unknown insn %08x\n", insn);
+		return -EINVAL;
+	}
+
+	/* extract op code */
+	op = (insn >> 21) & 0xf;
+
+	/* disallow unsupported opcodes */
+	if ((supported_ops & BIT(op)) == 0) {
+		pr_err("patch: unsupported opcode %x\n", op);
+		return -EINVAL;
+	}
+
+	if (imm <= 0xff) {
+		rot = 0;
+		val = imm;
+	} else {
+		rot = 32 - fls(imm); /* clz */
+		if (imm & ~(0xff000000 >> rot)) {
+			pr_err("patch: constant overflow %08x\n", imm);
+			return -EINVAL;
+		}
+		val  = (imm >> (24 - rot)) & 0x7f;
+		rot += 8; /* encoded i:imm3:a */

arch/arm/kernel/runtime-patch.c new file +37 -0

+
+	return 0;
+}
+
+#else
+
+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)
+{
+	u32 rot, val, op;
+
+	insn = __mem_to_opcode_arm(insn);
+
+	/* disallow special unconditional instructions
+	 * 1111 xxxx xxxx xxxx xxxx xxxx xxxx xxxx */
+	if ((insn >> 24) == 0xf) {
+		pr_err("patch: unconditional insn %08x\n", insn);
+		return -EINVAL;
+	}
+
+	/* allow only data processing (immediate)
+	 * xxxx 001x xxxx xxxx xxxx xxxx xxxx xxxx */
+	if (((insn >> 25) & 0x3) != 1) {
+		pr_err("patch: unknown insn %08x\n", insn);
+		return -EINVAL;
+	}
+
+	/* extract op code */
+	op = (insn >> 20) & 0x1f;
+
+	/* disallow unsupported 10xxx op codes */
+	if (((op >> 3) & 0x3) == 2) {
+		pr_err("patch: unsupported opcode %08x\n", insn);
+		return -EINVAL;
+	}
+
+	rot = imm ? __ffs(imm) / 2 : 0;
+	val = imm >> (rot * 2);

arch/arm/kernel/runtime-patch.c new file +22 -0

+
+	return 0;
+}
+
+#endif	/* CONFIG_THUMB2_KERNEL */
+
+static int apply_patch_imm8(const struct patch_info *p)
+{
+	u32 *insn_ptr = p->insn, ninsn;
+	int count = p->insn_size / sizeof(u32);
+	const struct patch_info_imm8 *info;
+	int err;
+
+
+	if (count <= 0 || p->data_size != count * sizeof(*info)) {
+		pr_err("patch: bad patch, insn size %d, data size %d\n",
+		       p->insn_size, p->data_size);
+		return -EINVAL;
+	}
+
+	for (info = patch_data(p); count; count--, info++, insn_ptr++) {
+		err = do_patch_imm8(info->insn, *info->imm, &ninsn);

arch/arm/kernel/runtime-patch.c new file +22 -0

+{
+	const struct patch_info *p = table, *end = (table + size);
+
+	for (p = table; p < end; p = patch_next(p)) {
+		int err = -EINVAL;
+
+		if (p->type == PATCH_IMM8)
+			err = apply_patch_imm8(p);
+		if (err)
+			return err;
+	}
+	return 0;
+}
+
+void __init runtime_patch_kernel(void)
+{
+	extern unsigned __runtime_patch_table_begin, __runtime_patch_table_end;
+	const void *start = &__runtime_patch_table_begin;
+	const void *end   = &__runtime_patch_table_end;
+
+	BUG_ON(runtime_patch(start, end - start));
+}

arch/arm/kernel/setup.c +3 -0

index a81dcec..669bbf0 100644
@@ -55,6 +55,7 @@
 #include <asm/traps.h>
 #include <asm/unwind.h>
 #include <asm/memblock.h>
+#include <asm/runtime-patch.h>
 
 #if defined(CONFIG_DEPRECATED_PARAM_STRUCT)
 #include "compat.h"

@@ -998,6 +999,8 @@ void __init setup_arch(char **cmdline_p)
 
 	if (mdesc->init_early)
 		mdesc->init_early();
+
+	runtime_patch_kernel();
 }
 
 

arch/arm/kernel/vmlinux.lds.S +10 -0

index 36ff15b..ea35ca0 100644
@@ -167,6 +167,16 @@ SECTIONS
 		*(.pv_table)
 		__pv_table_end = .;
 	}
+	.init.runtime_patch_table : {
+		__runtime_patch_table_begin = .;
+		*(.runtime.patch.table)
+		__runtime_patch_table_end = .;
+	}
+	.init.runtime_patch_code : {
+		__runtime_patch_code_begin = .;
+		*(.runtime.patch.code)
+		__runtime_patch_code_end = .;
+	}
 	.init.data : {
 #ifndef CONFIG_XIP_KERNEL
 		INIT_DATA

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Cyril Chemparathy

The original phys_to_virt/virt_to_phys patching implementation relied on early patching prior to MMU initialization. On PAE systems running out of >4G address space, this would have entailed an additional round of patching after switching over to the high address space.

Quoted 7 lines

The approach implemented here conceptually extends the original PHYS_OFFSET patching implementation with the introduction of "early" patch stubs. Early patch code is required to be functional out of the box, even before the patch is applied. This is implemented by inserting functional (but inefficient) load code into the .runtime.patch.code init section. Having functional code out of the box then allows us to defer the init time patch application until later in the init sequence.

There are currently a few different patching mechanisms in the kernel, and it would be good if we could collect more of them under some common framework.

For example, it might be possible to do the SMP-on-UP fixups using the same framework you propose. Best not to attempt that yet, though.

Overall, this looks well thought out and useful, though it looks like it has a few issues that need attention.

Comments below.

Cheers
---Dave

Quoted 6 lines

Note: the obtuse use of stringified symbols in patch_stub() and early_patch_stub() is intentional. Theoretically this should have been accomplished with formal operands passed into the asm block, but this requires the use of the 'c' modifier for instantiating the long (e.g. .long %c0). However, the 'c' modifier has been found to ICE certain versions of GCC, and therefore we resort to stringified symbols here.

You might find that the "n" constraint works. The explanation in the GCC docs is pretty incomprehensible, but at least it exists.

__stringify hacks are not uncommon anyway though, so it's not a problem either way.

arch/arm/include/asm/runtime-patch.h line 67

Quoted 5 lines

arch/arm/include/asm/runtime-patch.h new file +5 -0

+#define early_patch_stub(type, code, pad, patch_data, ...)		\
+	__asm__("@ patch stub\n"					\
+		"1:\n"							\
+		"	b	6f\n"					\
+		"	.fill	" __stringify(pad) ", 1, 0\n"		\

What is the pad argument for? It never seems to be set to anything other than 0 in your series.

The compiler uses a pretty dumb heuristic to guess the size of asms:
4 * (number of ; or \n in the string)

Directives that the compiler can't predict the size of are not safe if they output into any segment that the compiler uses. .fill/.skip are obvious candidates, but macro expansions, .rept, .irp etc. can cause these problems too.

For example:

	void g(int);
	void f(void)
	{
		g(0xd00dfeed);
		asm(".skip 0x1000");
	}

If you try building this with gcc -marm -Os for example:

/tmp/ccXYm1uP.s: Assembler messages:
/tmp/ccXYm1uP.s:21: Error: bad immediate value for offset (4100)

...because the assembler assumes that it can dump a literal at the end of the function and reference it from the g() callsite.

It may be that you have some intended future use for pad (such as pasting one instruction sequence in place of another possibly differently-sized sequence at fixup time), in which case this might require a bit more thought.

Quoted 8 lines

arch/arm/include/asm/runtime-patch.h new file +8 -0

+		"2:\n"							\
+		"	.pushsection .runtime.patch.table, \"a\"\n"	\
+		"3:\n"							\
+		"	.word 1b\n"					\
+		"	.hword (" __stringify(type) ")\n"		\
+		"	.byte (2b-1b)\n"				\
+		"	.byte (5f-4f)\n"				\
+		"4:\n"							\

arch/arm/include/asm/runtime-patch.h line 80

Quoted 5 lines

arch/arm/include/asm/runtime-patch.h new file +5 -0

+		patch_data						\
+		"	.align\n"					\
+		"5:\n"							\
+		"	.popsection\n"					\
+		"	.pushsection .runtime.patch.code, \"ax\"\n"	\

I have a vague feeling that this should have .text in the name somewhere, since it is code that gets executed in place.

arch/arm/include/asm/runtime-patch.h line 83

Quoted 3 lines

arch/arm/include/asm/runtime-patch.h new file +3 -0

+		"6:\n"							\
+		code							\
+		"	b 2b\n"						\

.ltorg

(See [1] below.)

arch/arm/include/asm/runtime-patch.h

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+		"	.popsection\n"					\
+		__VA_ARGS__)
+
+/* constant used to force encoding */
+#define __IMM8		(0x81 << 24)
+
+/*
+ * patch_imm8() - init-time specialized binary operation (imm8 operand)
+ *		  This effectively does: to = from "insn" sym,
+ *		  where the value of sym is fixed at init-time, and is patched

If I've understood correctly, then this description is a bit misleading. sym is not an absolute operand, but rather *(sym + ofs) is a variable containing the fixup operand.

Quoted 9 lines

+ *		  in as an immediate operand.  This value must be
+ *		  representible as an 8-bit quantity with an optional
+ *		  rotation.
+ *
+ *		  The stub code produced by this variant is non-functional
+ *		  prior to patching.  Use early_patch_imm8() if you need the
+ *		  code to be functional early on in the init sequence.

+ */
+#define patch_imm8(_insn, _to, _from, _sym, _ofs) \

Why are the _sym and _ofs parameters separate? Nothing in your series seems to requre _sym to be a symbol or _ofs to be a number; and nothing passes _ofs != 0, but anyway I don't see why the caller can't add those two values together in the macro argument.

arch/arm/include/asm/runtime-patch.h line 109

Quoted 7 lines

arch/arm/include/asm/runtime-patch.h new file +7 -0

+	patch_stub(							\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\

If _sym or _ofs is a complex expression, the + may mis-bind. If the _ofs parameter is needed at all, it would probably be a good idea to have parentheses around _sym and _ofs.

Quoted 31 lines

arch/arm/include/asm/runtime-patch.h new file +31 -0

+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* operands */						\
+			: [to]	 "=r" (_to)				\
+			: [from] "r"  (_from),				\
+			  [imm]	 "I"  (__IMM8),				\
+				 "i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * patch_imm8_mov() - same as patch_imm8(), but for mov/mvn instructions
+ */
+#define patch_imm8_mov(_insn, _to, _sym, _ofs)				\
+	patch_stub(							\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			_insn "	%[to], %[imm]\n",			\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn "	%[to], %[imm]\n",			\
+		/* operands */						\
+			: [to]	"=r" (_to)				\
+			: [imm]	"I"  (__IMM8),				\
+				"i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * early_patch_imm8() - early functional variant of patch_imm8() above.  The
+ *			same restrictions on the constant apply here.  This
+ *			version emits workable (albeit inefficient) code at
+ *			compile-time, and therefore functions even prior to

arch/arm/include/asm/runtime-patch.h line 150

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+ *			patch application.
+ */
+#define early_patch_imm8(_insn, _to, _from, _sym, _ofs)			\
+do {									\
+	unsigned long __tmp;						\
+	early_patch_stub(						\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			"ldr	%[tmp], =" __stringify(_sym + _ofs) "\n"\

This would not be OK if assembled into the .text section, for the reasons described above. (The compiler doesn't know about the extra data word injected by the ldr= pseudo-instruction.)

early_patch_stub puts <code> into a custom section, so that's OK.

However, there's still nothing to ensure that the literal word is in range of the load. That can be fixed with an .ltorg to dump out the literal(s) right after the branch following <code> in early_patch_stub.

Quoted 4 lines

arch/arm/include/asm/runtime-patch.h new file +4 -0

+			"ldr	%[tmp], [%[tmp]]\n"			\
+			_insn "	%[to], %[from], %[tmp]\n",		\
+		/* pad */						\
+			0,						\

arch/arm/include/asm/runtime-patch.h line 164

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* operands */						\
+			: [to]	 "=r"  (_to),				\
+			  [tmp]	 "=&r" (__tmp)				\
+			: [from] "r"   (_from),				\
+			  [imm]	 "I"   (__IMM8),			\
+				 "i"   (&(_sym))			\
+			: "cc");					\

Should we have "cc" here?

Since this macro is only used with a single instruction _insn, there would be no way to make use of a condition set by that instruction.

Therefore, flag-setting instructions don't really make any sense here, and "cc" could be removed.

If so, it could make sense for the apply_patch_imm8() implementation to check for and reject flag-setting encodings.

Quoted 5 lines

arch/arm/kernel/runtime-patch.c new file +0 -0

new file mode 100644
index 0000000..28a6367

arch/arm/kernel/runtime-patch.c line 31

Quoted 5 lines

arch/arm/kernel/runtime-patch.c new file +5 -0

+static inline void flush_icache_insn(void *insn_ptr, int bytes)
+{
+	unsigned long insn_addr = (unsigned long)insn_ptr;
+	flush_icache_range(insn_addr, insn_addr + bytes - 1);
+}

This function appears unused.

Do we actually do the cache flushing anywhere?

Quoted 2 lines

arch/arm/kernel/runtime-patch.c new file +2 -0

+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)
+{

arch/arm/kernel/runtime-patch.c line 92

Quoted 4 lines

arch/arm/kernel/runtime-patch.c new file +4 -0

+	*ninsn  = insn & ~(BIT(26) | 0x7 << 12 | 0xff);
+	*ninsn |= (rot >> 3) << 26;	/* field "i" */
+	*ninsn |= (rot & 0x7) << 12;	/* field "imm3" */
+	*ninsn |= val;

You need to convert this back to memory order. If fixups might be applied while the MMU is off, misaligned 32-bit accesses will fail. It's better to store the two halfwords separately:

	__opcode_to_mem_thumb16(__opcode_thumb32_first(foo))
	__opcode_to_mem_thumb16(__opcode_thumb32_second(foo))

If the MMU is on, you can use __opcode_to_mem_thumb32(foo) and do a possibly misaligned store, though.

This may be a good idea even if the MMU is on, because fixup is a once-only process and we don't expect a meaningful performance impact from that, especially when caching is enabled. But splitting the access may also make it easier to reuse the framework in situations where the cache and MMU are off.

Because of all this, I suggest you don't repeatedly modify *ninsn. Preparing the value and then writing it (or each half) once is probably cleaner.

Quoted 1 line

+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)

arch/arm/kernel/runtime-patch.c line 139

Quoted 2 lines

arch/arm/kernel/runtime-patch.c new file +2 -0

+	/* patch in new immediate and rotation */
+	*ninsn = (insn & ~0xfff) | (rot << 8) | val;

You need __opcode_to_mem_arm() to convert this back to memory order.

Quoted 1 line

+int runtime_patch(const void *table, unsigned size)

Minor nits: the type-unsafety could be dealt with outside this function; table could be struct patch_info const *.

Also, why do we not just pass end to this function instead of subtracting the table base and then adding it back again?

Hi Dave,

Thanks for the detailed review...

Quoted 14 lines · Dave Martin

Quoted 12 lines · Cyril Chemparathy

The original phys_to_virt/virt_to_phys patching implementation relied on early patching prior to MMU initialization. On PAE systems running out of >4G address space, this would have entailed an additional round of patching after switching over to the high address space.

The approach implemented here conceptually extends the original PHYS_OFFSET patching implementation with the introduction of "early" patch stubs. Early patch code is required to be functional out of the box, even before the patch is applied. This is implemented by inserting functional (but inefficient) load code into the .runtime.patch.code init section. Having functional code out of the box then allows us to defer the init time patch application until later in the init sequence.

Quoted 4 lines

There are currently a few different patching mechanisms in the kernel, and it would be good if we could collect more of them under some common framework.

That would be great! I could use some pointers here. I've looked at the kprobes code, and there doesn't appear to be much of an intersection there...

Quoted 3 lines

For example, it might be possible to do the SMP-on-UP fixups using the same framework you propose. Best not to attempt that yet, though.

... and I've looked at the ALT_SMP/ALT_UP stuff as well. The problem here appears to be that ALT_SMP is needed way early in boot
- up in head.S assembly-land.

The third place is probably the jump label mechanism. That may be a fit with some work, but I'm not sure yet.

Quoted 8 lines

Overall, this looks well thought out and useful, though it looks like it has a few issues that need attention.

Comments below.

Cheers
---Dave

[...]

Quoted 7 lines

Quoted 6 lines

Note: the obtuse use of stringified symbols in patch_stub() and early_patch_stub() is intentional. Theoretically this should have been accomplished with formal operands passed into the asm block, but this requires the use of the 'c' modifier for instantiating the long (e.g. .long %c0). However, the 'c' modifier has been found to ICE certain versions of GCC, and therefore we resort to stringified symbols here.

Quoted 6 lines

You might find that the "n" constraint works. The explanation in the GCC docs is pretty incomprehensible, but at least it exists.

__stringify hacks are not uncommon anyway though, so it's not a problem either way.

The stringify hack is nasty, and I'd like to get rid of it if I could...

I looked up constraints.md, and couldn't find "n" defined as a constraint. For that matter, I couldn't find "n" being handled as a modifier in arm_print_operand() either. BTW, I'm looking at GCC 4.6 for these.

Could you please point me to the doc in question that lists this constraint/modifier? Is this specific to newer (or older) versions of GCC.

[...]

Quoted 9 lines

arch/arm/include/asm/runtime-patch.h line 67

Quoted 5 lines

arch/arm/include/asm/runtime-patch.h new file +5 -0

+#define early_patch_stub(type, code, pad, patch_data, ...)		\
+	__asm__("@ patch stub\n"					\
+		"1:\n"							\
+		"	b	6f\n"					\
+		"	.fill	" __stringify(pad) ", 1, 0\n"		\

What is the pad argument for? It never seems to be set to anything other than 0 in your series.

The pad argument is used when we need more than one 32-bit instruction slot in the straight line (post patch) code path. Please look at patch 05/17 of the series (ARM: LPAE: support 64-bit virt_to_phys patching). When we patch 64-bit virt_to_phys(), we need that extra slot to fill up the upper 32-bits of the result.

Quoted 22 lines

The compiler uses a pretty dumb heuristic to guess the size of asms:
4 * (number of ; or \n in the string)

Directives that the compiler can't predict the size of are not safe if they output into any segment that the compiler uses. .fill/.skip are obvious candidates, but macro expansions, .rept, .irp etc. can cause these problems too.

For example:

	void g(int);
	void f(void)
	{
		g(0xd00dfeed);
		asm(".skip 0x1000");
	}

If you try building this with gcc -marm -Os for example:

/tmp/ccXYm1uP.s: Assembler messages:
/tmp/ccXYm1uP.s:21: Error: bad immediate value for offset (4100)

Quoted 9 lines

...because the assembler assumes that it can dump a literal at the end of the function and reference it from the g() callsite.

It may be that you have some intended future use for pad (such as pasting one instruction sequence in place of another possibly differently-sized sequence at fixup time), in which case this might require a bit more thought.

Good point. Thanks.

I'm not sure if this helps, but I don't think pad should be used to insert more than a couple of instruction words into the code.

Quoted 8 lines

Quoted 8 lines

arch/arm/include/asm/runtime-patch.h new file +8 -0

+		"2:\n"							\
+		"	.pushsection .runtime.patch.table, \"a\"\n"	\
+		"3:\n"							\
+		"	.word 1b\n"					\
+		"	.hword (" __stringify(type) ")\n"		\
+		"	.byte (2b-1b)\n"				\
+		"	.byte (5f-4f)\n"				\
+		"4:\n"							\

Quoted 9 lines

arch/arm/include/asm/runtime-patch.h line 80

Quoted 5 lines

arch/arm/include/asm/runtime-patch.h new file +5 -0

+		patch_data						\
+		"	.align\n"					\
+		"5:\n"							\
+		"	.popsection\n"					\
+		"	.pushsection .runtime.patch.code, \"ax\"\n"	\

I have a vague feeling that this should have .text in the name somewhere, since it is code that gets executed in place.

Fair enough. Will change to .runtime.patch.text instead.

Quoted 8 lines

arch/arm/include/asm/runtime-patch.h line 83

Quoted 3 lines

arch/arm/include/asm/runtime-patch.h new file +3 -0

+		"6:\n"							\
+		code							\
+		"	b 2b\n"						\

.ltorg

(See [1] below.)

Quoted 15 lines

arch/arm/include/asm/runtime-patch.h

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+		"	.popsection\n"					\
+		__VA_ARGS__)
+
+/* constant used to force encoding */
+#define __IMM8		(0x81 << 24)
+
+/*
+ * patch_imm8() - init-time specialized binary operation (imm8 operand)
+ *		  This effectively does: to = from "insn" sym,
+ *		  where the value of sym is fixed at init-time, and is patched

If I've understood correctly, then this description is a bit misleading. sym is not an absolute operand, but rather *(sym + ofs) is a variable containing the fixup operand.

Correct. I'll clarify in the text.

Quoted 10 lines

Quoted 9 lines

+ *		  in as an immediate operand.  This value must be
+ *		  representible as an 8-bit quantity with an optional
+ *		  rotation.
+ *
+ *		  The stub code produced by this variant is non-functional
+ *		  prior to patching.  Use early_patch_imm8() if you need the
+ *		  code to be functional early on in the init sequence.

+ */
+#define patch_imm8(_insn, _to, _from, _sym, _ofs) \

Quoted 5 lines

Why are the _sym and _ofs parameters separate? Nothing in your series seems to requre _sym to be a symbol or _ofs to be a number; and nothing passes _ofs != 0, but anyway I don't see why the caller can't add those two values together in the macro argument.

I could (should) really get rid of ofs. This is a hold over from earlier versions of the patch, where we were using the ofs to point to the upper 32-bits of the phys_offset ( = 4 for LE, 0 for BE).

I will clean this up in the next rev. Thanks.

Quoted 13 lines

arch/arm/include/asm/runtime-patch.h line 109

Quoted 8 lines

arch/arm/include/asm/runtime-patch.h new file +7 -0

+	patch_stub(							\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\

If _sym or _ofs is a complex expression, the + may mis-bind. If the _ofs parameter is needed at all, it would probably be a good idea to have parentheses around _sym and _ofs.

I'm guessing this doesn't apply once we get rid of ofs?

Quoted 75 lines

Quoted 31 lines

arch/arm/include/asm/runtime-patch.h new file +31 -0

+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* operands */						\
+			: [to]	 "=r" (_to)				\
+			: [from] "r"  (_from),				\
+			  [imm]	 "I"  (__IMM8),				\
+				 "i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * patch_imm8_mov() - same as patch_imm8(), but for mov/mvn instructions
+ */
+#define patch_imm8_mov(_insn, _to, _sym, _ofs)				\
+	patch_stub(							\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			_insn "	%[to], %[imm]\n",			\
+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn "	%[to], %[imm]\n",			\
+		/* operands */						\
+			: [to]	"=r" (_to)				\
+			: [imm]	"I"  (__IMM8),				\
+				"i"  (&(_sym))				\
+			: "cc")
+
+/*
+ * early_patch_imm8() - early functional variant of patch_imm8() above.  The
+ *			same restrictions on the constant apply here.  This
+ *			version emits workable (albeit inefficient) code at
+ *			compile-time, and therefore functions even prior to

arch/arm/include/asm/runtime-patch.h line 150

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+ *			patch application.
+ */
+#define early_patch_imm8(_insn, _to, _from, _sym, _ofs)			\
+do {									\
+	unsigned long __tmp;						\
+	early_patch_stub(						\
+		/* type */						\
+			PATCH_IMM8,					\
+		/* code */						\
+			"ldr	%[tmp], =" __stringify(_sym + _ofs) "\n"\

This would not be OK if assembled into the .text section, for the reasons described above. (The compiler doesn't know about the extra data word injected by the ldr= pseudo-instruction.)

early_patch_stub puts <code> into a custom section, so that's OK.

However, there's still nothing to ensure that the literal word is in range of the load. That can be fixed with an .ltorg to dump out the literal(s) right after the branch following <code> in early_patch_stub.

Quoted 4 lines

arch/arm/include/asm/runtime-patch.h new file +4 -0

+			"ldr	%[tmp], [%[tmp]]\n"			\
+			_insn "	%[to], %[from], %[tmp]\n",		\
+		/* pad */						\
+			0,						\

arch/arm/include/asm/runtime-patch.h line 164

Quoted 10 lines

arch/arm/include/asm/runtime-patch.h new file +10 -0

+		/* patch_data */					\
+			".long " __stringify(_sym + _ofs) "\n"		\
+			_insn "	%[to], %[from], %[imm]\n",		\
+		/* operands */						\
+			: [to]	 "=r"  (_to),				\
+			  [tmp]	 "=&r" (__tmp)				\
+			: [from] "r"   (_from),				\
+			  [imm]	 "I"   (__IMM8),			\
+				 "i"   (&(_sym))			\
+			: "cc");					\

Should we have "cc" here?

Since this macro is only used with a single instruction _insn, there would be no way to make use of a condition set by that instruction.

Therefore, flag-setting instructions don't really make any sense here, and "cc" could be removed.

Quoted 3 lines

If so, it could make sense for the apply_patch_imm8() implementation to check for and reject flag-setting encodings.

That makes sense. I've modified the do_patch_imm8() functions to explicitly check for attempts to set condition codes, and removed the "cc".

[...]

Quoted 5 lines

Quoted 5 lines

arch/arm/kernel/runtime-patch.c new file +0 -0

new file mode 100644
index 0000000..28a6367

[...]

Quoted 8 lines

arch/arm/kernel/runtime-patch.c line 31

Quoted 5 lines

arch/arm/kernel/runtime-patch.c new file +5 -0

+static inline void flush_icache_insn(void *insn_ptr, int bytes)
+{
+	unsigned long insn_addr = (unsigned long)insn_ptr;
+	flush_icache_range(insn_addr, insn_addr + bytes - 1);
+}

This function appears unused.

Indeed. This function should have been removed. Thanks.

Quoted 2 lines

Do we actually do the cache flushing anywhere?

Yes, in __patch_text().

[...]

Quoted 2 lines

Quoted 2 lines

arch/arm/kernel/runtime-patch.c new file +2 -0

+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)
+{

[...]

Quoted 25 lines

arch/arm/kernel/runtime-patch.c line 92

Quoted 4 lines

arch/arm/kernel/runtime-patch.c new file +4 -0

+	*ninsn  = insn & ~(BIT(26) | 0x7 << 12 | 0xff);
+	*ninsn |= (rot >> 3) << 26;	/* field "i" */
+	*ninsn |= (rot & 0x7) << 12;	/* field "imm3" */
+	*ninsn |= val;

You need to convert this back to memory order. If fixups might be applied while the MMU is off, misaligned 32-bit accesses will fail. It's better to store the two halfwords separately:

	__opcode_to_mem_thumb16(__opcode_thumb32_first(foo))
	__opcode_to_mem_thumb16(__opcode_thumb32_second(foo))

If the MMU is on, you can use __opcode_to_mem_thumb32(foo) and do a possibly misaligned store, though.

This may be a good idea even if the MMU is on, because fixup is a once-only process and we don't expect a meaningful performance impact from that, especially when caching is enabled. But splitting the access may also make it easier to reuse the framework in situations where the cache and MMU are off.

Because of all this, I suggest you don't repeatedly modify *ninsn. Preparing the value and then writing it (or each half) once is probably cleaner.

[...]

Quoted 1 line

Quoted 1 line

+static int do_patch_imm8(u32 insn, u32 imm, u32 *ninsn)

[...]

Quoted 5 lines

arch/arm/kernel/runtime-patch.c line 139

Quoted 2 lines

arch/arm/kernel/runtime-patch.c new file +2 -0

+	/* patch in new immediate and rotation */
+	*ninsn = (insn & ~0xfff) | (rot << 8) | val;

You need __opcode_to_mem_arm() to convert this back to memory order.

The do_patch_imm8() functions do not write to the instruction. The ninsn pointer here is not a pointer to the instruction that is being patched, it is simply a pointer used to return a value to the caller (apply_patch_imm8()).

The instruction is written in apply_patch_imm8():

	u32 ninsn;
	...
	err = do_patch_imm8(info->insn, *info->imm, &ninsn);
	if (err)
		return err;
	__patch_text(insn_ptr, ninsn);

Here the __patch_text() call converts the instruction and performs the actual write. If I read this correctly, the __patch_text() code takes care of the splitting up thumb2 instructions as you've indicated.

[...]

Quoted 8 lines

Quoted 1 line

+int runtime_patch(const void *table, unsigned size)

Minor nits: the type-unsafety could be dealt with outside this function; table could be struct patch_info const *.

Also, why do we not just pass end to this function instead of subtracting the table base and then adding it back again?

In addition to the internal usage within runtime-patch.c, this function is called from module.c. In the module load case, the base
+ size form is more convenient.

Second, based on Nico's comments about keeping the namespace clean, I've now moved the structure definitions to runtime-patch.c. These types are no longer exposed, and so we have to keep them opaque in this interface.

This patch fixes up the types used when converting back and forth between physical and virtual addresses.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Cyril Chemparathy <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

Quoted 6 lines · Cyril Chemparathy

This patch fixes up the types used when converting back and forth between physical and virtual addresses.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Cyril Chemparathy <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

Reviewed-by Catalin Marinas <[email protected]>

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

There is no corresponding change on the phys_to_virt() side, because computations on the upper 32-bits would be discarded anyway.

Signed-off-by Cyril Chemparathy <[email protected]>

---
 arch/arm/include/asm/memory.h |   38 ++++++++++++++++++++++++++++++++++++--
 arch/arm/kernel/head.S        |    4 ++++
 arch/arm/kernel/setup.c       |    2 +-
 3 files changed, 41 insertions(+), 3 deletions(-)

arch/arm/include/asm/memory.h +36 -2

index 88ca206..f3e8f88 100644
@@ -154,13 +154,47 @@
 #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
 
 extern unsigned long	__pv_offset;
-extern unsigned long	__pv_phys_offset;
+extern phys_addr_t	__pv_phys_offset;
 #define PHYS_OFFSET	__virt_to_phys(PAGE_OFFSET)
 
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
-	unsigned long t;
+	phys_addr_t t;
+
+#ifndef CONFIG_ARM_LPAE
 	early_patch_imm8("add", t, x, __pv_offset, 0);
+#else
+	unsigned long __tmp;
+
+#ifndef __ARMEB__
+#define PV_PHYS_HIGH	"(__pv_phys_offset + 4)"
+#else
+#define PV_PHYS_HIGH	"__pv_phys_offset"
+#endif
+
+	early_patch_stub(
+	/* type */		PATCH_IMM8,
+	/* code */
+		"ldr		%[tmp], =__pv_offset\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"add		%Q[to], %[from], %[tmp]\n"
+		"ldr		%[tmp], =" PV_PHYS_HIGH "\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"mov		%R[to], %[tmp]\n",
+	/* pad */		4,
+	/* patch_data */
+		".long		__pv_offset\n"
+		"add		%Q[to], %[from], %[imm]\n"
+		".long	"	PV_PHYS_HIGH "\n"
+		"mov		%R[to], %[imm]\n",
+	/* operands */
+		: [to]	 "=r"	(t),
+		  [tmp]	 "=&r"	(__tmp)
+		: [from] "r"	(x),
+		  [imm]	 "I"	(__IMM8),
+			 "i"	(&__pv_offset),
+			 "i"	(&__pv_phys_offset));
+#endif
 	return t;
 }
 

arch/arm/kernel/head.S +4 -0

index 69a3c09..61fb8df 100644
@@ -530,7 +530,11 @@ ENDPROC(__fixup_pv_offsets)
 
 	.align
 1:	.long	.
+#if defined(CONFIG_ARM_LPAE) && defined(__ARMEB__)
+	.long	__pv_phys_offset + 4
+#else
 	.long	__pv_phys_offset
+#endif
 	.long	__pv_offset
 	.long	PAGE_OFFSET
 #endif

arch/arm/kernel/setup.c +1 -1

index 59e0f57..edb4f42 100644
@@ -159,7 +159,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm, cpu_data);
  * The initializers here prevent these from landing in the BSS section.
  */
 unsigned long __pv_offset = 0xdeadbeef;
-unsigned long __pv_phys_offset = 0xdeadbeef;
+phys_addr_t   __pv_phys_offset = 0xdeadbeef;
 EXPORT_SYMBOL(__pv_phys_offset);
 
 #endif

Signature · 1 line

1.7.9.5

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

Quoted 3 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 5 lines · Catalin Marinas

Quoted 3 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

Quoted 5 lines

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Good point. We better get an early warning if that happens.

Quoted 15 lines · Nicolas Pitre

Quoted 12 lines · Catalin Marinas

Quoted 5 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 2 lines

Good point. We better get an early warning if that happens.

Thanks.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

Quoted 16 lines · Cyril Chemparathy

Quoted 15 lines · Nicolas Pitre

Quoted 12 lines · Catalin Marinas

Quoted 5 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 7 lines

Quoted 2 lines

Good point. We better get an early warning if that happens.

Thanks.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

No. That's not the point here.

Let's suppose a system with 1GB of physical RAM starting at 0xe0000000.

In this case there is no need for highmem. However the v2p patching code in the LPAE case assumes the high bits of a physical address are always the same which wouldn't be the case in this hypothetical example.

We want to make sure the kernel won't boot if a system with physical RAM crossing the 4GB address mark is encountered.

Quoted 34 lines · Nicolas Pitre

Quoted 16 lines · Cyril Chemparathy

Quoted 15 lines · Nicolas Pitre

Quoted 12 lines · Catalin Marinas

Quoted 5 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 7 lines

Quoted 2 lines

Good point. We better get an early warning if that happens.

Thanks.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

No. That's not the point here.

Let's suppose a system with 1GB of physical RAM starting at 0xe0000000.

In this case there is no need for highmem. However the v2p patching code in the LPAE case assumes the high bits of a physical address are always the same which wouldn't be the case in this hypothetical example.

Quoted 2 lines

We want to make sure the kernel won't boot if a system with physical RAM crossing the 4GB address mark is encountered.

I think that's too restrictive. The case with 1 or 2GB below 4GB limit and the rest of the RAM above is a valid one.

For such configurations
we just limit the lowmem to the memory within 4GB and leave the rest as highmem.

We don't want to limit lowmem. The lowmem is very precious memory and we want to maximize its size. In that case it is probably best to implement a real patchable 64-bit addition rather than artificially restricting the lowmem size.

Quoted 4 lines · Nicolas Pitre

We don't want to limit lowmem. The lowmem is very precious memory and we want to maximize its size. In that case it is probably best to implement a real patchable 64-bit addition rather than artificially restricting the lowmem size.

You don't need to. You can solve the V->P translation like this:

         movw    %hi, #0xVWXY            @ fixup
         adds    %lo, %lo, #offset       @ fixup
         adc     %hi, %hi, #0

which is probably the simplest way to do the fixup - keep the existing fixup code, and add support for that movw instruction. And that will work across any 4GB boundary just fine (we won't have more than 4GB of virtual address space on a 32-bit CPU anyway, so we only have to worry about one carry.)

And the P->V translation is truncating anyway, so that is just:

	sub	%lo, %lo, #offset

and nothing more.

Quoted 18 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

We don't want to limit lowmem. The lowmem is very precious memory and we want to maximize its size. In that case it is probably best to implement a real patchable 64-bit addition rather than artificially restricting the lowmem size.

You don't need to. You can solve the V->P translation like this:

         movw    %hi, #0xVWXY            @ fixup
         adds    %lo, %lo, #offset       @ fixup
         adc     %hi, %hi, #0

which is probably the simplest way to do the fixup - keep the existing fixup code, and add support for that movw instruction. And that will work across any 4GB boundary just fine (we won't have more than 4GB of virtual address space on a 32-bit CPU anyway, so we only have to worry about one carry.)

Quoted 6 lines

And the P->V translation is truncating anyway, so that is just:

	sub	%lo, %lo, #offset

and nothing more.

The existing fixup code runs too early - before we get a chance to switch over from the boot-time sub-4G alias to the real physical address space. Readability aside, part of the reasoning behind the C re-implementation was to defer the patch application until a later point in the boot process.

That said, we could patch once at boot, and then re-patch at switch over, I guess. Is that what you're recommending?

The existing fixup code runs too early - before we get a chance to switch over from the boot-time sub-4G alias to the real physical address space. Readability aside, part of the reasoning behind the C re-implementation was to defer the patch application until a later point in the boot process.

That said, we could patch once at boot, and then re-patch at switch over, I guess. Is that what you're recommending?

Quoted 4 lines · Nicolas Pitre

We don't want to limit lowmem. The lowmem is very precious memory and we want to maximize its size. In that case it is probably best to implement a real patchable 64-bit addition rather than artificially restricting the lowmem size.

You don't need to. You can solve the V->P translation like this:

        movw    %hi, #0xVWXY            @ fixup
        adds    %lo, %lo, #offset       @ fixup
        adc     %hi, %hi, #0

which is probably the simplest way to do the fixup - keep the existing fixup code, and add support for that movw instruction. And that will work across any 4GB boundary just fine (we won't have more than 4GB of virtual address space on a 32-bit CPU anyway, so we only have to worry about one carry.)

And the P->V translation is truncating anyway, so that is just:

	sub	%lo, %lo, #offset

and nothing more.

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

We don't want to limit lowmem. The lowmem is very precious memory and we want to maximize its size. In that case it is probably best to implement a real patchable 64-bit addition rather than artificially restricting the lowmem size.

You don't need to. You can solve the V->P translation like this:

        movw    %hi, #0xVWXY            @ fixup
        adds    %lo, %lo, #offset       @ fixup
        adc     %hi, %hi, #0

That's exactly what I mean when I say a real 64-bit addition. Despite one of the arguments being a 32 bit value, the overflow needs to be carried to the high part.

Quoted 5 lines

which is probably the simplest way to do the fixup - keep the existing fixup code, and add support for that movw instruction. And that will work across any 4GB boundary just fine (we won't have more than 4GB of virtual address space on a 32-bit CPU anyway, so we only have to worry about one carry.)

Exactly.

Quoted 5 lines

And the P->V translation is truncating anyway, so that is just:

	sub	%lo, %lo, #offset

and nothing more.

Yes, that's what the posted code does already.

Quoted 35 lines · Catalin Marinas

Quoted 34 lines · Nicolas Pitre

Quoted 16 lines · Cyril Chemparathy

Quoted 15 lines · Nicolas Pitre

Quoted 12 lines · Catalin Marinas

Quoted 5 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 7 lines

Quoted 2 lines

Good point. We better get an early warning if that happens.

Thanks.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

No. That's not the point here.

Let's suppose a system with 1GB of physical RAM starting at 0xe0000000.

In this case there is no need for highmem. However the v2p patching code in the LPAE case assumes the high bits of a physical address are always the same which wouldn't be the case in this hypothetical example.

Quoted 5 lines

Quoted 2 lines

We want to make sure the kernel won't boot if a system with physical RAM crossing the 4GB address mark is encountered.

I think that's too restrictive. The case with 1 or 2GB below 4GB limit and the rest of the RAM above is a valid one.

Valid: sure. Likely: probably not. That would complicate address decoding in hardware for little gain.

Quoted 16 lines · Cyril Chemparathy

Quoted 15 lines · Nicolas Pitre

Quoted 12 lines · Catalin Marinas

Quoted 5 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.

So this assumes that for the kernel linear mapping, all the physical addresses have the same upper 32-bit. That's a good optimisation but I haven't seen this check when calculating lowmem in sanity_check_meminfo. Someone may build platform with memory starting at 3GB and going across the 4GB limit.

Quoted 7 lines

Quoted 2 lines

Good point. We better get an early warning if that happens.

Thanks.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

No. That's not the point here.

Let's suppose a system with 1GB of physical RAM starting at 0xe0000000.

In this case there is no need for highmem. However the v2p patching code in the LPAE case assumes the high bits of a physical address are always the same which wouldn't be the case in this hypothetical example.

We want to make sure the kernel won't boot if a system with physical RAM crossing the 4GB address mark is encountered.

I'm thinking of splitting the bank at the 32-bit boundary in such an event, assuming that the remaining memory should be usable as highmem.

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

There is no corresponding change on the phys_to_virt() side, because computations on the upper 32-bits would be discarded anyway.

Signed-off-by Cyril Chemparathy <[email protected]>

---
 arch/arm/include/asm/memory.h |   38 ++++++++++++++++++++++++++++++++++++--
 arch/arm/kernel/head.S        |    4 ++++
 arch/arm/kernel/setup.c       |    2 +-
 3 files changed, 41 insertions(+), 3 deletions(-)

arch/arm/include/asm/memory.h +25 -2

index 88ca206..f3e8f88 100644
@@ -154,13 +154,47 @@
 #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
 
 extern unsigned long	__pv_offset;
-extern unsigned long	__pv_phys_offset;
+extern phys_addr_t	__pv_phys_offset;
 #define PHYS_OFFSET	__virt_to_phys(PAGE_OFFSET)
 
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
-	unsigned long t;
+	phys_addr_t t;
+
+#ifndef CONFIG_ARM_LPAE
 	early_patch_imm8("add", t, x, __pv_offset, 0);
+#else
+	unsigned long __tmp;
+
+#ifndef __ARMEB__
+#define PV_PHYS_HIGH	"(__pv_phys_offset + 4)"
+#else
+#define PV_PHYS_HIGH	"__pv_phys_offset"
+#endif
+
+	early_patch_stub(
+	/* type */		PATCH_IMM8,
+	/* code */
+		"ldr		%[tmp], =__pv_offset\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"add		%Q[to], %[from], %[tmp]\n"
+		"ldr		%[tmp], =" PV_PHYS_HIGH "\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"mov		%R[to], %[tmp]\n",
+	/* pad */		4,
+	/* patch_data */
+		".long		__pv_offset\n"

Quoted 54 lines · Cyril Chemparathy

This patch adds support for 64-bit physical addresses in virt_to_phys() patching. This does not do real 64-bit add/sub, but instead patches in the upper 32-bits of the phys_offset directly into the output of virt_to_phys.

There is no corresponding change on the phys_to_virt() side, because computations on the upper 32-bits would be discarded anyway.

Signed-off-by Cyril Chemparathy <[email protected]>

---
 arch/arm/include/asm/memory.h |   38 ++++++++++++++++++++++++++++++++++++--
 arch/arm/kernel/head.S        |    4 ++++
 arch/arm/kernel/setup.c       |    2 +-
 3 files changed, 41 insertions(+), 3 deletions(-)

arch/arm/include/asm/memory.h +25 -2

index 88ca206..f3e8f88 100644
@@ -154,13 +154,47 @@
 #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
 
 extern unsigned long	__pv_offset;
-extern unsigned long	__pv_phys_offset;
+extern phys_addr_t	__pv_phys_offset;
 #define PHYS_OFFSET	__virt_to_phys(PAGE_OFFSET)
 
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
-	unsigned long t;
+	phys_addr_t t;
+
+#ifndef CONFIG_ARM_LPAE
 	early_patch_imm8("add", t, x, __pv_offset, 0);
+#else
+	unsigned long __tmp;
+
+#ifndef __ARMEB__
+#define PV_PHYS_HIGH	"(__pv_phys_offset + 4)"
+#else
+#define PV_PHYS_HIGH	"__pv_phys_offset"
+#endif
+
+	early_patch_stub(
+	/* type */		PATCH_IMM8,
+	/* code */
+		"ldr		%[tmp], =__pv_offset\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"add		%Q[to], %[from], %[tmp]\n"
+		"ldr		%[tmp], =" PV_PHYS_HIGH "\n"
+		"ldr		%[tmp], [%[tmp]]\n"
+		"mov		%R[to], %[tmp]\n",
+	/* pad */		4,
+	/* patch_data */
+		".long		__pv_offset\n"

arch/arm/include/asm/memory.h line 196

Quoted 10 lines

arch/arm/include/asm/memory.h +10 -0

+		"add		%Q[to], %[from], %[imm]\n"
+		".long	"	PV_PHYS_HIGH "\n"
+		"mov		%R[to], %[imm]\n",
+	/* operands */
+		: [to]	 "=r"	(t),
+		  [tmp]	 "=&r"	(__tmp)
+		: [from] "r"	(x),
+		  [imm]	 "I"	(__IMM8),
+			 "i"	(&__pv_offset),
+			 "i"	(&__pv_phys_offset));

So, the actual offset we can apply is:

__pv_phys_offset + __pv_offset

where:

* the high 32 bits of the address being fixed up are assumed to be 0 (true, because the kernel is initially always fixed up to an address range <4GB)

* the low 32 bits of __pv_phys_offset are assumed to be 0 (?)

* the full offset is of the form

        ([..0..]XX[..0..] << 32) | [..0..]YY[..0..] 

Is this intentional? It seems like a rather weird constraint... but it may be sensible. PAGE_OFFSET is probably 0xc0000000 or 0x80000000, (so YY can handle that) and the actual RAM above 4GB will likely be huge and aligned on some enormous boundary in such situations (so that XX can handle that).

So long as the low RAM alias is not misaligned relative to the high alias on a finer granularity than 16MB (so that YY = (PAGE_OFFSET +/- the misalignment) is still a legal immediate), I guess there should not be a problem.

This patch applies to PAGE_MASK, PMD_MASK, and PGDIR_MASK, where forcing unsigned long math truncates the mask at the 32-bits. This clearly does bad things on PAE systems.

This patch fixes this problem by defining these masks as signed quantities. We then rely on sign extension to do the right thing.

Signed-off-by Cyril Chemparathy <[email protected]>

Signed-off-by Vitaly Andrianov <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

This patch applies to PAGE_MASK, PMD_MASK, and PGDIR_MASK, where forcing unsigned long math truncates the mask at the 32-bits. This clearly does bad things on PAE systems.

This patch fixes this problem by defining these masks as signed quantities. We then rely on sign extension to do the right thing.

Signed-off-by Cyril Chemparathy <[email protected]>

Signed-off-by Vitaly Andrianov <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

---
 arch/arm/include/asm/page.h           |    2 +-
 arch/arm/include/asm/pgtable-3level.h |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

arch/arm/include/asm/page.h +0 -0

index ecf9019..1e0fe08 100644

arch/arm/include/asm/page.h +0 -0

 #ifndef __ASSEMBLY__
 

arch/arm/include/asm/pgtable-3level.h +1 -1

index b249035..ae39d11 100644
@@ -48,16 +48,16 @@
 #define PMD_SHIFT		21
 
 #define PMD_SIZE		(1UL << PMD_SHIFT)
-#define PMD_MASK		(~(PMD_SIZE-1))
+#define PMD_MASK		(~((1 << PMD_SHIFT) - 1))
 #define PGDIR_SIZE		(1UL << PGDIR_SHIFT)

From: Vitaly Andrianov <[email protected]>

This patch fixes the alloc_init_pud() function to use phys_addr_t instead of unsigned long when passing in the phys argument.

This is an extension to commit 97092e0c56830457af0639f6bd904537a150ea4a (ARM:
pgtable: use phys_addr_t for physical addresses), which applied similar changes elsewhere in the ARM memory management code.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Cyril Chemparathy <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

From: Vitaly Andrianov <[email protected]>

The free_memmap() was mistakenly using unsigned long type to represent physical addresses. This breaks on PAE systems where memory could be placed above the 32-bit addressible limit.

This patch fixes this function to properly use phys_addr_t instead.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Cyril Chemparathy <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

---
 arch/arm/mm/init.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/arm/mm/init.c +1 -1

index 9aec41f..19ba70b 100644
@@ -457,7 +457,7 @@ static inline void
 free_memmap(unsigned long start_pfn, unsigned long end_pfn)
 {
 	struct page *start_pg, *end_pg;
-	unsigned long pg, pgend;
+	phys_addr_t pg, pgend;
 
 	/*
 	 * Convert start_pfn/end_pfn to a struct page pointer.

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Quoted 4 lines · Catalin Marinas

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Bootmem only deals with physical addresses which fit within the size of an 'unsigned long'. Unfortunately, the bootmem API is a mess of 'unsigned long' physical addresses and PFNs.

Years ago there was a patch to make it use only PFNs but other changes resulted in that patch being thrown away.

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Bootmem only deals with physical addresses which fit within the size of an 'unsigned long'. Unfortunately, the bootmem API is a mess of 'unsigned long' physical addresses and PFNs.

Quoted 3 lines

Years ago there was a patch to make it use only PFNs but other changes resulted in that patch being thrown away.

A separate patch has been posted for bootmem (see [1]).

Tejun suggested that we'd be better off moving entirely to memblock instead (see [2]).

Quoted 11 lines · Cyril Chemparathy

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Bootmem only deals with physical addresses which fit within the size of an 'unsigned long'. Unfortunately, the bootmem API is a mess of 'unsigned long' physical addresses and PFNs.

Quoted 8 lines

Quoted 3 lines

Years ago there was a patch to make it use only PFNs but other changes resulted in that patch being thrown away.

A separate patch has been posted for bootmem (see [1]).

Tejun suggested that we'd be better off moving entirely to memblock instead (see [2]).

Yes we should, but that's not as easy as typing a few words in an email. When I tried it when I integrated memblock into our boot sequence, I found that sparsemem wouldn't work without bootmem being in place.

It may be that things have now moved on, and we can just eliminate bootmem once and for all, but that's something I've not looked at for quite some time.

Quoted 25 lines · Russell King - ARM Linux

Quoted 11 lines · Cyril Chemparathy

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Bootmem only deals with physical addresses which fit within the size of an 'unsigned long'. Unfortunately, the bootmem API is a mess of 'unsigned long' physical addresses and PFNs.

Quoted 8 lines

Quoted 3 lines

Years ago there was a patch to make it use only PFNs but other changes resulted in that patch being thrown away.

A separate patch has been posted for bootmem (see [1]).

Tejun suggested that we'd be better off moving entirely to memblock instead (see [2]).

Yes we should, but that's not as easy as typing a few words in an email. When I tried it when I integrated memblock into our boot sequence, I found that sparsemem wouldn't work without bootmem being in place.

Quoted 4 lines

It may be that things have now moved on, and we can just eliminate bootmem once and for all, but that's something I've not looked at for quite some time.

It appears to be not that hard actually... Or maybe I'm totally missing your point. Could it be that you last looked at this prior to the nobootmem compatibility stuff being added in?

+static unsigned long free_all_lowmem(void)

Quoted 26 lines · Cyril Chemparathy

Quoted 25 lines · Russell King - ARM Linux

Quoted 11 lines · Cyril Chemparathy

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

This function also calls free_bootmem() which takes unsigned long. Are patches sent separately for this or we just ignore holes in memmap? There are other calls to free_bootmem() or reserve_bootmem(), do they just work with the high phys addresses?

Bootmem only deals with physical addresses which fit within the size of an 'unsigned long'. Unfortunately, the bootmem API is a mess of 'unsigned long' physical addresses and PFNs.

Quoted 8 lines

Quoted 3 lines

Years ago there was a patch to make it use only PFNs but other changes resulted in that patch being thrown away.

A separate patch has been posted for bootmem (see [1]).

Tejun suggested that we'd be better off moving entirely to memblock instead (see [2]).

Yes we should, but that's not as easy as typing a few words in an email. When I tried it when I integrated memblock into our boot sequence, I found that sparsemem wouldn't work without bootmem being in place.

Quoted 8 lines

Quoted 4 lines

It may be that things have now moved on, and we can just eliminate bootmem once and for all, but that's something I've not looked at for quite some time.

It appears to be not that hard actually... Or maybe I'm totally missing your point. Could it be that you last looked at this prior to the nobootmem compatibility stuff being added in?

I was just trying the same :)

Quoted 1 line

+static unsigned long free_all_lowmem(void)

I don't think that's needed. free_all_bootmem() in mm/nobootmem.c takes care of freeing lowmem but it has a different notion of max_low_pfn. So this hunk did the trick:

(quoted diff) +2 -2

@@ -420,8 +366,8 @@ void __init bootmem_init(void)
  	 * Note: max_low_pfn and max_pfn reflect the number of _pages_ in
  	 * the system, not the maximum PFN.
  	 */
-	max_low_pfn = max_low - PHYS_PFN_OFFSET;
-	max_pfn = max_high - PHYS_PFN_OFFSET;
+	max_low_pfn = max_low;
+	max_pfn = max_high;
  }
 
  static inline int free_area(unsigned long pfn, unsigned long end, char *s)

Review void __init bootmem_init(void) · line 371

Quoted 13 lines · Catalin Marinas

I don't think that's needed. free_all_bootmem() in mm/nobootmem.c takes care of freeing lowmem but it has a different notion of max_low_pfn. So this hunk did the trick:

(quoted diff) +2 -2

@@ -420,8 +366,8 @@ void __init bootmem_init(void)
 	 * Note: max_low_pfn and max_pfn reflect the number of _pages_ in
 	 * the system, not the maximum PFN.
 	 */
-	max_low_pfn = max_low - PHYS_PFN_OFFSET;
-	max_pfn = max_high - PHYS_PFN_OFFSET;
+	max_low_pfn = max_low;
+	max_pfn = max_high;
 }

Did you actually look to see where that's used before you made the change. I don't think you did.

The reason we have that there is that much of the kernel assumes memory always starts at physical zero, so the max*pfn variables can be used to generate bitmasks to cover the range of system memory addresses - iow, (1 << max_low_pfn) - 1.

Eg, in the block code:

        blk_max_low_pfn = max_low_pfn - 1;
        blk_max_pfn = max_pfn - 1;
...
        unsigned long b_pfn = dma_mask >> PAGE_SHIFT;

        if (b_pfn < blk_max_low_pfn)
                dma = 1;

Having a DMA mask for a peripheral which only has 24 bits wired (so 0x00ffffff) with a system memory offset of 0xc0000000 results in apparantly _all_ system memory being DMA-able according to this test unless max_low_pfn is defined as the _number_ of bits in the RAM address.

In dma_get_required_mask():

        u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);

                low_totalram = (1 << (fls(low_totalram) - 1));
                low_totalram += low_totalram - 1;

which results in (for a phys offset of 0xc0000000) low_totalram being 0xffffffff unconditionally no matter how much RAM you actually have.

Quoted 43 lines · Russell King - ARM Linux

Review void __init bootmem_init(void) · line 371

Quoted 13 lines · Catalin Marinas

I don't think that's needed. free_all_bootmem() in mm/nobootmem.c takes care of freeing lowmem but it has a different notion of max_low_pfn. So this hunk did the trick:

(quoted diff) +2 -2

@@ -420,8 +366,8 @@ void __init bootmem_init(void)
 	 * Note: max_low_pfn and max_pfn reflect the number of _pages_ in
 	 * the system, not the maximum PFN.
 	 */
-	max_low_pfn = max_low - PHYS_PFN_OFFSET;
-	max_pfn = max_high - PHYS_PFN_OFFSET;
+	max_low_pfn = max_low;
+	max_pfn = max_high;
 }

Did you actually look to see where that's used before you made the change. I don't think you did.

The reason we have that there is that much of the kernel assumes memory always starts at physical zero, so the max*pfn variables can be used to generate bitmasks to cover the range of system memory addresses - iow, (1 << max_low_pfn) - 1.

Eg, in the block code:

        blk_max_low_pfn = max_low_pfn - 1;
        blk_max_pfn = max_pfn - 1;
...
        unsigned long b_pfn = dma_mask >> PAGE_SHIFT;

        if (b_pfn < blk_max_low_pfn)
                dma = 1;

Having a DMA mask for a peripheral which only has 24 bits wired (so 0x00ffffff) with a system memory offset of 0xc0000000 results in apparantly _all_ system memory being DMA-able according to this test unless max_low_pfn is defined as the _number_ of bits in the RAM address.

In dma_get_required_mask():

        u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);

Quoted 5 lines

                low_totalram = (1 << (fls(low_totalram) - 1));
                low_totalram += low_totalram - 1;

which results in (for a phys offset of 0xc0000000) low_totalram being 0xffffffff unconditionally no matter how much RAM you actually have.

And for those platforms the phys and bus (dma) addresses are different.

So it's not about whether the physical RAM starts at 0 but whether the device has a different view of the RAM address space.

The reverse could
also be problematic if phys == bus and max_low_pfn corresponds to an address that's not actually accessible for the device (though in practice I don't expect this).

It appears to be not that hard actually... Or maybe I'm totally missing your point. Could it be that you last looked at this prior to the nobootmem compatibility stuff being added in?

The following patch appears to work just fine on a faked sparsemem system:

arch/arm/Kconfig +0 -0

index 92f598a..e94fa4c 100644
@@ -49,6 +49,7 @@ config ARM
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
 	select DCACHE_WORD_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) &&  

!CPU_BIG_ENDIAN

arch/arm/Kconfig +1 -0

+	select NO_BOOTMEM
 	help
 	  The ARM series is a line of low-power-consumption RISC chip designs
 	  licensed by ARM Ltd and targeted at embedded applications and

arch/arm/mm/init.c +0 -52

index 29560d8..0ca6c00 100644
@@ -150,58 +150,6 @@ static void __init find_limits(unsigned long *min,  
 unsigned long *max_low,
 	*max_high = bank_pfn_end(&mi->bank[mi->nr_banks - 1]);
 }
 
-static void __init arm_bootmem_init(unsigned long start_pfn,
-	unsigned long end_pfn)
-{
-	struct memblock_region *reg;
-	unsigned int boot_pages;
-	phys_addr_t bitmap;
-	pg_data_t *pgdat;
-
-	/*
-	 * Allocate the bootmem bitmap page.  This must be in a region
-	 * of memory which has already been mapped.
-	 */
-	boot_pages = bootmem_bootmap_pages(end_pfn - start_pfn);
-	bitmap = memblock_alloc_base(boot_pages << PAGE_SHIFT, L1_CACHE_BYTES,
-				__pfn_to_phys(end_pfn));
-
-	/*
-	 * Initialise the bootmem allocator, handing the
-	 * memory banks over to bootmem.
-	 */
-	node_set_online(0);
-	pgdat = NODE_DATA(0);
-	init_bootmem_node(pgdat, __phys_to_pfn(bitmap), start_pfn, end_pfn);
-
-	/* Free the lowmem regions from memblock into bootmem. */
-	for_each_memblock(memory, reg) {
-		unsigned long start = memblock_region_memory_base_pfn(reg);
-		unsigned long end = memblock_region_memory_end_pfn(reg);
-
-		if (end >= end_pfn)
-			end = end_pfn;
-		if (start >= end)
-			break;
-
-		free_bootmem(__pfn_to_phys(start), (end - start) << PAGE_SHIFT);
-	}
-
-	/* Reserve the lowmem memblock reserved regions in bootmem. */
-	for_each_memblock(reserved, reg) {
-		unsigned long start = memblock_region_reserved_base_pfn(reg);
-		unsigned long end = memblock_region_reserved_end_pfn(reg);
-
-		if (end >= end_pfn)
-			end = end_pfn;
-		if (start >= end)
-			break;
-
-		reserve_bootmem(__pfn_to_phys(start),
-			        (end - start) << PAGE_SHIFT, BOOTMEM_DEFAULT);
-	}
-}
-
 #ifdef CONFIG_ZONE_DMA
 

unsigned long arm_dma_zone_size __read_mostly;

arch/arm/mm/init.c +48 -2

@@ -393,8 +341,6 @@ void __init bootmem_init(void)
 
 	find_limits(&min, &max_low, &max_high);
 
-	arm_bootmem_init(min, max_low);
-
 	/*
 	 * Sparsemem tries to allocate bootmem in memory_present(),
 	 * so must be done after the fixed reservations

@@ -443,6 +389,54 @@ static inline int free_area(unsigned long pfn,  
 unsigned long end, char *s)
 	return pages;
 }
 
+static unsigned long free_all_lowmem(void)
+{
+	unsigned long max_low = max_low_pfn + PHYS_PFN_OFFSET;
+	struct memblock_region *mem, *res;
+	unsigned long total_pages = 0;
+
+	/* set highmem page free */
+	for_each_memblock(memory, mem) {
+		unsigned long start = memblock_region_memory_base_pfn(mem);
+		unsigned long end = memblock_region_memory_end_pfn(mem);
+
+		/* Ignore complete lowmem entries */
+		if (start > max_low)
+			continue;
+
+		/* Truncate partial highmem entries */
+		if (end > max_low)
+			end = max_low;
+
+		/* Find and exclude any reserved regions */
+		for_each_memblock(reserved, res) {
+			unsigned long res_start, res_end;
+
+			res_start = memblock_region_reserved_base_pfn(res);
+			res_end = memblock_region_reserved_end_pfn(res);
+
+			if (res_end < start)
+				continue;
+			if (res_start < start)
+				res_start = start;
+			if (res_start > end)
+				res_start = end;
+			if (res_end > end)
+				res_end = end;
+			if (res_start != start)
+				total_pages += free_area(start, res_start, NULL);
+			start = res_end;
+			if (start == end)
+				break;
+		}
+
+		/* And now free anything which remains */
+		if (start < end)
+			total_pages += free_area(start, end, NULL);
+	}
+	return total_pages;
+}
+
 /*
  * Poison init memory with an undefined instruction (ARM) or a branch  

to an
* undefined instruction (Thumb).

arch/arm/mm/init.c +1 -1

@@ -606,7 +600,7 @@ void __init mem_init(void)
 	/* this will put all unused low memory onto the freelists */
 	free_unused_memmap(&meminfo);
 
-	totalram_pages += free_all_bootmem();
+	totalram_pages += free_all_lowmem();
 
 #ifdef CONFIG_SA1111
 	/* now that our DMA memory is actually so designated, we can free it */

Signature · 3 lines

-- 
Thanks
- Cyril

Quoted 3 lines · Cyril Chemparathy

It appears to be not that hard actually... Or maybe I'm totally missing your point. Could it be that you last looked at this prior to the nobootmem compatibility stuff being added in?

Quite possible - we converted over to memblock here:

commit 2778f62056ada442414392d7ccd41188bb631619

Author: Russell King <[email protected]>
Date:   Fri Jul 9 16:27:52 2010 +0100

It looks like nobootmem.c was created here:

commit 0932587328d9bd5b500a640fbaff3290c8d4cabf

Author: Yinghai Lu <[email protected]>
Date:   Thu Feb 24 14:43:05 2011 +0100

So, when I was porting ARM to memblock, we had:

commit 08677214e318297f228237be0042aac754f48f1d

Author: Yinghai Lu <[email protected]>
Date:   Wed Feb 10 01:20:20 2010 -0800

which causes a load of bootmem functions to panic() when bootmem is disabled.

Quoted 1 line

The following patch appears to work just fine on a faked sparsemem system:

Great. Don't forget to also check flatmem too, just to be sure.

From: Vitaly Andrianov <[email protected]>

This patch fixes the initrd setup code to use phys_addr_t instead of assuming 32-bit addressing. Without this we cannot boot on systems where initrd is located above the 4G physical address limit.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Cyril Chemparathy <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

From: Vitaly Andrianov <[email protected]>

This patch fixes the initrd setup code to use phys_addr_t instead of assuming 32-bit addressing. Without this we cannot boot on systems where initrd is located above the 4G physical address limit.

Quoted 5 lines · Cyril Chemparathy

From: Vitaly Andrianov <[email protected]>

This patch fixes the initrd setup code to use phys_addr_t instead of assuming 32-bit addressing. Without this we cannot boot on systems where initrd is located above the 4G physical address limit.

The description needs updating. You're no longer using phys_addr_t for the initrd size.