| Message ID | CAKv+Gu8OD3p+GSwB+SPRf3pKwZB2R1t6x1zK7czGCXj1QhB7_g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hey, Op 03-09-14 om 14:18 schreef Ard Biesheuvel: > Could you please try adding the visibility attribute lik this instead? > > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h > index 044a2fd3c5fe..8725d85f1903 100644 > --- a/arch/x86/include/asm/efi.h > +++ b/arch/x86/include/asm/efi.h > @@ -178,7 +178,7 @@ struct efi_config { > bool is64; > } __packed; > > -extern struct efi_config *efi_early; > +extern __attribute__((visibility("hidden"))) struct efi_config *efi_early; > > #define efi_call_early(f, ...) \ > efi_early->call(efi_early->f, __VA_ARGS__); > > Before this change, I get 18 R_X86_64_GOTPCREL relocations pointing to > efi_early, both in efi-stub-helper.c and eboot.c. > After the change, I get 0, using 'readelf -a > drivers/firmware/efi/libstub/efi-stub-helper.o > arch/x86/boot/compressed/eboot.o|grep GOTPCREL|wc -l' > Yeah that fixes things! Feel free to slap on a reported-reviewed-and-tested-by on your patch. :-) ~Maarten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 3 September 2014 17:30, Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > Hey, > > Op 03-09-14 om 14:18 schreef Ard Biesheuvel: >> Could you please try adding the visibility attribute lik this instead? >> >> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h >> index 044a2fd3c5fe..8725d85f1903 100644 >> --- a/arch/x86/include/asm/efi.h >> +++ b/arch/x86/include/asm/efi.h >> @@ -178,7 +178,7 @@ struct efi_config { >> bool is64; >> } __packed; >> >> -extern struct efi_config *efi_early; >> +extern __attribute__((visibility("hidden"))) struct efi_config *efi_early; >> >> #define efi_call_early(f, ...) \ >> efi_early->call(efi_early->f, __VA_ARGS__); >> >> Before this change, I get 18 R_X86_64_GOTPCREL relocations pointing to >> efi_early, both in efi-stub-helper.c and eboot.c. >> After the change, I get 0, using 'readelf -a >> drivers/firmware/efi/libstub/efi-stub-helper.o >> arch/x86/boot/compressed/eboot.o|grep GOTPCREL|wc -l' >> > Yeah that fixes things! > > Feel free to slap on a reported-reviewed-and-tested-by on your patch. :-) > Will do, thanks. @Matt: so there is two ways to fix this, the patch above addressing this single instance, and alternatively, adding a #pragma GCC visiblilty push(hidden) to all .c files under libstub/, *before* the #includes. The latter would catch future problems regarding newly introduced global variables, but it may be a bit overkill in this case, as libstub is not expected to be in flux in the foreseeable future. Any preferences?
> > Any reason we can't reuse the existing GOT fixup code in the early x86 > boot code? We're not executing it before the EFI boot stub atm, which is > the reason Maarten is hitting these difficulties. > > Maarten, does the following help? > > If not, Ard please go ahead with option #2 above. Overkill yes, but I've > done the single __attribute__() hacks in other projects and someone > (usually me) always eventually forgets to tag some instance. > I think we really have two options: either fix up the GOT (which may be a null operation, if the GOT is empty) or we add a compile-time check that the GOT is empty, lest we will keep having these problems. Since the GOT fixup loop is only a few instructions, it doesn't seem to be all that problematic to just do it -- but make sure we don't end up running it twice on any code path! -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 3 September 2014 23:47, H. Peter Anvin <hpa@zytor.com> wrote: >> >> Any reason we can't reuse the existing GOT fixup code in the early x86 >> boot code? We're not executing it before the EFI boot stub atm, which is >> the reason Maarten is hitting these difficulties. >> >> Maarten, does the following help? >> >> If not, Ard please go ahead with option #2 above. Overkill yes, but I've >> done the single __attribute__() hacks in other projects and someone >> (usually me) always eventually forgets to tag some instance. >> > > I think we really have two options: either fix up the GOT (which may be > a null operation, if the GOT is empty) or we add a compile-time check > that the GOT is empty, lest we will keep having these problems. > > Since the GOT fixup loop is only a few instructions, it doesn't seem to > be all that problematic to just do it -- but make sure we don't end up > running it twice on any code path! > So how about we: - add ASSERT(_got == _egot, "GOT entries not supported in boot/compressed") to the linker script - #define __nogotentry __attribute__((visibility(hidden))) somewhere in compiler.h or wherever else it belongs - add the __nogotentry qualifiers to each extern that requires it, which is currently: early_serial_base efi_early free_mem_end_ptr free_mem_ptr real_mode - get rid of the fixup code in assembly Any idea what the oldest GCC is we should support?
On Wed, 03 Sep, at 02:47:32PM, H. Peter Anvin wrote: > > I think we really have two options: either fix up the GOT (which may be > a null operation, if the GOT is empty) or we add a compile-time check > that the GOT is empty, lest we will keep having these problems. > > Since the GOT fixup loop is only a few instructions, it doesn't seem to > be all that problematic to just do it -- but make sure we don't end up > running it twice on any code path! Urgh, I totally did make it run twice. My bad.
On Thu, 04 Sep, at 08:47:57AM, Ard Biesheuvel wrote: > > So how about we: > - add ASSERT(_got == _egot, "GOT entries not supported in > boot/compressed") to the linker script > - #define __nogotentry __attribute__((visibility(hidden))) somewhere > in compiler.h or wherever else it belongs > - add the __nogotentry qualifiers to each extern that requires it, > which is currently: > early_serial_base > efi_early > free_mem_end_ptr > free_mem_ptr > real_mode > - get rid of the fixup code in assembly Seems fine to me since it was Peter who introduced the GOT fixup asm code in the first place. > Any idea what the oldest GCC is we should support? I'm pretty sure we still support GCC 3.x.x. The symbol visibility support was added in GCC 4.x right?
Op 04-09-14 om 09:40 schreef Matt Fleming: > On Thu, 04 Sep, at 08:47:57AM, Ard Biesheuvel wrote: >> So how about we: >> - add ASSERT(_got == _egot, "GOT entries not supported in >> boot/compressed") to the linker script >> - #define __nogotentry __attribute__((visibility(hidden))) somewhere >> in compiler.h or wherever else it belongs >> - add the __nogotentry qualifiers to each extern that requires it, >> which is currently: >> early_serial_base >> efi_early >> free_mem_end_ptr >> free_mem_ptr >> real_mode >> - get rid of the fixup code in assembly > > Seems fine to me since it was Peter who introduced the GOT fixup asm > code in the first place. > >> Any idea what the oldest GCC is we should support? > I'm pretty sure we still support GCC 3.x.x. The symbol visibility > support was added in GCC 4.x right? > gcc 3.3.6 has documented visibility at least. https://gcc.gnu.org/onlinedocs/gcc-3.3.6/gcc/Function-Attributes.html#Function-Attributes I couldn't find it in earlier manuals, but grepping through gcc 3.0.4 source code shows that it would appear to be supported there too. ~Maarten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 044a2fd3c5fe..8725d85f1903 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -178,7 +178,7 @@ struct efi_config { bool is64; } __packed; -extern struct efi_config *efi_early; +extern __attribute__((visibility("hidden"))) struct efi_config *efi_early; #define efi_call_early(f, ...) \ efi_early->call(efi_early->f, __VA_ARGS__);