lkml.org 
[lkml]   [2012]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 01/17] ARM: add mechanism for late code patching
On Tue, 11 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>

I know I provided review before, but here's another nit I'd like fixed:

> --- /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

[...]

> +#else
> +
> +static inline int runtime_patch(const void *table, unsigned size)
> +{
> + return 0;
> +}

This is wrong. If runtime_patch() is ever called when
CONFIG_ARM_RUNTIME_PATCH is not set, then i'd better return an error and
not pretend it performed the requested action. Returning -ENOSYS would
be appropriate.

This is especially important in the following context:

> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -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;
> + }

Despite the vermagic check, If ever a .runtime.patch.table section is
found in a module to be loaded in a kernel with no support for it then
it is best to return an error than see the kernel crashing later on when
the fallback stubs have been discarded.


Nicolas


\
 
 \ /
  Last update: 2012-09-21 20:41    [W:0.245 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site