Messages in this thread | | | Date | Sat, 22 Sep 2012 17:41:24 -0400 | From | Cyril Chemparathy <> | Subject | Re: [PATCH v3 RESEND 01/17] ARM: add mechanism for late code patching |
| |
On 09/22/12 11:10, Nicolas Pitre wrote: > On Fri, 21 Sep 2012, Cyril Chemparathy wrote: > >> 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 <cyril@ti.com> >> Reviewed-by: Nicolas Pitre <nico@linaro.org> > > There is another problem with this. > > [...] >> diff --git a/arch/arm/include/asm/runtime-patch.h b/arch/arm/include/asm/runtime-patch.h >> new file mode 100644 >> index 0000000..366444d >> --- /dev/null >> +++ b/arch/arm/include/asm/runtime-patch.h >> @@ -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]; >> +}; > > 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. >
Thanks for trying this out. I'll take this up in the next rev.
-- Cyril.
| |