[edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking

Ard Biesheuvel posted 1 patch 4 years, 7 months ago
Failed in applying to current master (apply log)
ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
based relocations in spite of our attempts to avoid this, by using
hidden visibility, -Bsymbolic etc.

On AARCH64, we managed to work around this by processing the GOT
based relocations in GenFw. As it turns out, the same issue exists
on 32-bit ARM, but unfortunately, we cannot use a similar trick to
get rid of the GOT entry, and the relocation metadata is insufficient
to locate the GOT entry in the binary.

A bit of trial and error reveals that switching the linker options from
-pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
not the best approach, and instead, we can pass -pie to the linker
directly (using -Wl,xxx) rather than to the compiler directly, which
turns out to massage the linker in the right way, and prevents if from
emitting GOT based relocations. Your mileage may vary ...

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Let's test this on a couple of different Clang versions. If it still
produces problems, the only other way I see is to disable the builds
of platforms that incorporate this module for ARM/CLANG38 [which is
not the end of the world]

 ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
index 683397b7afd1..9e58e56fce09 100755
--- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
+++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
@@ -97,4 +97,4 @@
   gArmTokenSpaceGuid.PcdFvBaseAddress
 
 [BuildOptions]
-  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
+  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
-- 
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46835): https://edk2.groups.io/g/devel/message/46835
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Leif Lindholm 4 years, 7 months ago
On Wed, Sep 04, 2019 at 04:04:23PM -0700, Ard Biesheuvel wrote:
> In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
> based relocations in spite of our attempts to avoid this, by using
> hidden visibility, -Bsymbolic etc.
> 
> On AARCH64, we managed to work around this by processing the GOT
> based relocations in GenFw. As it turns out, the same issue exists
> on 32-bit ARM, but unfortunately, we cannot use a similar trick to
> get rid of the GOT entry, and the relocation metadata is insufficient
> to locate the GOT entry in the binary.
> 
> A bit of trial and error reveals that switching the linker options from
> -pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
> not the best approach, and instead, we can pass -pie to the linker
> directly (using -Wl,xxx) rather than to the compiler directly, which
> turns out to massage the linker in the right way, and prevents if from
> emitting GOT based relocations. Your mileage may vary ...
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Let's test this on a couple of different Clang versions. If it still
> produces problems, the only other way I see is to disable the builds
> of platforms that incorporate this module for ARM/CLANG38 [which is
> not the end of the world]
> 
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.in
> index 683397b7afd1..9e58e56fce09 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -97,4 +97,4 @@
>    gArmTokenSpaceGuid.PcdFvBaseAddress
>  
>  [BuildOptions]
> -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds

We already merged a fix for AARCH64 though - could/should this be
active on ARM only?

A problem I have with this patch is that ArmVirtQemuKernel curently
doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
with or without this patch):
ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
Mem64[0x8000000000+0x8000000000)@0x0
MapGcdMmioSpace: failed to set memory space attributes for region
[0x4010000000+0x10000000)

ASSERT_EFI_ERROR (Status = Unsupported)
ASSERT [PciHostBridgeDxe]
/work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
!EFI_ERROR (Status)
qemu-system-arm: terminating on signal 15 from pid 4680 (killall)

/
    Leif

> -- 
> 2.17.1
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46902): https://edk2.groups.io/g/devel/message/46902
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
On Thu, 5 Sep 2019 at 07:19, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Sep 04, 2019 at 04:04:23PM -0700, Ard Biesheuvel wrote:
> > In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
> > based relocations in spite of our attempts to avoid this, by using
> > hidden visibility, -Bsymbolic etc.
> >
> > On AARCH64, we managed to work around this by processing the GOT
> > based relocations in GenFw. As it turns out, the same issue exists
> > on 32-bit ARM, but unfortunately, we cannot use a similar trick to
> > get rid of the GOT entry, and the relocation metadata is insufficient
> > to locate the GOT entry in the binary.
> >
> > A bit of trial and error reveals that switching the linker options from
> > -pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
> > not the best approach, and instead, we can pass -pie to the linker
> > directly (using -Wl,xxx) rather than to the compiler directly, which
> > turns out to massage the linker in the right way, and prevents if from
> > emitting GOT based relocations. Your mileage may vary ...
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Let's test this on a couple of different Clang versions. If it still
> > produces problems, the only other way I see is to disable the builds
> > of platforms that incorporate this module for ARM/CLANG38 [which is
> > not the end of the world]
> >
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.in
> > index 683397b7afd1..9e58e56fce09 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -97,4 +97,4 @@
> >    gArmTokenSpaceGuid.PcdFvBaseAddress
> >
> >  [BuildOptions]
> > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
>
> We already merged a fix for AARCH64 though - could/should this be
> active on ARM only?
>

