Re: [PATCH v2 0/6] crypto: aes - allow generic AES to be omitted

[Eric Biggers <ebiggers3@gmail.com>]

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.

Eric