[PATCH] D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417

Mon Sep 9 09:33:50 PDT 2019

peter.smith marked 27 inline comments as done.
peter.smith added a comment.

In D67284#1663011 <https://reviews.llvm.org/D67284#1663011>, @ruiu wrote:

> I'm halfway through reviewing it, and it looks like a straightforward implementation of a mitigation. Can I ask a (perhaps noob) question? If a CPU can be locked up by an instruction sequence that triggers the bug, and if the CPU is used by Android, does that mean a user application can lock up the entire system?

Yes it is possible for a user application to lock the entire system. From the erratum description: " If the deadlock condition occurs, it can only be interrupted by pulling the RESET pin on the processor." There are several more run-time conditions that make this rare, and it only occurs on models with a 32k instruction cache.

Thanks very much for the comments, I will post a new patch shortly that I hope will address the comments. I think the AArch64ErrataFix.cpp will need a patch after this as some of the refactoring cleanups could also be applied there. I'll do that when it is clear what needs to be done.

================
Comment at: ELF/ARMErrataFix.cpp:42
+// - There is a 32-bit Thumb-2 branch instruction with an address of the form
+//   xxxxxxFFE. The first 2 bytes of the instruction are in 4KiB region 1, the
+//   second 2 bytes are in region 2.
----------------
ruiu wrote:
> MaskRay wrote:
> > What are "region 1" and "region 2"?
> I guess that means first page and second page?
Memory region is the word used in the official erratum description and the Architecture reference manual. It is closely related to a page so in this case it can be thought of as a page. I think memory region is used in the spec as memory regions can also be used in a non-paged context. I'll update the description to make it clear what region 1 and 2 are.

================
Comment at: ELF/ARMErrataFix.cpp:88
+// Return true if the half-word is the first half of a 32-bit instruction.
+// armv7ar architecture reference manual 32-bit instruction encoding
+// First halfword of a 32-bit instruction is of form:
----------------
MaskRay wrote:
> 32-bit Thumb instruction encoding? (The heading as used in the manual)
Yes, I'll update the comment to improve the reference. 

================
Comment at: ELF/ARMErrataFix.cpp:91
+// | 1 1 1 | op1 (2) | op2 (7) | x (4) |
+// With op1 == 0b00
+static bool is32bitInstruction(uint16_t hw) {
----------------
MaskRay wrote:
> op1 != 0b00 ?
> 
> op1 == 0b00 encodes a 16-bit Thumb instruction.
Yes, I forgot to finish the sentence used in the manual. I'll fix up the comment.

================
Comment at: ELF/ARMErrataFix.cpp:181
+// the source.
+static bool branchToFirstRegion(const InputSection *isec, uint64_t off,
+                                uint32_t instr) {
----------------
MaskRay wrote:
> > branchToFirstRegion
> 
> Is "first region" the term used to refer to `destAddr < sourceAddr && (destAddr & 0xfffff000) == (sourceAddr & 0xfffff000);`
Yes it is, I'll update the function name to make it more explicit. I also worked out that I don't need the destAddr < sourceAddr part.

================
Comment at: ELF/ARMErrataFix.cpp:269
+  }
+  off += 0x1000;
+  return scanRes;
----------------
MaskRay wrote:
> Say `off % 0x1000 = 0xffc` before the assignment. The erratum sequence in the next page starts at 0xffa. This increment will skip that erratum sequence.
> 
> Is this a possible scenario?
Yes I think there might be a case that gets recognized as an erratum where there is something like:
```
0x0ffc nop.w
label:
0x1000 b.w label
```
I will add a case to make sure the 0xffc is handled in a similar way to 0xffe and will add to the nopatch test case.

================
Comment at: ELF/ARMErrataFix.cpp:286
+  auto isArmMapSymbol = [](const Symbol *b) {
+    return b->getName() == "$a" || b->getName().startswith("$a.");
+  };
----------------
MaskRay wrote:
> In include/llvm/Object/ELFObjectFile.h and llvm-objdump, we just use `startswith("$a")`. Is there a reason to check `"$a."`? Below, you just use `return ms->getName().startswith("$t");`
Yes, $afoo without the . is not a mapping symbol. I'm thinking that it might be best to replace the map from InputSection -> vector<Symbol*> to InputSection -> std::pair<uint64_t offset, bool isThumb>, this would also work for AArch64.  

================
Comment at: ELF/ARMErrataFix.cpp:297
+  for (InputFile *file : objectFiles) {
+    auto *f = cast<ObjFile<ELF32LE>>(file);
+    for (Symbol *b : f->getLocalSymbols()) {
----------------
MaskRay wrote:
> Should there be a check to reject ELF32BE beforehand?
> It can be placed in Driver.cpp:checkOptions.
LLD doesn't support ELF32BE. I'd have thought it would give an error, but without passing an emulation, it seems like I can get LLD will accept a big-endian ELF file without error, I'll need to write a follow up patch to give an error.

As an aside:
Big Endian is strange on AArch64 and even stranger in ARM. In general it only tends to get used in networking so I've not seen any requests to implement it.
- AArch64 BE: ELF is BE, instructions are LE, Data is BE
- ARM BE : ELF is BE, instructions in relocatable objects are BE, Data is BE. Older ARM architectures like v4, v5 and v6 executable/dso instructions have BE instructions. All newer architectures have LE instructions and the linker needs to do the byte swaps.

================
Comment at: ELF/ARMErrataFix.cpp:351
+      while (patchIt != patchEnd) {
+        if ((*patchIt)->getBranchAddr() - outSecAddr >= prevIsecLimit)
+          break;
----------------
MaskRay wrote:
> Is it guaranteed that getBranchAddr() is monotonically increasing?
Yes. The getBranchAddr() is essentially the address in the InputSection that has the patch applied to it. Within an InputSectionDescriptions the InputSections->outSecOff monotonically increase, and as the patches are added to the end of the list, the getBranchAddr() monotonically increases.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67284/new/

https://reviews.llvm.org/D67284