lkml.org 
[lkml]   [2009]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFT 0/13] x86: unify vmlinux.lds
> > 
> > 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.
1)
#ifdef CONFIG_X86_32
. = ALIGN(32);
#else
. = ALIGN(PAGE_SIZE);
. = ALIGN(CONFIG_X86_L1_CACHE_BYTES);
#endif
.data.cacheline_aligned :


2)
#ifdef CONFIG_X86_32
. = ALIGN(32);
#else
. = ALIGN(CONFIG_X86_INTERNODE_CACHE_BYTES);
#endif
.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


Sam


\
 
 \ /
  Last update: 2009-04-29 10:25    [from the cache]
©2003-2011 Jasper Spaans