From patchwork Wed Sep 3 12:18:06 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 36604 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-oi0-f72.google.com (mail-oi0-f72.google.com [209.85.218.72]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 4666B2039B for ; Wed, 3 Sep 2014 12:18:35 +0000 (UTC) Received: by mail-oi0-f72.google.com with SMTP id e131sf39686214oig.11 for ; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe:content-type; bh=oN3PTqtORpMXcWUS18rRVll/eodGA3PRyOnQIs1Vj7w=; b=Nuweiz1WgJTRIrgok4cect66RsOWc+QvNvb1sRMybBTsqKxhDJ3sAsPo4eU4uYTaFJ aDL21B4KwT4+lzx+SyYj7y2sE2pKWaBT8I7XyWbU8PuB8VsZC1ANJNpkS+JpgYIJiGaj cHwoHMXKFC5cMTWsiHHcXVp0bh9pJvSRkIhV9ToItrFKYrDNKpxPCz7kYZ1mSBmbKyVI HMTewbUBbI9xhoQE0ea2/WED6rUysSZDbljWPXIDcnzfUYMPvePRIBdkdomfKo5No9fj IVhucrqusKgRinH//CSYzo++Emz2ekQYrZlgPyHfyB6vhuPCMmtis3HlN2R/IUMgGpOz pPpQ== X-Gm-Message-State: ALoCoQlu1F9qc5rvZkVSCN7Ahp6GY6nR+vwe8To7R0+Jr8a4cehpnWv6ZiPQptzhy9usNiLUvqvH X-Received: by 10.182.230.133 with SMTP id sy5mr23648870obc.22.1409746714867; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.41.211 with SMTP id z77ls2897747qgz.54.gmail; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) X-Received: by 10.52.127.5 with SMTP id nc5mr170003vdb.59.1409746714723; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) Received: from mail-vc0-f179.google.com (mail-vc0-f179.google.com [209.85.220.179]) by mx.google.com with ESMTPS id z11si3536248vcu.42.2014.09.03.05.18.34 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 03 Sep 2014 05:18:34 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.179 as permitted sender) client-ip=209.85.220.179; Received: by mail-vc0-f179.google.com with SMTP id hy4so8627297vcb.38 for ; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) X-Received: by 10.52.145.204 with SMTP id sw12mr44539vdb.79.1409746714591; Wed, 03 Sep 2014 05:18:34 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.45.67 with SMTP id uj3csp671367vcb; Wed, 3 Sep 2014 05:18:33 -0700 (PDT) X-Received: by 10.70.135.137 with SMTP id ps9mr11788072pdb.13.1409746713475; Wed, 03 Sep 2014 05:18:33 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id rg11si10837186pdb.142.2014.09.03.05.18.27 for ; Wed, 03 Sep 2014 05:18:28 -0700 (PDT) Received-SPF: none (google.com: linux-kernel-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756407AbaICMSL (ORCPT + 25 others); Wed, 3 Sep 2014 08:18:11 -0400 Received: from mail-la0-f45.google.com ([209.85.215.45]:41153 "EHLO mail-la0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756128AbaICMSJ (ORCPT ); Wed, 3 Sep 2014 08:18:09 -0400 Received: by mail-la0-f45.google.com with SMTP id pn19so9635806lab.4 for ; Wed, 03 Sep 2014 05:18:07 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.112.148.133 with SMTP id ts5mr39307332lbb.45.1409746686998; Wed, 03 Sep 2014 05:18:06 -0700 (PDT) Received: by 10.112.247.70 with HTTP; Wed, 3 Sep 2014 05:18:06 -0700 (PDT) In-Reply-To: <5406D0DB.90401@canonical.com> References: <5405E186.2080406@canonical.com> <20140902192941.GB3001@console-pimps.org> <5406D0DB.90401@canonical.com> Date: Wed, 3 Sep 2014 14:18:06 +0200 Message-ID: Subject: Re: [REGRESSION] "efi: efistub: Convert into static library" and preparation patches From: Ard Biesheuvel To: Maarten Lankhorst Cc: Matt Fleming , Ulf Winkelvos , Matt Fleming , LKML , "x86@kernel.org" , "H. Peter Anvin" , "linux-efi@vger.kernel.org" , Seth Forshee , Matthew Garrett Sender: linux-kernel-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ard.biesheuvel@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.179 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On 3 September 2014 10:27, Maarten Lankhorst wrote: > Op 03-09-14 om 08:06 schreef Ard Biesheuvel: >> On 2 September 2014 21:29, Matt Fleming wrote: >>> On Tue, 02 Sep, at 05:25:58PM, Maarten Lankhorst wrote: >>>> Hey, >>>> >>>> My macbook pro 8.2 fails to do a efi stub boot with these patches. >>>> >>>> Commit f23cf8bd5c1f49 "efi/x86: efistub: Move shared dependencies to " >>>> causes the first break, but this can be averted by changing >>>> >>>> struct efi_config *efi_early; >>>> >>>> to >>>> >>>> struct efi_config *efi_early __attribute__((visibility("hidden"))); >>> Weird. That sounds like a bug in the Apple EFI PE loader. Does any other >>> visibility result in a working kernel? >>> >>>> I also need to revert commit f4f75ad5741fe "efi: efistub: Convert into static library" >>>> to get boot working. >>> I'll take a look at the symbol changes between these commits and try and >>> guess what's going on. >>> >>>> I'm not an early boot expert, so I have no idea what's going on here. >>>> Only console output I see when the boot fails is "setup_efi_pci() failed!" after >>>> the commit that adds this message. >>> Yeah, that should be unrelated. >>> >>> Thanks for the report. >>> >> If x86 is anything like ARM in this respect, this is likely caused by >> the way PIC references to globals are emitted. >> When using default visibility, a reference to a global is emitted by a >> reference to the GOT, and an offset into the GOT, and the two are >> added and dereferenced at runtime. For this to work, the GOT needs to >> contain the correct absolute address for the global in question. This, >> in turn, relies on absolute relocations to be resolved at load time, >> which we don't do with the EFI stub. Could it be that the link address >> and load address are usually the same on x86 EFI but not on the Mac? >> >> With hidden visibility, the PIC reference is emitted using a relative >> relocation, to which the value of the program counter is added at >> runtime, and no GOT is involved, and all relocations can be resolved >> at build time. >> >> I think it makes sense to add a #pragma GCC visibility push(hidden) to >> efistub.h (if pragmas are in fashion these days), making sure that no >> global references (not even externs) will generate GOT entries. >> > Something like that probably. > > Following patch FAILS: > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index f277184e2ac1..f9738d92c9ce 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -15,6 +15,8 @@ > > #undef memcpy /* Use memcpy from misc.c */ > > +#pragma GCC visibility push(hidden) > + > #include "eboot.h" > > static efi_system_table_t *sys_table; > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index 32d5cca30f49..7ef10f36cc1b 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -13,6 +13,8 @@ > #include > #include > > +#pragma GCC visibility push(hidden) > + > #include "efistub.h" > > #define EFI_READ_CHUNK_SIZE (1024 * 1024) > > ----- > But this works: > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 7a801a310e37..eebd13ee301b 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -30,11 +30,10 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \ > $(obj)/string.o $(obj)/cmdline.o $(obj)/early_serial_console.o \ > $(obj)/piggy.o $(obj)/cpuflags.o $(obj)/aslr.o > > -$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone > +$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone -fvisibility=hidden > > ifeq ($(CONFIG_EFI_STUB), y) > - VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \ > - $(objtree)/drivers/firmware/efi/libstub/lib.a > + VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o > endif > > $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index f277184e2ac1..e18a265460dd 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -17,6 +17,8 @@ > > #include "eboot.h" > > +#include "../../../drivers/firmware/efi/libstub/efi-stub-helper.c" > + > static efi_system_table_t *sys_table; > > struct efi_config *efi_early; > > > In case it was caused by lacking -fshort-wchar or order of building, I tried the following, also fails: > --- > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 7a801a310e37..8f62c273badd 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -30,11 +30,10 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \ > $(obj)/string.o $(obj)/cmdline.o $(obj)/early_serial_console.o \ > $(obj)/piggy.o $(obj)/cpuflags.o $(obj)/aslr.o > > -$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone > +$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone -fvisibility=hidden > > ifeq ($(CONFIG_EFI_STUB), y) > - VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \ > - $(objtree)/drivers/firmware/efi/libstub/lib.a > + VMLINUX_OBJS += $(objtree)/drivers/firmware/efi/libstub/efi-stub-helper.o $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o > endif > > $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile > index b14bc2b9fb4d..66c5536cff4d 100644 > --- a/drivers/firmware/efi/libstub/Makefile > +++ b/drivers/firmware/efi/libstub/Makefile > @@ -8,7 +8,8 @@ cflags-$(CONFIG_X86_32) := -march=i386 > cflags-$(CONFIG_X86_64) := -mcmodel=small > cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ $(LINUX_INCLUDE) -O2 \ > -fPIC -fno-strict-aliasing -mno-red-zone \ > - -mno-mmx -mno-sse -DDISABLE_BRANCH_PROFILING > + -mno-mmx -mno-sse -DDISABLE_BRANCH_PROFILING \ > + -fvisibility=hidden -fshort-wchar > > cflags-$(CONFIG_ARM64) := $(subst -pg,,$(KBUILD_CFLAGS)) > cflags-$(CONFIG_ARM) := $(subst -pg,,$(KBUILD_CFLAGS)) \ > > > > --- > I have no idea why only directly referencing the file in eboot.c works. > Could you please try adding the visibility attribute lik this instead? 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' 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__);