I'd like to keep the GenFw change, since we've confirmed that it
correctly handles GOT based relocations should they appear. But if
this fix works for all Clang versions, I'd rather have it active for
AArch64 as well, since the best solution is still not to have any such
relocations in the first place.

> A problem I have with this patch is that ArmVirtQemuKernel curently
> doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> with or without this patch):
> ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> Mem64[0x8000000000+0x8000000000)@0x0
> MapGcdMmioSpace: failed to set memory space attributes for region
> [0x4010000000+0x10000000)
>
> ASSERT_EFI_ERROR (Status = Unsupported)
> ASSERT [PciHostBridgeDxe]
> /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> !EFI_ERROR (Status)
> qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
>
> /
>     Leif
>
> > --
> > 2.17.1
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46904): https://edk2.groups.io/g/devel/message/46904
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
On Thu, 5 Sep 2019 at 07:19, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Sep 04, 2019 at 04:04:23PM -0700, Ard Biesheuvel wrote:
> > In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
> > based relocations in spite of our attempts to avoid this, by using
> > hidden visibility, -Bsymbolic etc.
> >
> > On AARCH64, we managed to work around this by processing the GOT
> > based relocations in GenFw. As it turns out, the same issue exists
> > on 32-bit ARM, but unfortunately, we cannot use a similar trick to
> > get rid of the GOT entry, and the relocation metadata is insufficient
> > to locate the GOT entry in the binary.
> >
> > A bit of trial and error reveals that switching the linker options from
> > -pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
> > not the best approach, and instead, we can pass -pie to the linker
> > directly (using -Wl,xxx) rather than to the compiler directly, which
> > turns out to massage the linker in the right way, and prevents if from
> > emitting GOT based relocations. Your mileage may vary ...
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Let's test this on a couple of different Clang versions. If it still
> > produces problems, the only other way I see is to disable the builds
> > of platforms that incorporate this module for ARM/CLANG38 [which is
> > not the end of the world]
> >
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.in
> > index 683397b7afd1..9e58e56fce09 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -97,4 +97,4 @@
> >    gArmTokenSpaceGuid.PcdFvBaseAddress
> >
> >  [BuildOptions]
> > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
>
> We already merged a fix for AARCH64 though - could/should this be
> active on ARM only?
>
> A problem I have with this patch is that ArmVirtQemuKernel curently
> doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> with or without this patch):
> ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> Mem64[0x8000000000+0x8000000000)@0x0
> MapGcdMmioSpace: failed to set memory space attributes for region
> [0x4010000000+0x10000000)
>
> ASSERT_EFI_ERROR (Status = Unsupported)
> ASSERT [PciHostBridgeDxe]
> /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> !EFI_ERROR (Status)
> qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
>

Does it work with -M virt,highmem=off ?

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46903): https://edk2.groups.io/g/devel/message/46903
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Leif Lindholm 4 years, 7 months ago
On Thu, Sep 05, 2019 at 07:25:39AM -0700, Ard Biesheuvel wrote:
> > >  [BuildOptions]
> > > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> >
> > We already merged a fix for AARCH64 though - could/should this be
> > active on ARM only?
> >
> > A problem I have with this patch is that ArmVirtQemuKernel curently
> > doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> > with or without this patch):
> > ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> > Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> > Mem64[0x8000000000+0x8000000000)@0x0
> > MapGcdMmioSpace: failed to set memory space attributes for region
> > [0x4010000000+0x10000000)
> >
> > ASSERT_EFI_ERROR (Status = Unsupported)
> > ASSERT [PciHostBridgeDxe]
> > /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> > !EFI_ERROR (Status)
> > qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
> >
> 
> Does it work with -M virt,highmem=off ?

Ah, yes - that works fine then, also with the patch.

Well, with that, and your explanation in the other thread:
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46912): https://edk2.groups.io/g/devel/message/46912
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
On Thu, 5 Sep 2019 at 08:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Sep 05, 2019 at 07:25:39AM -0700, Ard Biesheuvel wrote:
> > > >  [BuildOptions]
> > > > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > >
> > > We already merged a fix for AARCH64 though - could/should this be
> > > active on ARM only?
> > >
> > > A problem I have with this patch is that ArmVirtQemuKernel curently
> > > doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> > > with or without this patch):
> > > ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> > > Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> > > Mem64[0x8000000000+0x8000000000)@0x0
> > > MapGcdMmioSpace: failed to set memory space attributes for region
> > > [0x4010000000+0x10000000)
> > >
> > > ASSERT_EFI_ERROR (Status = Unsupported)
> > > ASSERT [PciHostBridgeDxe]
> > > /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> > > !EFI_ERROR (Status)
> > > qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
> > >
> >
> > Does it work with -M virt,highmem=off ?
>
> Ah, yes - that works fine then, also with the patch.
>
> Well, with that, and your explanation in the other thread:
> Acked-by: Leif Lindholm <leif.lindholm@linaro.org>

