Re: [PATCH v3 RESEND 01/17] ARM: add mechanism for late code patching

[Nicolas Pitre <nicolas.pitre@linaro.org>]

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.


Nicolas