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

[Rafael Avila de Espindola via llvm-commits]

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:


> +  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.

> +// 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?


> +// - 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.

> +// 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?

> +// 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?

> +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.


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


LGTM with the above nits fixed (mostly comment updates).

Cheers,
Rafael