Module CRCs are implemented as absolute symbols that get resolved by
a linker script. We build an intermediate .o that contains an
unresolved symbol for each CRC. genksysms parses this .o, calculates
the CRCs and writes a linker script that "resolves" the symbols to
the calculated CRC.
Unfortunately the ppc64 relocatable kernel sees these CRCs as symbols
that need relocating and relocates them at boot. Commit d4703aef
(module: handle ppc64 relocating kcrctabs when CONFIG_RELOCATABLE=y)
added a hook to reverse the bogus relocations. Part of this patch
created a symbol at 0x0:
# head -2 /proc/kallsyms
0000000000000000 T reloc_start
c000000000000000 T .__start
This reloc_start symbol is causing lots of confusion to perf. It
thinks reloc_start is a massive function that stretches from 0x0 to
0xc000000000000000 and we get various cryptic errors out of perf,
including:
problem incrementing symbol count, skipping event
This patch removes the reloc_start linker script label and instead
defines it as PHYSICAL_START. We also need to wrap it with
CONFIG_PPC64 because the ppc32 kernel can set a non zero
PHYSICAL_START at compile time and we wouldn't want to subtract
it from the CRCs in that case.
Signed-off-by: Anton Blanchard <[email protected]>
Cc: <[email protected]>
---
This bug was originally reported on Fedora 19 (3.9.x), so I've marked
it for stable.
Index: b/arch/powerpc/include/asm/module.h
===================================================================
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -82,10 +82,9 @@ struct exception_table_entry;
void sort_ex_table(struct exception_table_entry *start,
struct exception_table_entry *finish);
-#ifdef CONFIG_MODVERSIONS
+#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
#define ARCH_RELOCATES_KCRCTAB
-
-extern const unsigned long reloc_start[];
+#define reloc_start PHYSICAL_START
#endif
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_MODULE_H */
Index: b/arch/powerpc/kernel/vmlinux.lds.S
===================================================================
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -38,9 +38,6 @@ jiffies = jiffies_64 + 4;
#endif
SECTIONS
{
- . = 0;
- reloc_start = .;
-
. = KERNELBASE;
/*
Anton Blanchard <[email protected]> writes:
> Module CRCs are implemented as absolute symbols that get resolved by
> a linker script. We build an intermediate .o that contains an
> unresolved symbol for each CRC. genksysms parses this .o, calculates
> the CRCs and writes a linker script that "resolves" the symbols to
> the calculated CRC.
>
> Unfortunately the ppc64 relocatable kernel sees these CRCs as symbols
> that need relocating and relocates them at boot. Commit d4703aef
> (module: handle ppc64 relocating kcrctabs when CONFIG_RELOCATABLE=y)
> added a hook to reverse the bogus relocations. Part of this patch
> created a symbol at 0x0:
>
> # head -2 /proc/kallsyms
> 0000000000000000 T reloc_start
> c000000000000000 T .__start
>
> This reloc_start symbol is causing lots of confusion to perf. It
> thinks reloc_start is a massive function that stretches from 0x0 to
> 0xc000000000000000 and we get various cryptic errors out of perf,
> including:
>
> problem incrementing symbol count, skipping event
>
> This patch removes the reloc_start linker script label and instead
> defines it as PHYSICAL_START. We also need to wrap it with
> CONFIG_PPC64 because the ppc32 kernel can set a non zero
> PHYSICAL_START at compile time and we wouldn't want to subtract
> it from the CRCs in that case.
>
> Signed-off-by: Anton Blanchard <[email protected]>
> Cc: <[email protected]>
Acked-by: Rusty Russell <[email protected]>
Ben?
Cheers,
Rusty.
On Mon, 2013-07-15 at 14:04 +1000, Anton Blanchard wrote:
> Module CRCs are implemented as absolute symbols that get resolved by
> a linker script. We build an intermediate .o that contains an
> unresolved symbol for each CRC. genksysms parses this .o, calculates
> the CRCs and writes a linker script that "resolves" the symbols to
> the calc
Scott, can somebody from FSL test that on 32-bit and Ack it ?
Thanks !
Cheers,
Ben.
> Unfortunately the ppc64 relocatable kernel sees these CRCs as symbols
> that need relocating and relocates them at boot. Commit d4703aef
> (module: handle ppc64 relocating kcrctabs when CONFIG_RELOCATABLE=y)
> added a hook to reverse the bogus relocations. Part of this patch
> created a symbol at 0x0:
>
> # head -2 /proc/kallsyms
> 0000000000000000 T reloc_start
> c000000000000000 T .__start
>
> This reloc_start symbol is causing lots of confusion to perf. It
> thinks reloc_start is a massive function that stretches from 0x0 to
> 0xc000000000000000 and we get various cryptic errors out of perf,
> including:
>
> problem incrementing symbol count, skipping event
>
> This patch removes the reloc_start linker script label and instead
> defines it as PHYSICAL_START. We also need to wrap it with
> CONFIG_PPC64 because the ppc32 kernel can set a non zero
> PHYSICAL_START at compile time and we wouldn't want to subtract
> it from the CRCs in that case.
>
> Signed-off-by: Anton Blanchard <[email protected]>
> Cc: <[email protected]>
> ---
>
> This bug was originally reported on Fedora 19 (3.9.x), so I've marked
> it for stable.
>
> Index: b/arch/powerpc/include/asm/module.h
> ===================================================================
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -82,10 +82,9 @@ struct exception_table_entry;
> void sort_ex_table(struct exception_table_entry *start,
> struct exception_table_entry *finish);
>
> -#ifdef CONFIG_MODVERSIONS
> +#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> #define ARCH_RELOCATES_KCRCTAB
> -
> -extern const unsigned long reloc_start[];
> +#define reloc_start PHYSICAL_START
> #endif
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_MODULE_H */
> Index: b/arch/powerpc/kernel/vmlinux.lds.S
> ===================================================================
> --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -38,9 +38,6 @@ jiffies = jiffies_64 + 4;
> #endif
> SECTIONS
> {
> - . = 0;
> - reloc_start = .;
> -
> . = KERNELBASE;
>
> /*
On 07/15/2013 03:47:06 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-07-15 at 14:04 +1000, Anton Blanchard wrote:
> > Module CRCs are implemented as absolute symbols that get resolved by
> > a linker script. We build an intermediate .o that contains an
> > unresolved symbol for each CRC. genksysms parses this .o, calculates
> > the CRCs and writes a linker script that "resolves" the symbols to
> > the calc
>
> Scott, can somebody from FSL test that on 32-bit and Ack it ?
>
> Thanks !
>
> Cheers,
> Ben.
It boots for me on e500mc and I can insert modules.
-Scott
On Tue, 2013-07-16 at 17:40 -0500, Scott Wood wrote:
> On 07/15/2013 03:47:06 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2013-07-15 at 14:04 +1000, Anton Blanchard wrote:
> > > Module CRCs are implemented as absolute symbols that get resolved by
> > > a linker script. We build an intermediate .o that contains an
> > > unresolved symbol for each CRC. genksysms parses this .o, calculates
> > > the CRCs and writes a linker script that "resolves" the symbols to
> > > the calc
> >
> > Scott, can somebody from FSL test that on 32-bit and Ack it ?
> >
> > Thanks !
> >
> > Cheers,
> > Ben.
>
> It boots for me on e500mc and I can insert modules.
But does perf work ? :-)
Cheers,
Ben.
On 07/16/2013 07:04:05 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-07-16 at 17:40 -0500, Scott Wood wrote:
> > On 07/15/2013 03:47:06 AM, Benjamin Herrenschmidt wrote:
> > > On Mon, 2013-07-15 at 14:04 +1000, Anton Blanchard wrote:
> > > > Module CRCs are implemented as absolute symbols that get
> resolved by
> > > > a linker script. We build an intermediate .o that contains an
> > > > unresolved symbol for each CRC. genksysms parses this .o,
> calculates
> > > > the CRCs and writes a linker script that "resolves" the symbols
> to
> > > > the calc
> > >
> > > Scott, can somebody from FSL test that on 32-bit and Ack it ?
> > >
> > > Thanks !
> > >
> > > Cheers,
> > > Ben.
> >
> > It boots for me on e500mc and I can insert modules.
>
> But does perf work ? :-)
What specifically should I do to test it?
-Scott
Hi Scott,
> What specifically should I do to test it?
Could you double check perf annotate works? I'm 99% sure it will but
that is what was failing on ppc64.
Anton
On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:
>
> Hi Scott,
>
> > What specifically should I do to test it?
>
> Could you double check perf annotate works? I'm 99% sure it will but
> that is what was failing on ppc64.
I'm not really sure what it's supposed to look like when "perf
annotate" works. It spits a bunch of unreadable[1] dark-blue-on-black
assembly code at me, all with "0.00 :" in the left column.
Oh, wait -- some lines have "100.00 : " on the left, in
even-more-unreadable dark-red-on-black.
Apart from the annoying colors, is there anything specific I should be
looking for? Some sort of error message, or output that actually makes
sense?
I've attached the output from "perf annotate" and "perf report".
perf.data was generated by "perf record find /usr > /dev/null" on an
NFS root (which took a few seconds to complete), so the large amount of
__alloc_skb makes some sense, but the way perf annotate shows 100% on
one instruction in each function seems odd.
-Scott
[1] ...unless I crank the brightness up on my monitor to the point
where whites are blinding, or redirect the output to a file so the
colors go away.
On Fri, Jul 19, 2013 at 05:59:30PM -0500, Scott Wood wrote:
> On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:
> >
> >Hi Scott,
> >
> >> What specifically should I do to test it?
> >
> >Could you double check perf annotate works? I'm 99% sure it will but
> >that is what was failing on ppc64.
>
> I'm not really sure what it's supposed to look like when "perf
> annotate" works. It spits a bunch of unreadable[1]
> dark-blue-on-black assembly code at me, all with "0.00 :" in the
> left column.
>
> Oh, wait -- some lines have "100.00 : " on the left, in
> even-more-unreadable dark-red-on-black.
>
> Apart from the annoying colors, is there anything specific I should
> be looking for? Some sort of error message, or output that actually
> makes sense?
The colours look fine on my terminal, so I don't know what you've done
there. If you care you can use "--stdio" to use the plainer interface,
though it still uses colours.
That output looks fine in terms of the bug Anton was chasing. As far as
only ever hitting one instruction that does look weird.
cheers
On 07/23/2013 08:30:32 AM, Michael Ellerman wrote:
> On Fri, Jul 19, 2013 at 05:59:30PM -0500, Scott Wood wrote:
> > On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:
> > >
> > >Hi Scott,
> > >
> > >> What specifically should I do to test it?
> > >
> > >Could you double check perf annotate works? I'm 99% sure it will
> but
> > >that is what was failing on ppc64.
> >
> > I'm not really sure what it's supposed to look like when "perf
> > annotate" works. It spits a bunch of unreadable[1]
> > dark-blue-on-black assembly code at me, all with "0.00 :" in the
> > left column.
> >
> > Oh, wait -- some lines have "100.00 : " on the left, in
> > even-more-unreadable dark-red-on-black.
> >
> > Apart from the annoying colors, is there anything specific I should
> > be looking for? Some sort of error message, or output that actually
> > makes sense?
>
> The colours look fine on my terminal, so I don't know what you've done
> there.
It probably looks better if the terminal is configured to have a light
background (which of course makes some other programs look worse), or
(as I noted) if you've got your monitor set to be very bright. I now
see that xfce4-terminal lets me redefine the standard colors, though,
so that should help.
> If you care you can use "--stdio" to use the plainer interface,
> though it still uses colours.
>
> That output looks fine in terms of the bug Anton was chasing. As far
> as
> only ever hitting one instruction that does look weird.
OK. I'll add "investigate weird e500 perf annotate results" to the
TODO list...
-Scott
Hi Scott,
> I'm not really sure what it's supposed to look like when "perf
> annotate" works. It spits a bunch of unreadable[1]
> dark-blue-on-black assembly code at me, all with "0.00 :" in the left
> column.
>
> Oh, wait -- some lines have "100.00 : " on the left, in
> even-more-unreadable dark-red-on-black.
>
> Apart from the annoying colors, is there anything specific I should
> be looking for? Some sort of error message, or output that actually
> makes sense?
Thanks for testing! Ben, I think the patch is good to go.
Anton
On Thu, 2013-07-25 at 08:34 +1000, Anton Blanchard wrote:
> > Apart from the annoying colors, is there anything specific I should
> > be looking for? Some sort of error message, or output that actually
> > makes sense?
>
> Thanks for testing! Ben, I think the patch is good to go.
Sent it yesterday to Linus, it's upstream already :-)
Cheers,
Ben.
On Thu, Jul 25, 2013 at 09:14:25AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 08:34 +1000, Anton Blanchard wrote:
> > > Apart from the annoying colors, is there anything specific I should
> > > be looking for? Some sort of error message, or output that actually
> > > makes sense?
> >
> > Thanks for testing! Ben, I think the patch is good to go.
>
> Sent it yesterday to Linus, it's upstream already :-)
>
> Cheers,
> Ben.
>
Sorry I'm a bit late to the thread, I've ben swamped. Has someone tested this
with kexec/kdump? Thats why the origional patch was created, because when kexec
loads the kernel at a different physical address, the relocations messed with
the module crc's, and modules couldn't load during the kexec boot. Assuming
that kernaddr_start gets set appropriately during boot, using PHYSICAL_START
should be fine, but I wanted to check, and don't currently have access to a
powerpc system to do so.
Neil
>
>
>
Hi Neil,
> Sorry I'm a bit late to the thread, I've ben swamped. Has someone
> tested this with kexec/kdump? Thats why the origional patch was
> created, because when kexec loads the kernel at a different physical
> address, the relocations messed with the module crc's, and modules
> couldn't load during the kexec boot. Assuming that kernaddr_start
> gets set appropriately during boot, using PHYSICAL_START should be
> fine, but I wanted to check, and don't currently have access to a
> powerpc system to do so. Neil
I tested a relocatable kernel forced to run at a non zero physical
address (ie basically kdump). I verified CRCs were bad with your
original patch backed out, and were good with this patch applied.
Anton
On Fri, Jul 26, 2013 at 11:19:13AM +1000, Anton Blanchard wrote:
>
> Hi Neil,
>
> > Sorry I'm a bit late to the thread, I've ben swamped. Has someone
> > tested this with kexec/kdump? Thats why the origional patch was
> > created, because when kexec loads the kernel at a different physical
> > address, the relocations messed with the module crc's, and modules
> > couldn't load during the kexec boot. Assuming that kernaddr_start
> > gets set appropriately during boot, using PHYSICAL_START should be
> > fine, but I wanted to check, and don't currently have access to a
> > powerpc system to do so. Neil
>
> I tested a relocatable kernel forced to run at a non zero physical
> address (ie basically kdump). I verified CRCs were bad with your
> original patch backed out, and were good with this patch applied.
>
> Anton
>
Perfect, sounds like a sufficient test to me.
Acked-by: Neil Horman <[email protected]>