lkml.org 
[lkml]   [2012]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 02/22] ARM: use late patch framework for phys-virt patching
    On Sun, 5 Aug 2012, Cyril Chemparathy wrote:

    > Hi Nicolas,
    >
    > On 8/4/2012 2:15 AM, Nicolas Pitre wrote:
    > > On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
    > >
    > > > This patch replaces the original physical offset patching implementation
    > > > with one that uses the newly added patching framework. In the process, we
    > > > now
    > > > unconditionally initialize the __pv_phys_offset and __pv_offset globals in
    > > > the
    > > > head.S code.
    > >
    > > Why unconditionally initializing those? There is no reason for that.
    > >
    >
    > We could keep this conditional on LPAE, but do you see any specific need for
    > keeping it conditional?

    This has nothing to do with LPAe. This is about
    CONFIG_ARM_PATCH_PHYS_VIRT only. If not selected, those global
    vaariables have no need to exist.

    > > Comments below.
    > >
    > > > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
    > > > index 835898e..d165896 100644
    > > > --- a/arch/arm/kernel/head.S
    > > > +++ b/arch/arm/kernel/head.S
    > > [...]
    > > > .data
    > > > .globl __pv_phys_offset
    > > > .type __pv_phys_offset, %object
    > > > __pv_phys_offset:
    > > > .long 0
    > > > .size __pv_phys_offset, . - __pv_phys_offset
    > > > +
    > > > + .globl __pv_offset
    > > > + .type __pv_offset, %object
    > > > __pv_offset:
    > > > .long 0
    > > > -#endif
    > > > + .size __pv_offset, . - __pv_offset
    > >
    > > Please move those to C code. They aren't of much use in this file
    > > anymore. This will allow you to use pphys_addr_t for them as well in
    > > your subsequent patch. And more importantly get rid of that ugly
    > > pv_offset_high that you introduced iin another patch.
    > >
    >
    > Moving it to C-code caused problems because these get filled in prior to BSS
    > being cleared.
    >
    > We could potentially have this initialized in C with a mystery dummy value to
    > prevent it from landing in BSS. Would that be acceptable?

    Just initialize them explicitly to zero. They will end up in .ddata
    section.
    >
    > > > index df5e897..39f8fce 100644
    > > > --- a/arch/arm/kernel/module.c
    > > > +++ b/arch/arm/kernel/module.c
    > > > @@ -317,11 +317,6 @@ int module_finalize(const Elf32_Ehdr *hdr, const
    > > > Elf_Shdr *sechdrs,
    > > > maps[i].txt_sec->sh_addr,
    > > > maps[i].txt_sec->sh_size);
    > > > #endif
    > > > -#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
    > > > - s = find_mod_section(hdr, sechdrs, ".pv_table");
    > > > - if (s)
    > > > - fixup_pv_table((void *)s->sh_addr, s->sh_size);
    > > > -#endif
    > > > s = find_mod_section(hdr, sechdrs, ".patch.table");
    > > > if (s)
    > > > patch_kernel((void *)s->sh_addr, s->sh_size);
    > >
    > > The patch_kernel code and its invokation should still be conditional on
    > > CONFIG_ARM_PATCH_PHYS_VIRT. This ability may still be configured out
    > > irrespective of the implementation used.
    > >
    >
    > Maybe CONFIG_ARM_PATCH_PHYS_VIRT is not quite appropriate if this is used to
    > patch up other things in addition to phys-virt stuff?

    Maybe, but at the moment this is not the case.

    > I could have this dependent on CONFIG_ARM_INIT_PATCH (or whatever nomenclature
    > we chose for this) and have CONFIG_ARM_PATCH_PHYS_VIRT depend on it.

    Let's cross that bridge in time.

    FWIW, I don't like "init patch" much. I feel like the "runtime"
    qualifier more pricisely describe this code than "init".


    Nicolas


    \
     
     \ /
      Last update: 2012-08-06 04:41    [W:7.242 / U:0.264 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site