Message-ID: <505E3084.80307@ti.com>
Date:	Sat, 22 Sep 2012 17:41:24 -0400
From:	Cyril Chemparathy <cyril@...com>
To:	Nicolas Pitre <nicolas.pitre@...aro.org>
CC:	<linux@....linux.org.uk>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>, <arnd@...db.de>,
	<catalin.marinas@....com>, <davidb@...eaurora.org>,
	<frank.rowand@...sony.com>, <hsweeten@...ionengravers.com>,
	<linus.walleij@...aro.org>, <marc.zyngier@....com>,
	<paul.gortmaker@...driver.com>, <plagnioj@...osoft.com>,
	<rabin@....in>, <rmallon@...il.com>, <rob.herring@...xeda.com>,
	<sboyd@...eaurora.org>, <sjg@...omium.org>, <tglx@...utronix.de>,
	<tj@...nel.org>, <vincent.guittot@...aro.org>, <vitalya@...com>,
	<will.deacon@....com>, <grant.likely@...retlab.ca>
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@...com>
>> Reviewed-by: Nicolas Pitre <nico@...aro.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists