Messages in this thread | | | Date | Mon, 8 May 2023 11:18:06 -0700 | Subject | Re: [PATCH v6 11/12] crypto: x86/aes-kl - Support AES algorithm using Key Locker instructions | From | "Chang S. Bae" <> |
| |
On 5/5/2023 5:01 PM, Eric Biggers wrote: > On Mon, Apr 10, 2023 at 03:59:35PM -0700, Chang S. Bae wrote: >> [PATCH v6 11/12] crypto: x86/aes-kl - Support AES algorithm using Key Locker instructions > > Thanks for dropping the unnecessary modes of operation (CBC, ECB, CTR). It > simplified the patchset quite a bit!
Yeah. But, there are more things to go away here as you pointed out here.
I thought some generic establishment (patch10) then introduce some mode-specific code (patch11). Considerably, this incremental change was expected to help reviewers.
Now I realize this introduces dead code on its hindsight. And this approach seems not helping that much.
> Now that only AES-XTS is included, can you please also merge this patch with the > following patch? As-is, this patch is misleading since it doesn't actually add > "support" for anything at all. It actually just adds an unfinished AES-XTS > implementation, which patch 12 then finishes. I assume that the current > patchset organization is left over from when you were trying to support multiple > modes of operation. IMO, it would be much easier to review if patches 11-12 > were merged into one patch that adds the new AES-XTS implementation.
Yes, I agree to merge them.
>> For disk encryption, storage bandwidth may be the bottleneck >> before encryption bandwidth, but the potential performance difference is >> why AES-KL is advertised as a distinct cipher in /proc/crypto rather than >> the kernel transparently replacing AES-NI usage with AES-KL. > > This does not correctly describe what is going on. Actually, this patchset > registers the AES-KL XTS algorithm with the usual name "xts(aes)". So, it can > potentially be used by any AES-XTS user. It seems that you're actually relying > on the algorithm priorities to prioritize AES-NI, as you've assigned priority > 200 to AES-KL, whereas AES-NI has priority 401. Is that what you intend, and if > so can you please update your explanation to properly explain this?
I think AES-KL could be a drop-in replacement for AES-NI IF it performs well -- something on par with AES-NI or better, AND it also supports all the key sizes. But, it can't be the default because that's not the case (at least for now).
> The alternative would be to use a unique algorithm name, such as > "keylocker-xts(aes)". I'm not sure that would be better, given that the > algorithms are compatible. However, that actually would seem to match the > explanation you gave more closely, so perhaps that's what you actually intended?
I think those AES implementations are functionally the same to end users. The key envelopment is not something user-visible to my understanding. So, I thought that same name makes sense.
Now looking at the changelog, this text in the 'performance' section appears to be relevant:
> the potential performance difference is why AES-KL is advertised as > a distinct cipher in /proc/crypto rather than the kernel > transparently replacing AES-NI usage with AES-KL.
But, this does not seem to be clear enough. Maybe, this exposition story can go under a new section. The changelog is already tl;dr...
> I strongly recommend skipping the 32-bit support, as it's unlikely to be worth > the effort. > > And actually, aeskl-intel_glue.c only registers the algorithm for 64-bit > anyway... So the 32-bit code paths are untested dead code.
Yeah, will do. Also, I'd make the module available only with X86-64. Then, a bit of comments for the reason should come along.
>> +static inline int aeskl_enc(const void *ctx, u8 *out, const u8 *in) >> +{ >> + if (unlikely(keylength(ctx) == AES_KEYSIZE_192)) >> + return -EINVAL; >> + else if (!valid_keylocker()) >> + return -ENODEV; >> + else if (_aeskl_enc(ctx, out, in)) >> + return -EINVAL; >> + else >> + return 0; >> +} >> + >> +static inline int aeskl_dec(const void *ctx, u8 *out, const u8 *in) >> +{ >> + if (unlikely(keylength(ctx) == AES_KEYSIZE_192)) >> + return -EINVAL; >> + else if (!valid_keylocker()) >> + return -ENODEV; >> + else if (_aeskl_dec(ctx, out, in)) >> + return -EINVAL; >> + else >> + return 0; >> +} > > I don't think the above two functions should exist. keylength() and > valid_keylocker() should be checked before calling xts_crypt_common(), and the > assembly functions should just return the correct error code (-EINVAL, > apparently) instead of an unspecified nonzero value. That would leave nothing > for a wrapper function to do. > > Note: if you take this suggestion, the assembly functions would need to use > SYM_TYPED_FUNC_START instead of SYM_FUNC_START.
I thought this is something benign to stay here. But, yes, I agree that it is better to simplify the code.
Thanks, Chang
| |