[PATCH, RFC, RFT] ARM relocation fixes

[PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 348 bytes --]

Current ARM relocation doesn't handle the cases when the relocation cant
be satisfied directly (like thumb call over 1M of distance or jump24 to
thumb mode. Attached patch adds missing tampoline and missing relocation
handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
EFI).
I couldn't test it on either arm-efi or ARM64

[-- Attachment #1.2: arm.diff --]
[-- Type: application/x-patch, Size: 29962 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Current ARM relocation doesn't handle the cases when the relocation cant
> be satisfied directly (like thumb call over 1M of distance or jump24 to
> thumb mode. Attached patch adds missing tampoline and missing relocation
> handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
> EFI).
> I couldn't test it on either arm-efi or ARM64

Amusingly, I wrote the attached on Saturday, based on a bug report
from Jon Masters @ Red Hat. Although an unlikely corner case, it does
probably need the addition of grub_arch_dl_get_tramp_got_size() from
your implementation in order to ensure the "veneers"[1] don't end up
in a heap region different to and too far away from the one the module
is loaded into.

I'll have a look and a poke on both 32- and 64-bit stuff and respond..

I would say the modifications to grub-mkimage for arm64 are probably
unnessecary: AArch64 relative branch range is +-128MB, and I don't
think we'll see grub kernel images that big.

/
    Leif

[1]
ARM terminology - "trampolines" for ARM refers to something generated
on the stack, which we don't see anymore since the nested functions were
removed.

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

*cough*

With actual attachment.

On Mon, Dec 02, 2013 at 11:53:31AM +0100, Leif Lindholm wrote:
> On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Current ARM relocation doesn't handle the cases when the relocation cant
> > be satisfied directly (like thumb call over 1M of distance or jump24 to
> > thumb mode. Attached patch adds missing tampoline and missing relocation
> > handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
> > EFI).
> > I couldn't test it on either arm-efi or ARM64
> 
> Amusingly, I wrote the attached on Saturday, based on a bug report
> from Jon Masters @ Red Hat. Although an unlikely corner case, it does
> probably need the addition of grub_arch_dl_get_tramp_got_size() from
> your implementation in order to ensure the "veneers"[1] don't end up
> in a heap region different to and too far away from the one the module
> is loaded into.
> 
> I'll have a look and a poke on both 32- and 64-bit stuff and respond..
> 
> I would say the modifications to grub-mkimage for arm64 are probably
> unnessecary: AArch64 relative branch range is +-128MB, and I don't
> think we'll see grub kernel images that big.
> 
> /
>     Leif
> 
> [1]
> ARM terminology - "trampolines" for ARM refers to something generated
> on the stack, which we don't see anymore since the nested functions were
> removed.

[-- Attachment #2: 0001-arm64-dl-generate-veneers-for-out-of-range-relocatio.patch --]
[-- Type: text/x-diff, Size: 2480 bytes --]

From 8d7948641b864168acc67cf0a4834585b3242748 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Sat, 30 Nov 2013 13:05:34 +0000
Subject: [PATCH] arm64: dl: generate veneers for out-of-range relocations

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/arm64/dl_helper.c |   45 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/grub-core/kern/arm64/dl_helper.c b/grub-core/kern/arm64/dl_helper.c
index ae4bce8..a473661 100644
--- a/grub-core/kern/arm64/dl_helper.c
+++ b/grub-core/kern/arm64/dl_helper.c
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <grub/cache.h>
 #include <grub/dl.h>
 #include <grub/elf.h>
 #include <grub/misc.h>
@@ -32,6 +33,46 @@ sign_compress_offset (grub_ssize_t offset, int bitpos)
 }
 
 /*
+ * AArch64 relative branch offset range is +-128MB.
+ * Modules are loaded onto the heap, which may be further away from the
+ * kernel than this. The workaround for this is in ARM-terminology called
+ * a "veneer" - a short sequence of a literal load, a branch to register
+ * and immediately following: the literal (64-bit aligned) 64-bit value.
+ */
+#ifndef GRUB_UTIL
+#define VENEER_LOAD 0x58000050 /* LDR  x16, [PC + 8] */
+#define VENEER_JUMP 0xd61f0200 /* BR   x16  */
+struct veneer {
+  grub_uint32_t load;
+  grub_uint32_t jump;
+  Elf64_Addr target;
+};
+
+static grub_err_t
+add_veneer (grub_uint32_t *place, Elf64_Addr adjust)
+{
+  struct veneer *current;
+  grub_err_t retval;
+
+  current = grub_malloc (sizeof(struct veneer));
+  if (!current)
+    return GRUB_ERR_OUT_OF_MEMORY;
+
+  current->load = VENEER_LOAD;
+  current->jump = VENEER_JUMP;
+  current->target = adjust;
+
+  retval = grub_arm64_reloc_xxxx26 (place, (Elf64_Addr) current);
+  if (retval == GRUB_ERR_NONE)
+    grub_arch_sync_caches (current, sizeof(struct veneer));
+  else
+    grub_free (current);
+
+  return retval;
+}
+#endif
+
+/*
  * grub_arm64_reloc_xxxx26():
  *
  * JUMP26/CALL26 relocations for B and BL instructions.
@@ -54,8 +95,12 @@ grub_arm64_reloc_xxxx26 (grub_uint32_t *place, Elf64_Addr adjust)
 
   if ((offset < offset_low) || (offset > offset_high))
     {
+#ifndef GRUB_UTIL
+      return add_veneer (place, adjust);
+#else
       return grub_error (GRUB_ERR_BAD_MODULE,
 			 N_("CALL26 Relocation out of range"));
+#endif
     }
 
   grub_dprintf ("dl", "  reloc_xxxx64 %p %c= 0x%llx\n",
-- 
1.7.10.4

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On 02.12.2013 11:53, Leif Lindholm wrote:
> On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> Current ARM relocation doesn't handle the cases when the relocation cant
>> be satisfied directly (like thumb call over 1M of distance or jump24 to
>> thumb mode. Attached patch adds missing tampoline and missing relocation
>> handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
>> EFI).
>> I couldn't test it on either arm-efi or ARM64
> 
> Amusingly, I wrote the attached
Nothing is attached to your mail. Right now could you pause work on
*/dl.c: I'm reorganising them to declare more of it as platform
independent and unify handling (ARM dl.c is unnecessarily different from
other versions and forget some of ELF handling)
> on Saturday, based on a bug report
> from Jon Masters @ Red Hat. Although an unlikely corner case, it does
> probably need the addition of grub_arch_dl_get_tramp_got_size() from
> your implementation in order to ensure the "veneers"[1] don't end up
> in a heap region different to and too far away from the one the module
> is loaded into.
> 
> I'll have a look and a poke on both 32- and 64-bit stuff and respond..
> 
> I would say the modifications to grub-mkimage for arm64 are probably
> unnessecary: AArch64 relative branch range is +-128MB, and I don't
> think we'll see grub kernel images that big.
There are no changes of this kind to mkimage. I only added missing ARM
handling and the functions which now became mkimage-specific were moved
to it.
> 
> /
>     Leif
> 
> [1]
> ARM terminology - "trampolines" for ARM refers to something generated
> on the stack, which we don't see anymore since the nested functions were
> removed.
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 11:59:44AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 02.12.2013 11:53, Leif Lindholm wrote:
> > On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> Current ARM relocation doesn't handle the cases when the relocation cant
> >> be satisfied directly (like thumb call over 1M of distance or jump24 to
> >> thumb mode. Attached patch adds missing tampoline and missing relocation
> >> handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
> >> EFI).
> >> I couldn't test it on either arm-efi or ARM64
> > 
> > Amusingly, I wrote the attached
> Nothing is attached to your mail. Right now could you pause work on
> */dl.c: I'm reorganising them to declare more of it as platform
> independent and unify handling (ARM dl.c is unnecessarily different from
> other versions and forget some of ELF handling)

Ok, I'll hold off.

> > on Saturday, based on a bug report
> > from Jon Masters @ Red Hat. Although an unlikely corner case, it does
> > probably need the addition of grub_arch_dl_get_tramp_got_size() from
> > your implementation in order to ensure the "veneers"[1] don't end up
> > in a heap region different to and too far away from the one the module
> > is loaded into.
> > 
> > I'll have a look and a poke on both 32- and 64-bit stuff and respond..
> > 
> > I would say the modifications to grub-mkimage for arm64 are probably
> > unnessecary: AArch64 relative branch range is +-128MB, and I don't
> > think we'll see grub kernel images that big.
>
> There are no changes of this kind to mkimage. I only added missing ARM
> handling and the functions which now became mkimage-specific were moved
> to it.

Well, I was referring specifically to:
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -731,13 +731,13 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
                   case R_AARCH64_JUMP26:
                   case R_AARCH64_CALL26:
                     {
-                      grub_err_t err;
                       sym_addr -= offset;
                       sym_addr -= SUFFIX (entry_point);
-                      err = grub_arm64_reloc_xxxx26((grub_uint32_t *)target,
+                      if (!grub_arm_64_check_xxxx26_offset (sym_addr))
+                        grub_util_error ("%s", _("CALL26 Relocation out of range"));
+
+                      grub_arm64_set_xxxx26_offset((grub_uint32_t *)target,
                                                     sym_addr);
-                      if (err)
-                        grub_util_error ("%s", grub_errmsg);
                     }
                     break;
                   default:
which isn't wrong, but that offset test can only fail when your GRUB
kernel is greater than 128MB in size.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

On 02.12.2013 12:28, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 11:59:44AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 02.12.2013 11:53, Leif Lindholm wrote:
>>> On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> Current ARM relocation doesn't handle the cases when the relocation cant
>>>> be satisfied directly (like thumb call over 1M of distance or jump24 to
>>>> thumb mode. Attached patch adds missing tampoline and missing relocation
>>>> handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
>>>> EFI).
>>>> I couldn't test it on either arm-efi or ARM64
>>>
>>> Amusingly, I wrote the attached
>> Nothing is attached to your mail. Right now could you pause work on
>> */dl.c: I'm reorganising them to declare more of it as platform
>> independent and unify handling (ARM dl.c is unnecessarily different from
>> other versions and forget some of ELF handling)
> 
> Ok, I'll hold off.
> 
>>> on Saturday, based on a bug report
>>> from Jon Masters @ Red Hat. Although an unlikely corner case, it does
>>> probably need the addition of grub_arch_dl_get_tramp_got_size() from
>>> your implementation in order to ensure the "veneers"[1] don't end up
>>> in a heap region different to and too far away from the one the module
>>> is loaded into.
>>>
>>> I'll have a look and a poke on both 32- and 64-bit stuff and respond..
>>>
>>> I would say the modifications to grub-mkimage for arm64 are probably
>>> unnessecary: AArch64 relative branch range is +-128MB, and I don't
>>> think we'll see grub kernel images that big.
>>
>> There are no changes of this kind to mkimage. I only added missing ARM
>> handling and the functions which now became mkimage-specific were moved
>> to it.
> 
> Well, I was referring specifically to:
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -731,13 +731,13 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
>                    case R_AARCH64_JUMP26:
>                    case R_AARCH64_CALL26:
>                      {
> -                      grub_err_t err;
>                        sym_addr -= offset;
>                        sym_addr -= SUFFIX (entry_point);
> -                      err = grub_arm64_reloc_xxxx26((grub_uint32_t *)target,
> +                      if (!grub_arm_64_check_xxxx26_offset (sym_addr))
> +                        grub_util_error ("%s", _("CALL26 Relocation out of range"));
> +
> +                      grub_arm64_set_xxxx26_offset((grub_uint32_t *)target,
>                                                      sym_addr);
> -                      if (err)
> -                        grub_util_error ("%s", grub_errmsg);
>                      }
>                      break;
>                    default:
> which isn't wrong, but that offset test can only fail when your GRUB
> kernel is greater than 128MB in size.
> 
True that's why it just aborts the image creation.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

On 02.12.2013 12:28, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 11:59:44AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 02.12.2013 11:53, Leif Lindholm wrote:
>>> On Sun, Dec 01, 2013 at 07:06:32AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> Current ARM relocation doesn't handle the cases when the relocation cant
>>>> be satisfied directly (like thumb call over 1M of distance or jump24 to
>>>> thumb mode. Attached patch adds missing tampoline and missing relocation
>>>> handling to EFI code (it didn't allow to use ARM (no-Thumb) binary with
>>>> EFI).
>>>> I couldn't test it on either arm-efi or ARM64
>>>
>>> Amusingly, I wrote the attached
>> Nothing is attached to your mail. Right now could you pause work on
>> */dl.c: I'm reorganising them to declare more of it as platform
>> independent and unify handling (ARM dl.c is unnecessarily different from
>> other versions and forget some of ELF handling)
> 
> Ok, I'll hold off.
> 
I've uploaded current work to phcoder/reloc
>>> on Saturday, based on a bug report
>>> from Jon Masters @ Red Hat. Although an unlikely corner case, it does
>>> probably need the addition of grub_arch_dl_get_tramp_got_size() from
>>> your implementation in order to ensure the "veneers"[1] don't end up
>>> in a heap region different to and too far away from the one the module
>>> is loaded into.
>>>
>>> I'll have a look and a poke on both 32- and 64-bit stuff and respond..
>>>
>>> I would say the modifications to grub-mkimage for arm64 are probably
>>> unnessecary: AArch64 relative branch range is +-128MB, and I don't
>>> think we'll see grub kernel images that big.
>>
>> There are no changes of this kind to mkimage. I only added missing ARM
>> handling and the functions which now became mkimage-specific were moved
>> to it.
> 
> Well, I was referring specifically to:
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -731,13 +731,13 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
>                    case R_AARCH64_JUMP26:
>                    case R_AARCH64_CALL26:
>                      {
> -                      grub_err_t err;
>                        sym_addr -= offset;
>                        sym_addr -= SUFFIX (entry_point);
> -                      err = grub_arm64_reloc_xxxx26((grub_uint32_t *)target,
> +                      if (!grub_arm_64_check_xxxx26_offset (sym_addr))
> +                        grub_util_error ("%s", _("CALL26 Relocation out of range"));
> +
> +                      grub_arm64_set_xxxx26_offset((grub_uint32_t *)target,
>                                                      sym_addr);
> -                      if (err)
> -                        grub_util_error ("%s", grub_errmsg);
>                      }
>                      break;
>                    default:
> which isn't wrong, but that offset test can only fail when your GRUB
> kernel is greater than 128MB in size.
> 
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 12:46:13PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >>> Amusingly, I wrote the attached
> >> Nothing is attached to your mail. Right now could you pause work on
> >> */dl.c: I'm reorganising them to declare more of it as platform
> >> independent and unify handling (ARM dl.c is unnecessarily different from
> >> other versions and forget some of ELF handling)
> > 
> > Ok, I'll hold off.
> > 
> I've uploaded current work to phcoder/reloc

Ok, so I've tested this on arm64, and it works on the commercial FVP
Base model (which does not trigger veneer generation due to its runtime
memory map), but crashes on the Foundation model (which does).
So the generic dl refactoring seems correct, but somehing about the
veneers is fishy.

My default ARMv7 UEFI build fails to grub-install with
/work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.

I am investigating these issues.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 02:30:51PM +0100, Leif Lindholm wrote:
> > >>> Amusingly, I wrote the attached
> > >> Nothing is attached to your mail. Right now could you pause work on
> > >> */dl.c: I'm reorganising them to declare more of it as platform
> > >> independent and unify handling (ARM dl.c is unnecessarily different from
> > >> other versions and forget some of ELF handling)
> > > 
> > > Ok, I'll hold off.
> > > 
> > I've uploaded current work to phcoder/reloc
> 
> Ok, so I've tested this on arm64, and it works on the commercial FVP
> Base model (which does not trigger veneer generation due to its runtime
> memory map), but crashes on the Foundation model (which does).
> So the generic dl refactoring seems correct, but somehing about the
> veneers is fishy.

Well, this one seems to be because mod->trampptr is never initilised
(which should be a problem also on the other archs?).

With
diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index 33ccc98..92027e0 100644
--- a/grub-core/kern/dl.c
+++ b/grub-core/kern/dl.c
@@ -317,6 +317,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
 #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
   ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
   mod->tramp = ptr;
+  mod->trampptr = (ptr + tramp);
   ptr += tramp;
   ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
   mod->got = ptr;

This runs successfully on my foundation model.

Presumably, on other architectures a similar thing for mod->gotptr is
required?

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 03:14:50PM +0100, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 02:30:51PM +0100, Leif Lindholm wrote:
> > > >>> Amusingly, I wrote the attached
> > > >> Nothing is attached to your mail. Right now could you pause work on
> > > >> */dl.c: I'm reorganising them to declare more of it as platform
> > > >> independent and unify handling (ARM dl.c is unnecessarily different from
> > > >> other versions and forget some of ELF handling)
> > > > 
> > > > Ok, I'll hold off.
> > > > 
> > > I've uploaded current work to phcoder/reloc
> > 
> > Ok, so I've tested this on arm64, and it works on the commercial FVP
> > Base model (which does not trigger veneer generation due to its runtime
> > memory map), but crashes on the Foundation model (which does).
> > So the generic dl refactoring seems correct, but somehing about the
> > veneers is fishy.
> 
> Well, this one seems to be because mod->trampptr is never initilised
> (which should be a problem also on the other archs?).
> 
> With
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 33ccc98..92027e0 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -317,6 +317,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
>    mod->tramp = ptr;
> +  mod->trampptr = (ptr + tramp);
>    ptr += tramp;
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
>    mod->got = ptr;
> 
> This runs successfully on my foundation model.

Umm, that's obviously not correct. But it did work :)

What is needed is to store the size of the module before adding
trampoline and got sizes.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 03:33:06PM +0100, Leif Lindholm wrote:
> > > Ok, so I've tested this on arm64, and it works on the commercial FVP
> > > Base model (which does not trigger veneer generation due to its runtime
> > > memory map), but crashes on the Foundation model (which does).
> > > So the generic dl refactoring seems correct, but somehing about the
> > > veneers is fishy.
> > 
> > Well, this one seems to be because mod->trampptr is never initilised
> > (which should be a problem also on the other archs?).
> > 
> > With
> > diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> > index 33ccc98..92027e0 100644
> > --- a/grub-core/kern/dl.c
> > +++ b/grub-core/kern/dl.c
> > @@ -317,6 +317,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
> >  #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
> >    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
> >    mod->tramp = ptr;
> > +  mod->trampptr = (ptr + tramp);
> >    ptr += tramp;
> >    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
> >    mod->got = ptr;
> > 
> > This runs successfully on my foundation model.
> 
> Umm, that's obviously not correct. But it did work :)
> 
> What is needed is to store the size of the module before adding
> trampoline and got sizes.

So - after some much needed Lunch, and coffee, I think the below would
be correct?

diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index 33ccc98..a83b744 100644
--- a/grub-core/kern/dl.c
+++ b/grub-core/kern/dl.c
@@ -316,10 +316,10 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
     }
 #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
   ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
-  mod->tramp = ptr;
+  mod->tramp = mod->trampptr = ptr;
   ptr += tramp;
   ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
-  mod->got = ptr;
+  mod->got = mod->gotptr = ptr;
   ptr += got;
 #endif
 
/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On 02.12.2013 15:14, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 02:30:51PM +0100, Leif Lindholm wrote:
>>>>>> Amusingly, I wrote the attached
>>>>> Nothing is attached to your mail. Right now could you pause work on
>>>>> */dl.c: I'm reorganising them to declare more of it as platform
>>>>> independent and unify handling (ARM dl.c is unnecessarily different from
>>>>> other versions and forget some of ELF handling)
>>>>
>>>> Ok, I'll hold off.
>>>>
>>> I've uploaded current work to phcoder/reloc
>>
>> Ok, so I've tested this on arm64, and it works on the commercial FVP
>> Base model (which does not trigger veneer generation due to its runtime
>> memory map), but crashes on the Foundation model (which does).
>> So the generic dl refactoring seems correct, but somehing about the
>> veneers is fishy.
> 
> Well, this one seems to be because mod->trampptr is never initilised
> (which should be a problem also on the other archs?).
> 
Thanks, good catch, fixed, well it's my WIP branch.
> With
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 33ccc98..92027e0 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -317,6 +317,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
>    mod->tramp = ptr;
> +  mod->trampptr = (ptr + tramp);
>    ptr += tramp;
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
>    mod->got = ptr;
> 
> This runs successfully on my foundation model.
> 
> Presumably, on other architectures a similar thing for mod->gotptr is
> required?
> 
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> .
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

On 02.12.2013 18:32, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 03:33:06PM +0100, Leif Lindholm wrote:
>>>> Ok, so I've tested this on arm64, and it works on the commercial FVP
>>>> Base model (which does not trigger veneer generation due to its runtime
>>>> memory map), but crashes on the Foundation model (which does).
>>>> So the generic dl refactoring seems correct, but somehing about the
>>>> veneers is fishy.
>>>
>>> Well, this one seems to be because mod->trampptr is never initilised
>>> (which should be a problem also on the other archs?).
>>>
>>> With
>>> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
>>> index 33ccc98..92027e0 100644
>>> --- a/grub-core/kern/dl.c
>>> +++ b/grub-core/kern/dl.c
>>> @@ -317,6 +317,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>>>  #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
>>>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
>>>    mod->tramp = ptr;
>>> +  mod->trampptr = (ptr + tramp);
>>>    ptr += tramp;
>>>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
>>>    mod->got = ptr;
>>>
>>> This runs successfully on my foundation model.
>>
>> Umm, that's obviously not correct. But it did work :)
>>
>> What is needed is to store the size of the module before adding
>> trampoline and got sizes.
> 
> So - after some much needed Lunch, and coffee, I think the below would
> be correct?
> 
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 33ccc98..a83b744 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -316,10 +316,10 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>      }
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined (__sparc__)
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
> -  mod->tramp = ptr;
> +  mod->tramp = mod->trampptr = ptr;
>    ptr += tramp;
>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_GOT_ALIGN);
> -  mod->got = ptr;
> +  mod->got = mod->gotptr = ptr;
>    ptr += got;
>  #endif
>  
I've committed a fix before I saw this message but it did essentially
the same.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On 02.12.2013 14:30, Leif Lindholm wrote:
> My default ARMv7 UEFI build fails to grub-install with
> /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
This is a problem because of asm functions which are always ARM and gcc
uses bl to jump to them from thumb that an't be satisified. I wonder if
it's better to add veneers to mkimage or to add explicit thumb interwork
to all asm functions like I did in my other patch. In my local build
I've cheated by decraling all those functions with __attribute__
((long_call)) but it's probably not right solution so I didn't commit it.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 06:45:48PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > My default ARMv7 UEFI build fails to grub-install with
> > /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
>
> This is a problem because of asm functions which are always ARM and gcc
> uses bl to jump to them from thumb that an't be satisified. I wonder if
> it's better to add veneers to mkimage or to add explicit thumb interwork
> to all asm functions like I did in my other patch.

Ah. When linking with a standalone linker, it rewrites BL to BLX where
this is required for state change - so doing that #ifdef GRUB_UTIL
wouldn't be wrong.

> In my local build
> I've cheated by decraling all those functions with __attribute__
> ((long_call)) but it's probably not right solution so I didn't commit it.

On a lazy note, putting -mlong-calls back into CFLAGS_PLATFORM resolves
this issue (since it forces full address loads into PC rather than
using relative addressing and unconditional instruction set state
change).

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On 02.12.2013 20:40, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 06:45:48PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> My default ARMv7 UEFI build fails to grub-install with
>>> /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
>>
>> This is a problem because of asm functions which are always ARM and gcc
>> uses bl to jump to them from thumb that an't be satisified. I wonder if
>> it's better to add veneers to mkimage or to add explicit thumb interwork
>> to all asm functions like I did in my other patch.
> 
> Ah. When linking with a standalone linker, it rewrites BL to BLX where
> this is required for state change
Doesn't this require Thumb2 ?



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Mon, Dec 02, 2013 at 09:04:47PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >>> My default ARMv7 UEFI build fails to grub-install with
> >>> /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
> >>
> >> This is a problem because of asm functions which are always ARM and gcc
> >> uses bl to jump to them from thumb that an't be satisified. I wonder if
> >> it's better to add veneers to mkimage or to add explicit thumb interwork
> >> to all asm functions like I did in my other patch.
> > 
> > Ah. When linking with a standalone linker, it rewrites BL to BLX where
> > this is required for state change
>
> Doesn't this require Thumb2 ?

No, BLX immediate has been supported since ARMv5T.
(Yes, that is effectively a 32-bit encoding in what is a 16-bit
instruction set - but so is the basic BL.)

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

On 02.12.2013 21:46, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 09:04:47PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>>> My default ARMv7 UEFI build fails to grub-install with
>>>>> /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
>>>>
>>>> This is a problem because of asm functions which are always ARM and gcc
>>>> uses bl to jump to them from thumb that an't be satisified. I wonder if
>>>> it's better to add veneers to mkimage or to add explicit thumb interwork
>>>> to all asm functions like I did in my other patch.
>>>
>>> Ah. When linking with a standalone linker, it rewrites BL to BLX where
>>> this is required for state change
>>
>> Doesn't this require Thumb2 ?
> 
> No, BLX immediate has been supported since ARMv5T.
> (Yes, that is effectively a 32-bit encoding in what is a 16-bit
> instruction set - but so is the basic BL.)
> 
I've looked through encoding of those instructions and see how much it's
a mess. b* and b*x don't have similar set of options which makes
validating them a difficult error-prone task. So I think, I'll just add
veneers to mkimage, just like we do on ia64 (either by making a
pc-relative variant of veneers or adding fixup for them)
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

On 02.12.2013 21:46, Leif Lindholm wrote:
> On Mon, Dec 02, 2013 at 09:04:47PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>>> My default ARMv7 UEFI build fails to grub-install with
>>>>> /work/local/grub/uefi/sbin/grub-install: error: bl/b.w targettting ARM.
>>>>
>>>> This is a problem because of asm functions which are always ARM and gcc
>>>> uses bl to jump to them from thumb that an't be satisified. I wonder if
>>>> it's better to add veneers to mkimage or to add explicit thumb interwork
>>>> to all asm functions like I did in my other patch.
>>>
>>> Ah. When linking with a standalone linker, it rewrites BL to BLX where
>>> this is required for state change
>>
>> Doesn't this require Thumb2 ?
> 
> No, BLX immediate has been supported since ARMv5T.
> (Yes, that is effectively a 32-bit encoding in what is a 16-bit
> instruction set - but so is the basic BL.)
> 
In my branch I added veneers to mkimage. Do you have a tutorial on
setting up arm-efi VM?
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Tue, Dec 03, 2013 at 06:37:33AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > No, BLX immediate has been supported since ARMv5T.
> > (Yes, that is effectively a 32-bit encoding in what is a 16-bit
> > instruction set - but so is the basic BL.)
> > 
> I've looked through encoding of those instructions and see how much it's
> a mess. b* and b*x don't have similar set of options which makes
> validating them a difficult error-prone task. So I think, I'll just add
> veneers to mkimage, just like we do on ia64 (either by making a
> pc-relative variant of veneers or adding fixup for them)

Not B, BL.
There is a 1-bit range difference between Thumb BL and BLX, which we
need to check for anyway. This check already exists (and must exist) in
the code. Adding veneers would be pure overhead.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

On 03.12.2013 09:14, Leif Lindholm wrote:
> On Tue, Dec 03, 2013 at 06:37:33AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> No, BLX immediate has been supported since ARMv5T.
>>> (Yes, that is effectively a 32-bit encoding in what is a 16-bit
>>> instruction set - but so is the basic BL.)
>>>
>> I've looked through encoding of those instructions and see how much it's
>> a mess. b* and b*x don't have similar set of options which makes
>> validating them a difficult error-prone task. So I think, I'll just add
>> veneers to mkimage, just like we do on ia64 (either by making a
>> pc-relative variant of veneers or adding fixup for them)
> 
> Not B, BL.
> There is a 1-bit range difference between Thumb BL and BLX, which we
> need to check for anyway. This check already exists (and must exist) in
> the code. Adding veneers would be pure overhead.
> 
I meant that you can use conditions with bl but not blx. So if we have a
reloc on ARM bl.e targetting Thumb then we have to add veneers. Since we
have only small number of interworking calls it's probably easier to
always add veneers on interworking relative relocations rather than
having micro-optimisation and get some minor case wrong.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Tue, Dec 03, 2013 at 09:22:56AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> I've looked through encoding of those instructions and see how much it's
> >> a mess. b* and b*x don't have similar set of options which makes
> >> validating them a difficult error-prone task. So I think, I'll just add
> >> veneers to mkimage, just like we do on ia64 (either by making a
> >> pc-relative variant of veneers or adding fixup for them)
> > 
> > Not B, BL.
> > There is a 1-bit range difference between Thumb BL and BLX, which we
> > need to check for anyway. This check already exists (and must exist) in
> > the code. Adding veneers would be pure overhead.
> 
> I meant that you can use conditions with bl but not blx. So if we have a
> reloc on ARM bl.e targetting Thumb then we have to add veneers. Since we
> have only small number of interworking calls it's probably easier to
> always add veneers on interworking relative relocations rather than
> having micro-optimisation and get some minor case wrong.

OK, but the only place we could ever have a problem with this would
be if we had asm in the kernel _explicitly_ done as .thumb.
Which we don't. We explicitly moved away from that in order to have
support for pre-v7 processors.

All modules will have full 32-bit external references, so will not
use these instructions anyway. Any internal references within modules
will be linked with LD, which will fix this up automatically.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

On 03.12.2013 09:47, Leif Lindholm wrote:
> On Tue, Dec 03, 2013 at 09:22:56AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> I've looked through encoding of those instructions and see how much it's
>>>> a mess. b* and b*x don't have similar set of options which makes
>>>> validating them a difficult error-prone task. So I think, I'll just add
>>>> veneers to mkimage, just like we do on ia64 (either by making a
>>>> pc-relative variant of veneers or adding fixup for them)
>>>
>>> Not B, BL.
>>> There is a 1-bit range difference between Thumb BL and BLX, which we
>>> need to check for anyway. This check already exists (and must exist) in
>>> the code. Adding veneers would be pure overhead.
>>
>> I meant that you can use conditions with bl but not blx. So if we have a
>> reloc on ARM bl.e targetting Thumb then we have to add veneers. Since we
>> have only small number of interworking calls it's probably easier to
>> always add veneers on interworking relative relocations rather than
>> having micro-optimisation and get some minor case wrong.
> 
> OK, but the only place we could ever have a problem with this would
> be if we had asm in the kernel _explicitly_ done as .thumb.
> Which we don't. We explicitly moved away from that in order to have
> support for pre-v7 processors.
> 
We also call C code from asm. One such instance (for division
instructions) caused the problem
> All modules will have full 32-bit external references, so will not
> use these instructions anyway. Any internal references within modules
> will be linked with LD, which will fix this up automatically.
> 
In my small test I compiled:
extern void g(void);

void f (int x)
{
  if (!x)
    g();
}
And got following assembly with -Os:

   0:	e3500000 	cmp	r0, #0
   4:	e92d4008 	push	{r3, lr}
   8:	0bfffffe 	bleq	0 <g>
			8: R_ARM_JUMP24	g
   c:	e8bd4008 	pop	{r3, lr}
  10:	e12fff1e 	bx	lr

If g is a function in thumb kernel or thumb module then you need a veneer.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Tue, Dec 03, 2013 at 10:31:13AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> I meant that you can use conditions with bl but not blx. So if we have a
> >> reloc on ARM bl.e targetting Thumb then we have to add veneers. Since we
> >> have only small number of interworking calls it's probably easier to
> >> always add veneers on interworking relative relocations rather than
> >> having micro-optimisation and get some minor case wrong.
> > 
> > OK, but the only place we could ever have a problem with this would
> > be if we had asm in the kernel _explicitly_ done as .thumb.
> > Which we don't. We explicitly moved away from that in order to have
> > support for pre-v7 processors.
> > 
> We also call C code from asm. One such instance (for division
> instructions) caused the problem
> > All modules will have full 32-bit external references, so will not
> > use these instructions anyway. Any internal references within modules
> > will be linked with LD, which will fix this up automatically.
> > 
> In my small test I compiled:
> extern void g(void);
> 
> void f (int x)
> {
>   if (!x)
>     g();
> }
> And got following assembly with -Os:
> 
>    0:	e3500000 	cmp	r0, #0
>    4:	e92d4008 	push	{r3, lr}
>    8:	0bfffffe 	bleq	0 <g>
> 			8: R_ARM_JUMP24	g
>    c:	e8bd4008 	pop	{r3, lr}
>   10:	e12fff1e 	bx	lr
> 
> If g is a function in thumb kernel or thumb module then you need a veneer.

Ok, you got me. Didn't consider -Os.
But the second case would still be auto-added by the linker.

But what is the objection to -mlong-calls?

My armv7 kernel ends up only slightly larger with this option (57272
bytes vs. 57088) - 184 bytes, from which 12 bytes per veneer can be
subtracted. And the overall arm-efi directory is smaller (10031244 vs.
10254924). For just the *.mod too (1229498 vs. 1234034).

When compiling for For ARM (A32) (i.e. armv6), there is no difference
in kernel size, but modules do grow 1.8% from 1477726 to 1503986.
But is it really worth adding complexity to grub-mkimage for a small
benefit to legacy platforms only? Could we instead add an arm_cflags
with -mlong-calls for kernel in Makefile.core.def?

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Leif Lindholm]

On Tue, Dec 03, 2013 at 09:09:36AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >>> Ah. When linking with a standalone linker, it rewrites BL to BLX where
> >>> this is required for state change
> >>
> >> Doesn't this require Thumb2 ?
> > 
> > No, BLX immediate has been supported since ARMv5T.
> > (Yes, that is effectively a 32-bit encoding in what is a 16-bit
> > instruction set - but so is the basic BL.)
> > 
> In my branch I added veneers to mkimage. Do you have a tutorial on
> setting up arm-efi VM?

It is slightly trickier for 32-bit ARM, in that we don't have a costless
model available. There is a port to Qemu underway, but it is not entirely
complete, and requires a patch or two in Qemu.

I can however verify that 559c2fe runs correctly on my TC2 hardware
platform.

/
    Leif

Re: [PATCH, RFC, RFT] ARM relocation fixes

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On 03.12.2013 12:16, Leif Lindholm wrote:
> On Tue, Dec 03, 2013 at 10:31:13AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> I meant that you can use conditions with bl but not blx. So if we have a
>>>> reloc on ARM bl.e targetting Thumb then we have to add veneers. Since we
>>>> have only small number of interworking calls it's probably easier to
>>>> always add veneers on interworking relative relocations rather than
>>>> having micro-optimisation and get some minor case wrong.
>>>
>>> OK, but the only place we could ever have a problem with this would
>>> be if we had asm in the kernel _explicitly_ done as .thumb.
>>> Which we don't. We explicitly moved away from that in order to have
>>> support for pre-v7 processors.
>>>
>> We also call C code from asm. One such instance (for division
>> instructions) caused the problem
>>> All modules will have full 32-bit external references, so will not
>>> use these instructions anyway. Any internal references within modules
>>> will be linked with LD, which will fix this up automatically.
>>>
>> In my small test I compiled:
>> extern void g(void);
>>
>> void f (int x)
>> {
>>   if (!x)
>>     g();
>> }
>> And got following assembly with -Os:
>>
>>    0:	e3500000 	cmp	r0, #0
>>    4:	e92d4008 	push	{r3, lr}
>>    8:	0bfffffe 	bleq	0 <g>
>> 			8: R_ARM_JUMP24	g
>>    c:	e8bd4008 	pop	{r3, lr}
>>   10:	e12fff1e 	bx	lr
>>
>> If g is a function in thumb kernel or thumb module then you need a veneer.
> 
> Ok, you got me. Didn't consider -Os.
> But the second case would still be auto-added by the linker.
> 
LD in -r mode doesn't always resolve all relocs
> But what is the objection to -mlong-calls?
> 
Originally it was from my experiments with clang. It doesn't accept
-mlong-calls. But clang isn't enough of motivation for this complexity,
far from it. My motivation is to have a robust dynamic linker with
interwork possibilities, that we won't have to rewrite when new compiler
changes behaviour or if we decide to decrease the requirement to armv4.
I think, I'll make build system add -mlong-calls if it's supported by
compiler.
> My armv7 kernel ends up only slightly larger with this option (57272
> bytes vs. 57088) - 184 bytes, from which 12 bytes per veneer can be
> subtracted. And the overall arm-efi directory is smaller (10031244 vs.
> 10254924). For just the *.mod too (1229498 vs. 1234034).
> 
> When compiling for For ARM (A32) (i.e. armv6), there is no difference
> in kernel size, but modules do grow 1.8% from 1477726 to 1503986.
I'm confused by numbers: I don't see which ones relate to which configs
(long-calls/no-long-calls A32/T16/T32)
> But is it really worth adding complexity to grub-mkimage for a small
> benefit to legacy platforms only? Could we instead add an arm_cflags
> with -mlong-calls for kernel in Makefile.core.def?
> 
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]