Thanks. I am going to replace the last paragraph of the commit log with

    Note that in this particular case, we are interested in PIE linking
    only (i.e., producing a .rela section containing dynamic relocations
    that the startup code can process directly), and not in position
    independent code generation, and by passing the -pie option to the
    linker directly using -Wl,-pie (and dropping -shared), we can coerce
    the GOLD linker into doing only the former rather than both when it
    performs its LTO code generation.

and push (unless you have any objections)

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46924): https://edk2.groups.io/g/devel/message/46924
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Leif Lindholm 4 years, 7 months ago
On Thu, Sep 05, 2019 at 10:06:56AM -0700, Ard Biesheuvel wrote:
> On Thu, 5 Sep 2019 at 08:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Thu, Sep 05, 2019 at 07:25:39AM -0700, Ard Biesheuvel wrote:
> > > > >  [BuildOptions]
> > > > > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > > > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > >
> > > > We already merged a fix for AARCH64 though - could/should this be
> > > > active on ARM only?
> > > >
> > > > A problem I have with this patch is that ArmVirtQemuKernel curently
> > > > doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> > > > with or without this patch):
> > > > ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> > > > Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> > > > Mem64[0x8000000000+0x8000000000)@0x0
> > > > MapGcdMmioSpace: failed to set memory space attributes for region
> > > > [0x4010000000+0x10000000)
> > > >
> > > > ASSERT_EFI_ERROR (Status = Unsupported)
> > > > ASSERT [PciHostBridgeDxe]
> > > > /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> > > > !EFI_ERROR (Status)
> > > > qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
> > > >
> > >
> > > Does it work with -M virt,highmem=off ?
> >
> > Ah, yes - that works fine then, also with the patch.
> >
> > Well, with that, and your explanation in the other thread:
> > Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Thanks. I am going to replace the last paragraph of the commit log with
> 
>     Note that in this particular case, we are interested in PIE linking
>     only (i.e., producing a .rela section containing dynamic relocations
>     that the startup code can process directly), and not in position
>     independent code generation, and by passing the -pie option to the
>     linker directly using -Wl,-pie (and dropping -shared), we can coerce
>     the GOLD linker into doing only the former rather than both when it
>     performs its LTO code generation.
> 
> and push (unless you have any objections)

No objections.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46926): https://edk2.groups.io/g/devel/message/46926
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
On Thu, 5 Sep 2019 at 10:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Sep 05, 2019 at 10:06:56AM -0700, Ard Biesheuvel wrote:
> > On Thu, 5 Sep 2019 at 08:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Thu, Sep 05, 2019 at 07:25:39AM -0700, Ard Biesheuvel wrote:
> > > > > >  [BuildOptions]
> > > > > > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > > > > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > > > >
> > > > > We already merged a fix for AARCH64 though - could/should this be
> > > > > active on ARM only?
> > > > >
> > > > > A problem I have with this patch is that ArmVirtQemuKernel curently
> > > > > doesn't boot on my qemu (with/without kvm, built with GCC5 or CLANG38,
> > > > > with or without this patch):
> > > > > ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF]
> > > > > Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0
> > > > > Mem64[0x8000000000+0x8000000000)@0x0
> > > > > MapGcdMmioSpace: failed to set memory space attributes for region
> > > > > [0x4010000000+0x10000000)
> > > > >
> > > > > ASSERT_EFI_ERROR (Status = Unsupported)
> > > > > ASSERT [PciHostBridgeDxe]
> > > > > /work/git/edk2/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c(293):
> > > > > !EFI_ERROR (Status)
> > > > > qemu-system-arm: terminating on signal 15 from pid 4680 (killall)
> > > > >
> > > >
> > > > Does it work with -M virt,highmem=off ?
> > >
> > > Ah, yes - that works fine then, also with the patch.
> > >
> > > Well, with that, and your explanation in the other thread:
> > > Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> > Thanks. I am going to replace the last paragraph of the commit log with
> >
> >     Note that in this particular case, we are interested in PIE linking
> >     only (i.e., producing a .rela section containing dynamic relocations
> >     that the startup code can process directly), and not in position
> >     independent code generation, and by passing the -pie option to the
> >     linker directly using -Wl,-pie (and dropping -shared), we can coerce
> >     the GOLD linker into doing only the former rather than both when it
> >     performs its LTO code generation.
> >
> > and push (unless you have any objections)
>
> No objections.
>

