[lkml]   [2009]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH/RFT 0/13] x86: unify
    > > 
    > > o 64 bit uses PHDRS more extensively than 32 bit. Could they be the same?
    > Hm, PHDRS content really matters mostly for the vDSO, so that gdb
    > can treat the vsyscall entry page(s) more as a normal DSO.
    > for the kernel image itself it does not matter much how standard of
    > an ELF binary it is: the boot loader does not care about the PHDR
    > description of linker segments and we dont execute the binary.
    > UML and lguest has its own ELF-binary creation methods.
    > I think the only relevancy it has on the kernel image is on readonly
    > symbols: the PHDRS command gives a reasonable default flags value to
    > various segments. We _usually_ give all segments their proper
    > permission explicitly - but it was not unheard of to have mixups
    > there and to see supposedly-readonly sections end up in a rw area or
    > for rw sections to end up in the readonly section.
    > The latter will be found quickly because it triggers a kernel crash
    > - the former kind of bug can linger for a long time.
    > So i think we should generate proper PHDRS (i.e. use the 64-bit
    > linker script portion to also include percpu and init-data bits),
    > for consistency.
    > Do you know what the linker does if the PHDRS and the section flags
    > collide? Does the local flag override the PHDRS flag? I havent
    > checked.

    I have not looked much into the linker support of PHDRS.
    Which is also why I did not dare touching this stuff.

    From 'info ld':
    The linker will normally set the segment flags based on the sections
    which comprise the segment. You may use the `FLAGS' keyword to
    explicitly specify the segment flags.

    So the PHDRS settings take effect.

    > > o _stext does not cover all text for 32 bit - a bug? For 64b bit it does.
    > > It is only the .code16 wakeup stuff that is not covered but anyway.
    > that's a bug that should be fixed. Harmless - but needs some testing
    > - there are tools (profilers, etc.) that might have assumptions
    > about _stext so this needs some test-time.
    > Also, _stext is the start address for the readonly section - so by
    > moving it down a bit on 32-bit we extend readonly to that .code16
    > suspend code. If it contains any self-modifying code it will crash.

    hpa should know about the latter.
    I suggest to give the current patchset some air time before we move _stext.

    > > o _edata covers much more on 32 bit
    > 32-bit is corret there. We do use _edata in a couple of places, such
    > as in resource ranges - so there could be side-effects, but any such
    > side effects would likely show some real hidden bug or uncleanliness
    > so it's good to fix that.

    OK - again if we could wait a bit with this change it would be good.

    > > o The nosave stuff differs (but that is due to the PHDRS stuff anyway)
    > nosavedata is a really ancient construct used almost nowhere. That's
    > a question to Rafael and Linus: can we just get rid of it? The only
    > user seems to be:
    > int in_suspend __nosavedata = 0;

    A lot of stuff added just to support a single integer..
    If we could get rid of that it would be great.

    > > o Different alignmnet requirements in several spots
    > do you have a list of them? There's hpa's fix from yesterday that
    > shows that we have real bugs there.

    There are two places - pasted below.
    #ifdef CONFIG_X86_32
    . = ALIGN(32);
    .data.cacheline_aligned :

    #ifdef CONFIG_X86_32
    . = ALIGN(32);
    .data.read_mostly :

    > > o All the stuff added to support relocable kernels
    > hpa found a bug (well, misfeature) in the relocatable kernel code
    > too.

    If I understood this correct we had an issue that the start address of
    the section was no aligned because the ALIGN() was located inside
    the output section.

    That should not be a problem after applying this patchset as
    they are almost all moved out.
    I left them in the output section where they:
    - are used to align the end address of an output section
    - for .text where the .code16 had special requirments to avoid hurting 64 bit
    - for .data_nosave on 64 bit - because I forgot to delete it
    The latter is a noop since we have an identical ALING() just above it


     \ /
      Last update: 2009-04-29 10:25    [W:0.039 / U:1.488 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site