[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.167 / U:2.220 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site