This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [RFC PATCH, binutils, ARM 1/9] Refactor Cortex-A8 erratum workaround in preparation
- From: "Thomas Preud'homme" <thomas dot preudhomme at foss dot arm dot com>
- To: <binutils at sourceware dot org>
- Date: Wed, 23 Dec 2015 15:54:21 +0800
- Subject: RE: [RFC PATCH, binutils, ARM 1/9] Refactor Cortex-A8 erratum workaround in preparation
- Authentication-results: sourceware.org; auth=none
- References: <004501d13d56$d19aa5f0$74cff1d0$ at foss dot arm dot com>
Better when the patch is there.
> From: binutils-owner@sourceware.org [mailto:binutils-
> owner@sourceware.org] On Behalf Of Thomas Preud'homme
> Sent: Wednesday, December 23, 2015 3:52 PM
>
> Hi,
>
> [Posting patch series as RFC]
>
> This patch is part of a patch series to add support for ARMv8-M security
> extension[1] to GNU ld. This specific patch refactors Cortex-A8 erratum
> workaround code to handle Thumb code relocation in stubs in the same
> as other relocations in stubs, as needed by subsequent patches.
>
> Contrary to other types of veneers, the ones for Cortex-A8 erratum
> workaround are generated for branches without relocations against
> them (see comment below "if (found && found->non_a8_stub)" in
> cortex_a8_erratum_scan () function). Because of that, the related code
> needs to patch the source of the branch itself rather than rely on
> elf32_arm_final_link_relocate to be called to relocate it.
>
> Currently, this is done by storing the offset of the source of the branch
> from the beginning of its section in struct elf32_arm_stub_hash_entry's
> target_value field rather than the offset of the target of the branch.
> arm_build_one_stub () then compute the target offset from the source
> offset and the distance between the source and target of the branch
> stored in a new target_addend field of struct
> elf32_arm_stub_hash_entry. That offset is taken from the branch
> immediate and thus includes the addend. Because of that, the relocation
> addend for the veneer must not be used when computing the target
> offset which which led to the creation of the if in arm_build_one_stub ()
> function (see the difference in computation of points_to).
>
> While all this works, this special casing makes the code less maintainable
> and more difficult to understand (especially target_value having a
> different meaning). This patch aims at improving this.
>
>
> [1] Software requirements for ARMv8-M security extension are
> described in document ARM-ECM-0359818 [2]
> [2] Available on http://infocenter.arm.com in Developer guides and
> articles > Software development > ARMÂv8-M Security Extensions:
> Requirements on Development Tools
>
> ChangeLog entry is as follows:
>
>
> *** bfd/ChangeLog ***
>
> 2015-08-07 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * elf32-arm.c (struct elf32_arm_stub_hash_entry): Delete
> target_addend
> field and add source_value.
> (struct a8_erratum_fix): Delete addend field and add target_offset.
> (stub_hash_newfunc): Initialize source_value field amd remove
> initialization for target_addend.
> (arm_build_one_stub): Stop special casing Thumb relocations:
> promote
> the else to being always executed, moving the
> arm_stub_a8_veneer_b_cond specific code in it. Remove
> stub_entry->target_addend from points_to computation.
> (cortex_a8_erratum_scan): Store in a8_erratum_fix structure the
> offset
> to target symbol from start of section rather than the offset from the
> stub address.
> (elf32_arm_size_stubs): Set stub_entry's source_value and
> target_value
> fields from struct a8_erratum_fix's offset and target_offset
> respectively.
> (make_branch_to_a8_stub): Rename target variable to loc.
> Compute
> veneered_insn_loc and loc using stub_entry's source_value.
diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 2f1b212..77082a7 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -2608,8 +2608,12 @@ struct elf32_arm_stub_hash_entry
bfd_vma target_value;
asection *target_section;
- /* Offset to apply to relocation referencing target_value. */
- bfd_vma target_addend;
+ /* Same as above but for the source of the branch to the stub. Used for
+ Cortex-A8 erratum workaround to patch it to branch to the stub. As
+ such, source section does not need to be recorded since Cortex-A8 erratum
+ workaround stubs are only generated when both source and target are in the
+ same section. */
+ bfd_vma source_value;
/* The instruction which caused this stub to be generated (only valid for
Cortex-A8 erratum workaround stubs at present). */
@@ -2777,7 +2781,7 @@ struct a8_erratum_fix
bfd *input_bfd;
asection *section;
bfd_vma offset;
- bfd_vma addend;
+ bfd_vma target_offset;
unsigned long orig_insn;
char *stub_name;
enum elf32_arm_stub_type stub_type;
@@ -3354,9 +3358,9 @@ stub_hash_newfunc (struct bfd_hash_entry *entry,
eh = (struct elf32_arm_stub_hash_entry *) entry;
eh->stub_sec = NULL;
eh->stub_offset = 0;
+ eh->source_value = 0;
eh->target_value = 0;
eh->target_section = NULL;
- eh->target_addend = 0;
eh->orig_insn = 0;
eh->stub_type = arm_stub_none;
eh->stub_size = 0;
@@ -4385,65 +4389,36 @@ arm_build_one_stub (struct bfd_hash_entry *gen_entry,
BFD_ASSERT (nrelocs != 0 && nrelocs <= MAXRELOCS);
for (i = 0; i < nrelocs; i++)
- if (template_sequence[stub_reloc_idx[i]].r_type == R_ARM_THM_JUMP24
- || template_sequence[stub_reloc_idx[i]].r_type == R_ARM_THM_JUMP19
- || template_sequence[stub_reloc_idx[i]].r_type == R_ARM_THM_CALL
- || template_sequence[stub_reloc_idx[i]].r_type == R_ARM_THM_XPC22)
- {
- Elf_Internal_Rela rel;
- bfd_boolean unresolved_reloc;
- char *error_message;
- enum arm_st_branch_type branch_type
- = (template_sequence[stub_reloc_idx[i]].r_type != R_ARM_THM_XPC22
- ? ST_BRANCH_TO_THUMB : ST_BRANCH_TO_ARM);
- bfd_vma points_to = sym_value + stub_entry->target_addend;
-
- rel.r_offset = stub_entry->stub_offset + stub_reloc_offset[i];
- rel.r_info = ELF32_R_INFO (0,
- template_sequence[stub_reloc_idx[i]].r_type);
- rel.r_addend = template_sequence[stub_reloc_idx[i]].reloc_addend;
-
- if (stub_entry->stub_type == arm_stub_a8_veneer_b_cond && i == 0)
- /* The first relocation in the elf32_arm_stub_a8_veneer_b_cond[]
- template should refer back to the instruction after the original
- branch. */
- points_to = sym_value;
-
- /* There may be unintended consequences if this is not true. */
- BFD_ASSERT (stub_entry->h == NULL);
-
- /* Note: _bfd_final_link_relocate doesn't handle these relocations
- properly. We should probably use this function unconditionally,
- rather than only for certain relocations listed in the enclosing
- conditional, for the sake of consistency. */
- elf32_arm_final_link_relocate (elf32_arm_howto_from_type
- (template_sequence[stub_reloc_idx[i]].r_type),
- stub_bfd, info->output_bfd, stub_sec, stub_sec->contents, &rel,
- points_to, info, stub_entry->target_section, "", STT_FUNC,
- branch_type, (struct elf_link_hash_entry *) stub_entry->h,
- &unresolved_reloc, &error_message);
- }
- else
- {
- Elf_Internal_Rela rel;
- bfd_boolean unresolved_reloc;
- char *error_message;
- bfd_vma points_to = sym_value + stub_entry->target_addend
- + template_sequence[stub_reloc_idx[i]].reloc_addend;
-
- rel.r_offset = stub_entry->stub_offset + stub_reloc_offset[i];
- rel.r_info = ELF32_R_INFO (0,
- template_sequence[stub_reloc_idx[i]].r_type);
- rel.r_addend = 0;
-
- elf32_arm_final_link_relocate (elf32_arm_howto_from_type
- (template_sequence[stub_reloc_idx[i]].r_type),
- stub_bfd, info->output_bfd, stub_sec, stub_sec->contents, &rel,
- points_to, info, stub_entry->target_section, "", STT_FUNC,
- stub_entry->branch_type,
- (struct elf_link_hash_entry *) stub_entry->h, &unresolved_reloc,
- &error_message);
- }
+ {
+ Elf_Internal_Rela rel;
+ bfd_boolean unresolved_reloc;
+ char *error_message;
+ bfd_vma points_to =
+ sym_value + template_sequence[stub_reloc_idx[i]].reloc_addend;
+
+ rel.r_offset = stub_entry->stub_offset + stub_reloc_offset[i];
+ rel.r_info = ELF32_R_INFO (0,
+ template_sequence[stub_reloc_idx[i]].r_type);
+ rel.r_addend = 0;
+
+ if (stub_entry->stub_type == arm_stub_a8_veneer_b_cond && i == 0)
+ /* The first relocation in the elf32_arm_stub_a8_veneer_b_cond[]
+ template should refer back to the instruction after the original
+ branch. We use target_section as Cortex-A8 erratum workaround stubs
+ are only generated when both source and target are in the same
+ section. */
+ points_to = stub_entry->target_section->output_section->vma
+ + stub_entry->target_section->output_offset
+ + stub_entry->source_value;
+
+ elf32_arm_final_link_relocate (elf32_arm_howto_from_type
+ (template_sequence[stub_reloc_idx[i]].r_type),
+ stub_bfd, info->output_bfd, stub_sec, stub_sec->contents, &rel,
+ points_to, info, stub_entry->target_section, "", STT_FUNC,
+ stub_entry->branch_type,
+ (struct elf_link_hash_entry *) stub_entry->h, &unresolved_reloc,
+ &error_message);
+ }
return TRUE;
#undef MAXRELOCS
@@ -5036,7 +5011,8 @@ cortex_a8_erratum_scan (bfd *input_bfd,
a8_fixes[num_a8_fixes].input_bfd = input_bfd;
a8_fixes[num_a8_fixes].section = section;
a8_fixes[num_a8_fixes].offset = i;
- a8_fixes[num_a8_fixes].addend = offset;
+ a8_fixes[num_a8_fixes].target_offset =
+ target - base_vma;
a8_fixes[num_a8_fixes].orig_insn = insn;
a8_fixes[num_a8_fixes].stub_name = stub_name;
a8_fixes[num_a8_fixes].stub_type = stub_type;
@@ -5609,9 +5585,9 @@ elf32_arm_size_stubs (bfd *output_bfd,
stub_entry->stub_offset = 0;
stub_entry->id_sec = link_sec;
stub_entry->stub_type = a8_fixes[i].stub_type;
+ stub_entry->source_value = a8_fixes[i].offset;
stub_entry->target_section = a8_fixes[i].section;
- stub_entry->target_value = a8_fixes[i].offset;
- stub_entry->target_addend = a8_fixes[i].addend;
+ stub_entry->target_value = a8_fixes[i].target_offset;
stub_entry->orig_insn = a8_fixes[i].orig_insn;
stub_entry->branch_type = a8_fixes[i].branch_type;
@@ -16064,7 +16040,7 @@ make_branch_to_a8_stub (struct bfd_hash_entry *gen_entry,
bfd_vma veneered_insn_loc, veneer_entry_loc;
bfd_signed_vma branch_offset;
bfd *abfd;
- unsigned int target;
+ unsigned int loc;
stub_entry = (struct elf32_arm_stub_hash_entry *) gen_entry;
data = (struct a8_branch_to_stub_data *) in_arg;
@@ -16075,9 +16051,11 @@ make_branch_to_a8_stub (struct bfd_hash_entry *gen_entry,
contents = data->contents;
+ /* We use target_section as Cortex-A8 erratum workaround stubs are only
+ generated when both source and target are in the same section. */
veneered_insn_loc = stub_entry->target_section->output_section->vma
+ stub_entry->target_section->output_offset
- + stub_entry->target_value;
+ + stub_entry->source_value;
veneer_entry_loc = stub_entry->stub_sec->output_section->vma
+ stub_entry->stub_sec->output_offset
@@ -16089,7 +16067,7 @@ make_branch_to_a8_stub (struct bfd_hash_entry *gen_entry,
branch_offset = veneer_entry_loc - veneered_insn_loc - 4;
abfd = stub_entry->target_section->owner;
- target = stub_entry->target_value;
+ loc = stub_entry->source_value;
/* We attempt to avoid this condition by setting stubs_always_after_branch
in elf32_arm_size_stubs if we've enabled the Cortex-A8 erratum workaround.
@@ -16150,8 +16128,8 @@ make_branch_to_a8_stub (struct bfd_hash_entry *gen_entry,
return FALSE;
}
- bfd_put_16 (abfd, (branch_insn >> 16) & 0xffff, &contents[target]);
- bfd_put_16 (abfd, branch_insn & 0xffff, &contents[target + 2]);
+ bfd_put_16 (abfd, (branch_insn >> 16) & 0xffff, &contents[loc]);
+ bfd_put_16 (abfd, branch_insn & 0xffff, &contents[loc + 2]);
return TRUE;
}
>
>
> The patch doesn't show any regression when running the binutils-gdb
> testsuite for the arm-none-eabi target.
>
> Any comments?
>
> Best regards,
>
> Thomas