Thanks

Pushed as 8a1305a11f3b..04d9d89b7dd4

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46927): https://edk2.groups.io/g/devel/message/46927
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Laszlo Ersek 4 years, 7 months ago
On 09/05/19 01:04, Ard Biesheuvel wrote:
> In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
> based relocations in spite of our attempts to avoid this, by using
> hidden visibility, -Bsymbolic etc.
> 
> On AARCH64, we managed to work around this by processing the GOT
> based relocations in GenFw. As it turns out, the same issue exists
> on 32-bit ARM, but unfortunately, we cannot use a similar trick to
> get rid of the GOT entry, and the relocation metadata is insufficient
> to locate the GOT entry in the binary.
> 
> A bit of trial and error reveals that switching the linker options from
> -pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
> not the best approach, and instead, we can pass -pie to the linker
> directly (using -Wl,xxx) rather than to the compiler directly, which
> turns out to massage the linker in the right way, and prevents if from
> emitting GOT based relocations. Your mileage may vary ...
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Let's test this on a couple of different Clang versions. If it still
> produces problems, the only other way I see is to disable the builds
> of platforms that incorporate this module for ARM/CLANG38 [which is
> not the end of the world]
> 
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index 683397b7afd1..9e58e56fce09 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -97,4 +97,4 @@
>    gArmTokenSpaceGuid.PcdFvBaseAddress
>  
>  [BuildOptions]
> -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> 

Sorry for being late to this patch, I can see [*] it's upstream alrady.
Nonetheless:

Acked-by: Laszlo Ersek <lersek@redhat.com>

[*] Not from my inbox though -- I've got again too much on my plate and
am fetching email in batches. But, between my rebasing of
"ArmVirtPkg/PlatformBootManagerLib: unload image on
EFI_SECURITY_VIOLATION", building it, and attempting to push it, you had
pushed this one -- and I did notice that. :)

Thanks,
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46938): https://edk2.groups.io/g/devel/message/46938
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: revert to PIE linking
Posted by Ard Biesheuvel 4 years, 7 months ago
On Thu, 5 Sep 2019 at 12:33, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 09/05/19 01:04, Ard Biesheuvel wrote:
> > In some cases, the CLANG38 toolchain profile in LTO mode emits GOT
> > based relocations in spite of our attempts to avoid this, by using
> > hidden visibility, -Bsymbolic etc.
> >
> > On AARCH64, we managed to work around this by processing the GOT
> > based relocations in GenFw. As it turns out, the same issue exists
> > on 32-bit ARM, but unfortunately, we cannot use a similar trick to
> > get rid of the GOT entry, and the relocation metadata is insufficient
> > to locate the GOT entry in the binary.
> >
> > A bit of trial and error reveals that switching the linker options from
> > -pie to -shared in commit e07092edca8442db4a941dbeea0cd196c7bf8ec9 was
> > not the best approach, and instead, we can pass -pie to the linker
> > directly (using -Wl,xxx) rather than to the compiler directly, which
> > turns out to massage the linker in the right way, and prevents if from
> > emitting GOT based relocations. Your mileage may vary ...
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Let's test this on a couple of different Clang versions. If it still
> > produces problems, the only other way I see is to disable the builds
> > of platforms that incorporate this module for ARM/CLANG38 [which is
> > not the end of the world]
> >
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > index 683397b7afd1..9e58e56fce09 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -97,4 +97,4 @@
> >    gArmTokenSpaceGuid.PcdFvBaseAddress
> >
> >  [BuildOptions]
> > -  GCC:*_*_*_DLINK_FLAGS = -shared -Wl,-Bsymbolic -Wl,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> > +  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> >
>
> Sorry for being late to this patch, I can see [*] it's upstream alrady.
> Nonetheless:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks. Since the patch is already merged, I'll find another patch to
attach it to :-)

> [*] Not from my inbox though -- I've got again too much on my plate and
> am fetching email in batches. But, between my rebasing of
> "ArmVirtPkg/PlatformBootManagerLib: unload image on
> EFI_SECURITY_VIOLATION", building it, and attempting to push it, you had
> pushed this one -- and I did notice that. :)
>
> Thanks,
> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46997): https://edk2.groups.io/g/devel/message/46997
Mute This Topic: https://groups.io/mt/33144570/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-