On 19 June 2017 at 05:15, Eric Biggers <ebiggers3@xxxxxxxxx> wrote: > Hi Ard, > > On Fri, Jun 16, 2017 at 01:17:43PM +0200, Ard Biesheuvel wrote: >> The generic AES driver uses 16 lookup tables of 1 KB each, and has >> encryption and decryption routines that are fully unrolled. Given how >> the dependencies between this code and other drivers are declared in >> Kconfig files, this code is always pulled into the core kernel, even >> if it is usually superseded at runtime by accelerated drivers that >> exist for many architectures. >> >> This leaves us with 25 KB of dead code in the kernel, which is negligible >> in typical environments, but which is actually a big deal for the IoT >> domain, where every kilobyte counts. >> >> Also, the scalar, table based AES routines that exist for ARM, arm64, i586 >> and x86_64 share the lookup tables with AES generic, and may be invoked >> occasionally when the time-invariant AES-NI or other special instruction >> drivers are called in interrupt context, at which time the SIMD register >> file cannot be used. Pulling 16 KB of code and 9 KB of instructions into >> the L1s (and evicting what was already there) when a softirq happens to >> be handled in the context of an interrupt taken from kernel mode (which >> means no SIMD on x86) is also something that we may like to avoid, by >> falling back to a much smaller and moderately less performant driver. >> (Note that arm64 will be updated shortly to supply fallbacks for all >> SIMD based AES implementations, which will be based on the core routines >> [if they are accepted].) >> >> For the reasons above, this series refactors the way the various AES >> implementations are wired up, to allow the generic version in >> crypto/aes_generic.c to be omitted from the build entirely. >> > > This looks better now. I think the help text and prompts could still use some > improvement. For the prompts, on x86_64 now I see: > > -*- AES cipher algorithms > [*] Fixed time AES cipher > [*] AES cipher algorithms (x86_64) > [*] AES cipher algorithms (AES-NI) > > The first is actually the generic table-based implementation now, and it can be > deselected if the generic fixed-time implementation is selected and the x86_64 > table-based implementation is deselected. How about making the prompts be: > > AES cipher algorithm (generic, table-based) > AES cipher algorithm (generic, time-invariant) > AES cipher algorithm (x86_64, table-based) > AES cipher algorithm (AES-NI) > > For the help text, removing the Wikipedia-style boilerplate is good, but IMO the > help text should at least spell out "AES (Advanced Encryption Standard)". It's > "obvious" to people familiar with crypto algorithms, but I always find it > annoying when Kconfig options elsewhere in the kernel use unfamiliar acronyms > which the developers didn't bother to spell out because it was "obvious" to > them. > > The help text could also give a bit more information to help people decide which > options to enable. For example, the help for CRYPTO_AES_X86_64 could say that > it's only useful on older processors that do not have AES-NI instructions, and > that the AES-NI implementation, if enabled, will take priority on newer > processors. Similarly for the generic implementations, though note that the > user may still be required to enable at least one of them as a fallback. Also, > the AES-NI and ARMv8-CE implementations are not only time-invariant but also the > fastest --- and therefore strongly recommended to enable. > Thanks Eric, all good feedback. I will incorporate it into the next respin. -- Ard.
