[PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Ard Biesheuvel]

In order to permit the use of compilers that only implement the small
code model [which involves the use of ADRP instructions that require
4 KB segment alignment] for generating PE/COFF binaries with a small
footprint, we patch ADRP instructions into ADR instructions while doing
the ELF to PE/COFF conversion.

As it turns out, the linker may be doing the same, but for different
reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
affects ADRP instructions appearing at a certain offset in memory, and
one of the mitigations for this erratum is to patch them into ADR
instructions at link time if the symbol reference is within -/+ 1 MB.
However, the LD linker fails to update the static relocation tables, and
so we end up with an ADR instruction in the fully linked binary, but
with a relocation entry in the RELA section identifying it as an ADRP
instruction.

Since the linker has already updated the symbol reference, there is no
handling needed in GenFw for such instructions, and we can simply treat
it as an ordinary ADR. However, since it is guaranteed to be accompanied
by an add or load instruction with a LO12 relocation referencing the same
symbol, the section offset check we apply to ADR instructions is going to
take place anyway, so we can just disregard the ADR instruction entirely.

Reported-by: Eugene Cohen <eugene@hp.com>
Suggested-by: Eugene Cohen <eugene@hp.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index d574300ac4fe..d623dce1f9da 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -1044,6 +1044,19 @@ WriteSections64 (
             /* fall through */
 
           case R_AARCH64_ADR_PREL_PG_HI21:
+            //
+            // In order to handle Cortex-A53 erratum #843419, the LD linker may
+            // convert ADRP instructions into ADR instructions, but without
+            // updating the static relocation type, and so we may end up here
+            // while the instruction in question is actually ADR. So let's
+            // just disregard it: the section offset check we apply below to
+            // ADR instructions will trigger for its R_AARCH64_xxx_ABS_LO12_NC
+            // companion instruction as well, so it is safe to omit it here.
+            //
+            if ((*(UINT32 *)Targ & BIT31) == 0) {
+              break;
+            }
+
             //
             // AArch64 PG_H21 relocations are typically paired with ABS_LO12
             // relocations, where a PC-relative reference with +/- 4 GB range is
-- 
2.17.1

Re: [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Ard Biesheuvel]

(+ Leif, Liming)

On Thu, 7 Nov 2019 at 10:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> In order to permit the use of compilers that only implement the small
> code model [which involves the use of ADRP instructions that require
> 4 KB segment alignment] for generating PE/COFF binaries with a small
> footprint, we patch ADRP instructions into ADR instructions while doing
> the ELF to PE/COFF conversion.
>
> As it turns out, the linker may be doing the same, but for different
> reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
> affects ADRP instructions appearing at a certain offset in memory, and
> one of the mitigations for this erratum is to patch them into ADR
> instructions at link time if the symbol reference is within -/+ 1 MB.
> However, the LD linker fails to update the static relocation tables, and
> so we end up with an ADR instruction in the fully linked binary, but
> with a relocation entry in the RELA section identifying it as an ADRP
> instruction.
>
> Since the linker has already updated the symbol reference, there is no
> handling needed in GenFw for such instructions, and we can simply treat
> it as an ordinary ADR. However, since it is guaranteed to be accompanied
> by an add or load instruction with a LO12 relocation referencing the same
> symbol, the section offset check we apply to ADR instructions is going to
> take place anyway, so we can just disregard the ADR instruction entirely.
>
> Reported-by: Eugene Cohen <eugene@hp.com>
> Suggested-by: Eugene Cohen <eugene@hp.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index d574300ac4fe..d623dce1f9da 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -1044,6 +1044,19 @@ WriteSections64 (
>              /* fall through */
>
>            case R_AARCH64_ADR_PREL_PG_HI21:
> +            //
> +            // In order to handle Cortex-A53 erratum #843419, the LD linker may
> +            // convert ADRP instructions into ADR instructions, but without
> +            // updating the static relocation type, and so we may end up here
> +            // while the instruction in question is actually ADR. So let's
> +            // just disregard it: the section offset check we apply below to
> +            // ADR instructions will trigger for its R_AARCH64_xxx_ABS_LO12_NC
> +            // companion instruction as well, so it is safe to omit it here.
> +            //
> +            if ((*(UINT32 *)Targ & BIT31) == 0) {
> +              break;
> +            }
> +
>              //
>              // AArch64 PG_H21 relocations are typically paired with ABS_LO12
>              // relocations, where a PC-relative reference with +/- 4 GB range is
> --
> 2.17.1
>

Re: [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Cohen, Eugene]

Ard,

Fantastic discovery tracking this down to the toolchain.

Tested-by: Eugene Cohen <eugene@hp.com>

Thanks,

Eugene

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, November 07, 2019 2:09 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> Cc: Cohen, Eugene <eugene@hp.com>
> Subject: Re: [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP
> instructions that are patched already
> 
> (+ Leif, Liming)
> 
> On Thu, 7 Nov 2019 at 10:06, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> >
> > In order to permit the use of compilers that only implement the small
> > code model [which involves the use of ADRP instructions that require
> > 4 KB segment alignment] for generating PE/COFF binaries with a small
> > footprint, we patch ADRP instructions into ADR instructions while
> > doing the ELF to PE/COFF conversion.
> >
> > As it turns out, the linker may be doing the same, but for different
> > reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
> > affects ADRP instructions appearing at a certain offset in memory, and
> > one of the mitigations for this erratum is to patch them into ADR
> > instructions at link time if the symbol reference is within -/+ 1 MB.
> > However, the LD linker fails to update the static relocation tables,
> > and so we end up with an ADR instruction in the fully linked binary,
> > but with a relocation entry in the RELA section identifying it as an
> > ADRP instruction.
> >
> > Since the linker has already updated the symbol reference, there is no
> > handling needed in GenFw for such instructions, and we can simply
> > treat it as an ordinary ADR. However, since it is guaranteed to be
> > accompanied by an add or load instruction with a LO12 relocation
> > referencing the same symbol, the section offset check we apply to ADR
> > instructions is going to take place anyway, so we can just disregard the ADR
> instruction entirely.
> >
> > Reported-by: Eugene Cohen <eugene@hp.com>
> > Suggested-by: Eugene Cohen <eugene@hp.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > index d574300ac4fe..d623dce1f9da 100644
> > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > @@ -1044,6 +1044,19 @@ WriteSections64 (
> >              /* fall through */
> >
> >            case R_AARCH64_ADR_PREL_PG_HI21:
> > +            //
> > +            // In order to handle Cortex-A53 erratum #843419, the LD linker may
> > +            // convert ADRP instructions into ADR instructions, but without
> > +            // updating the static relocation type, and so we may end up here
> > +            // while the instruction in question is actually ADR. So let's
> > +            // just disregard it: the section offset check we apply below to
> > +            // ADR instructions will trigger for its
> R_AARCH64_xxx_ABS_LO12_NC
> > +            // companion instruction as well, so it is safe to omit it here.
> > +            //
> > +            if ((*(UINT32 *)Targ & BIT31) == 0) {
> > +              break;
> > +            }
> > +
> >              //
> >              // AArch64 PG_H21 relocations are typically paired with ABS_LO12
> >              // relocations, where a PC-relative reference with +/- 4
> > GB range is
> > --
> > 2.17.1
> >

Re: [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Leif Lindholm]

I *think* I understand what's going on (thank you for detailed commit
message and comment block). And that looks like a good workaround.

Thanks!
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


On Thu, Nov 07, 2019 at 10:09:27AM +0100, Ard Biesheuvel wrote:
> (+ Leif, Liming)
> 
> On Thu, 7 Nov 2019 at 10:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > In order to permit the use of compilers that only implement the small
> > code model [which involves the use of ADRP instructions that require
> > 4 KB segment alignment] for generating PE/COFF binaries with a small
> > footprint, we patch ADRP instructions into ADR instructions while doing
> > the ELF to PE/COFF conversion.
> >
> > As it turns out, the linker may be doing the same, but for different
> > reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
> > affects ADRP instructions appearing at a certain offset in memory, and
> > one of the mitigations for this erratum is to patch them into ADR
> > instructions at link time if the symbol reference is within -/+ 1 MB.
> > However, the LD linker fails to update the static relocation tables, and
> > so we end up with an ADR instruction in the fully linked binary, but
> > with a relocation entry in the RELA section identifying it as an ADRP
> > instruction.
> >
> > Since the linker has already updated the symbol reference, there is no
> > handling needed in GenFw for such instructions, and we can simply treat
> > it as an ordinary ADR. However, since it is guaranteed to be accompanied
> > by an add or load instruction with a LO12 relocation referencing the same
> > symbol, the section offset check we apply to ADR instructions is going to
> > take place anyway, so we can just disregard the ADR instruction entirely.
> >
> > Reported-by: Eugene Cohen <eugene@hp.com>
> > Suggested-by: Eugene Cohen <eugene@hp.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > index d574300ac4fe..d623dce1f9da 100644
> > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > @@ -1044,6 +1044,19 @@ WriteSections64 (
> >              /* fall through */
> >
> >            case R_AARCH64_ADR_PREL_PG_HI21:
> > +            //
> > +            // In order to handle Cortex-A53 erratum #843419, the LD linker may
> > +            // convert ADRP instructions into ADR instructions, but without
> > +            // updating the static relocation type, and so we may end up here
> > +            // while the instruction in question is actually ADR. So let's
> > +            // just disregard it: the section offset check we apply below to
> > +            // ADR instructions will trigger for its R_AARCH64_xxx_ABS_LO12_NC
> > +            // companion instruction as well, so it is safe to omit it here.
> > +            //
> > +            if ((*(UINT32 *)Targ & BIT31) == 0) {
> > +              break;
> > +            }
> > +
> >              //
> >              // AArch64 PG_H21 relocations are typically paired with ABS_LO12
> >              // relocations, where a PC-relative reference with +/- 4 GB range is
> > --
> > 2.17.1
> >

Re: [edk2-devel] [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Liming Gao]

Acked-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ard
>Biesheuvel
>Sent: Thursday, November 07, 2019 5:06 PM
>To: devel@edk2.groups.io
>Cc: eugene@hp.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: [edk2-devel] [PATCH 1/1] BaseTools/GenFw AARCH64: disregard
>ADRP instructions that are patched already
>
>In order to permit the use of compilers that only implement the small
>code model [which involves the use of ADRP instructions that require
>4 KB segment alignment] for generating PE/COFF binaries with a small
>footprint, we patch ADRP instructions into ADR instructions while doing
>the ELF to PE/COFF conversion.
>
>As it turns out, the linker may be doing the same, but for different
>reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
>affects ADRP instructions appearing at a certain offset in memory, and
>one of the mitigations for this erratum is to patch them into ADR
>instructions at link time if the symbol reference is within -/+ 1 MB.
>However, the LD linker fails to update the static relocation tables, and
>so we end up with an ADR instruction in the fully linked binary, but
>with a relocation entry in the RELA section identifying it as an ADRP
>instruction.
>
>Since the linker has already updated the symbol reference, there is no
>handling needed in GenFw for such instructions, and we can simply treat
>it as an ordinary ADR. However, since it is guaranteed to be accompanied
>by an add or load instruction with a LO12 relocation referencing the same
>symbol, the section offset check we apply to ADR instructions is going to
>take place anyway, so we can just disregard the ADR instruction entirely.
>
>Reported-by: Eugene Cohen <eugene@hp.com>
>Suggested-by: Eugene Cohen <eugene@hp.com>
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>b/BaseTools/Source/C/GenFw/Elf64Convert.c
>index d574300ac4fe..d623dce1f9da 100644
>--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>@@ -1044,6 +1044,19 @@ WriteSections64 (
>             /* fall through */
>
>           case R_AARCH64_ADR_PREL_PG_HI21:
>+            //
>+            // In order to handle Cortex-A53 erratum #843419, the LD linker may
>+            // convert ADRP instructions into ADR instructions, but without
>+            // updating the static relocation type, and so we may end up here
>+            // while the instruction in question is actually ADR. So let's
>+            // just disregard it: the section offset check we apply below to
>+            // ADR instructions will trigger for its R_AARCH64_xxx_ABS_LO12_NC
>+            // companion instruction as well, so it is safe to omit it here.
>+            //
>+            if ((*(UINT32 *)Targ & BIT31) == 0) {
>+              break;
>+            }
>+
>             //
>             // AArch64 PG_H21 relocations are typically paired with ABS_LO12
>             // relocations, where a PC-relative reference with +/- 4 GB range is
>--
>2.17.1
>
>
>

Re: [edk2-devel] [PATCH 1/1] BaseTools/GenFw AARCH64: disregard ADRP instructions that are patched already

[Ard Biesheuvel]

On Fri, 8 Nov 2019 at 01:39, Gao, Liming <liming.gao@intel.com> wrote:
>
> Acked-by: Liming Gao <liming.gao@intel.com>
>

Thanks all

Pushed as 9e9f0be353d4..f55c76b301f1

> >-----Original Message-----
> >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ard
> >Biesheuvel
> >Sent: Thursday, November 07, 2019 5:06 PM
> >To: devel@edk2.groups.io
> >Cc: eugene@hp.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >Subject: [edk2-devel] [PATCH 1/1] BaseTools/GenFw AARCH64: disregard
> >ADRP instructions that are patched already
> >
> >In order to permit the use of compilers that only implement the small
> >code model [which involves the use of ADRP instructions that require
> >4 KB segment alignment] for generating PE/COFF binaries with a small
> >footprint, we patch ADRP instructions into ADR instructions while doing
> >the ELF to PE/COFF conversion.
> >
> >As it turns out, the linker may be doing the same, but for different
> >reasons: there is a silicon erratum #843419 for ARM Cortex-A53 which
> >affects ADRP instructions appearing at a certain offset in memory, and
> >one of the mitigations for this erratum is to patch them into ADR
> >instructions at link time if the symbol reference is within -/+ 1 MB.
> >However, the LD linker fails to update the static relocation tables, and
> >so we end up with an ADR instruction in the fully linked binary, but
> >with a relocation entry in the RELA section identifying it as an ADRP
> >instruction.
> >
> >Since the linker has already updated the symbol reference, there is no
> >handling needed in GenFw for such instructions, and we can simply treat
> >it as an ordinary ADR. However, since it is guaranteed to be accompanied
> >by an add or load instruction with a LO12 relocation referencing the same
> >symbol, the section offset check we apply to ADR instructions is going to
> >take place anyway, so we can just disregard the ADR instruction entirely.
> >
> >Reported-by: Eugene Cohen <eugene@hp.com>
> >Suggested-by: Eugene Cohen <eugene@hp.com>
> >Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >---
> > BaseTools/Source/C/GenFw/Elf64Convert.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> >diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> >b/BaseTools/Source/C/GenFw/Elf64Convert.c
> >index d574300ac4fe..d623dce1f9da 100644
> >--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> >+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> >@@ -1044,6 +1044,19 @@ WriteSections64 (
> >             /* fall through */
> >
> >           case R_AARCH64_ADR_PREL_PG_HI21:
> >+            //
> >+            // In order to handle Cortex-A53 erratum #843419, the LD linker may
> >+            // convert ADRP instructions into ADR instructions, but without
> >+            // updating the static relocation type, and so we may end up here
> >+            // while the instruction in question is actually ADR. So let's
> >+            // just disregard it: the section offset check we apply below to
> >+            // ADR instructions will trigger for its R_AARCH64_xxx_ABS_LO12_NC
> >+            // companion instruction as well, so it is safe to omit it here.
> >+            //
> >+            if ((*(UINT32 *)Targ & BIT31) == 0) {
> >+              break;
> >+            }
> >+
> >             //
> >             // AArch64 PG_H21 relocations are typically paired with ABS_LO12
> >             // relocations, where a PC-relative reference with +/- 4 GB range is
> >--
> >2.17.1
> >
> >
> >
>