[PATCH] D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Many thanks for taking the time to review this and some of the other Arm/AArch64 specific stuff, it is much appreciated.

Posting into Phabricator to keep comments together.

>> +  if (Config->EMachine == EM_AARCH64 && Config->FixCortexA53Errata843419) {
>>  +    Script->assignAddresses();
>>  +    reportA53Errata843419Fixes();
>>  +  }
> 
> Why do you need assignAddresses()? This is just after a loop that stops
>  when address don't change.

At the moment, until https://reviews.llvm.org/D37944 Add support for AArch64 Range Thunks lands the assignAddresses won't be entered. Some merge work will be needed here as Thunks and Errata fixes can impact each other.

>> +// This file implements Section Patching for the purpose of working around
>>  +// errata in CPUs. The general principle is that an erratum sequence of one or
>>  +// more instructions is detected in the instruction stream, one of the
>>  +// instructions in the sequence is replaced with a branch to a patch sequence
>>  +// of replacement instructions. At the end of the replacement sequence the
>>  +// patch branches back to the instruction stream.
>>  +
>>  +// This technique is only suitable for fixing an erratum when:
>>  +// - There is a set of necessary conditions required to trigger the erratum that
>>  +// can be detected at static link time.
>>  +// - There is a set of replacement instructions that can be used to remove at
>>  +// least one of the necessary conditions that trigger the erratum.
>>  +// - We can overwrite an instruction in the erratum sequence with a branch to
>>  +// the replacement sequence.
>>  +// - We can place the replacement sequence within range of the branch.
> 
> Given where this is called you also need to be able to place the
>  replacement sequence after all other sections or you will need to
>  reevaluate addresses for thunks, no?

Yes. At time of writing AArch64 doesn't support thunks, my preference would be to land the range thunks support for AArch64 first (https://reviews.llvm.org/D37944) and then merge this in with some test cases involving range extension thunks.

>> +// - The implementation here only supports one patch, the AArch64 Cortex-53
>>  +// errata 843419 that affects r0p0, r0p1, r0p2 and r0p4 versions of the core.
>>  +// To keep the initial version simple there is no support for multiple
>>  +// architectures or selection of different patches.
> 
> BTW, is it public knowledge what uses these versions? I am not sure if
> that is something that is present in a few early dev boards or in most
> of the phones on the planet.

Unfortunately I don't have concrete information, we'd need to know for each phone what SoC it was using, and for each SoC which rev of the Cortex-A53 it was using. What I do know is that it was reproduced in real Android software on enough phones to make the NDK enable the gold/bfd equivalent option by default. My understanding is that Ubuntu for AArch64 and the Linaro aarch64-linux-gnu toolchains also enable it by default. My guess, given the low shelf life of mobiles, is the overall percentage of phones is low, but there are so many phones out there that it could still run into hundreds of thousands of phones still in use.

>> +// Load/store register (unsigned immediate)
>>  +// | size (2) 11 | 1 V 01 | opc (2) | imm12 | Rn (5) | Rt (5) |
>>  +static bool isLoadStoreRegisterUnsigned(uint32_t Instr) {
>>  +  return (Instr & 0x3b000000) == 0x39000000;
>>  +}



> A crazy idea for another day: can all these functions be auto generated
>  from the td files that llvm-mc uses?

I did think a bit about that. In theory some of it, but in general the information in mc doesn't give you the information in the right form or in a stable enough form that you could interrogate for details such as does this instruction read or write this register? I suspect that quite a bit more information that wouldn't be needed for compilation/disassembly would need to be added to the .td files to make it work.

>> +// Note that this function refers to v8.0 only and does not include the
>>  +// additional load and store instructions added for in later revisions of
>>  +// the architecture such as the Atomic memory operations introduced
>>  +// in v8.1.
> 
> Because the errata is fixed on all implementations of v8.1?

In effect yes. The errata is specific to the cortex-a53 CPU, and all cortex-a53s are v8.0.

>> +static void report843419Fix(uint64_t AdrpAddr) {
>>  +  if (!Config->Verbose)
>>  +    return;
>>  +  message("detected cortex-a53-843419 erratum sequence starting at " +
>>  +          utohexstr(AdrpAddr) + " in unpatched output.");
>>  +}
> 
> This one is actually simpler without the early return.

Ok I'll fix.

>> +  bool OptionalAllowed = Limit - Off > 12;
> 
> Move bool after the if since it is not used in it.

Ok I'll fix.

> LGTM with the above nits fixed (mostly comment updates).
> 
> Cheers,
> Rafael

I think it will be best if I get the range-extension thunks in first, and then merge this one in so that the interactions can be tested.


https://reviews.llvm.org/D36742