> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Sent: Tuesday, July 30, 2019 10:24 AM
> To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
> Cc: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>; Pascal van Leeuwen
> <pascalvanl@xxxxxxxxx>; linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx
> Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
>
> On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > > From: linux-crypto-owner@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On
> Behalf Of Antoine Tenart
> > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > > >
> > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx
> *ctx, u8 *iv,
> > > > > > u32 length)
> > > > > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > > >
> > > > > I think it's better for maintenance and readability to have something
> > > > > like:
> > > > >
> > > > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > > > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > > >
> > > > Not really. I *really* want to execute that for any mode other than ECB,
> > > > ECB being the *only* mode that does not require an IV (which I know
> > > > for a fact, being the architect and all :-).
> > > > And I don't believe a long list of modes that *do* require an IV would
> > > > be more readable or easy to maintain than this single compare ...
> > >
> > > That's where I disagree as you need extra knowledge to be aware of this.
> > > Being explicit removes any possible question one may ask. But that's a
> > > small point really :)
> > >
> > Well, while we're disagreeing ... I disagree with your assertion that you
> > would need more knowledge to know which modes do NOT need an IV
> > than to know which modes DO need an IV. There's really no fundamental
> > difference, it's two sides of the exact same coin ... you need that
> > knowledge either way.
>
> The point is if you look for occurrences of, let's say
> CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
> easier if you have direct comparisons.
>
Not a good enough reason considering the downsides of having less
performant code that is more difficult to maintain, IMHO (you will
have to keep adding modes as we have plenty more and may add even
more to the hardware later - and make sure the list is and stays
complete going forward).
So how often do you really need to do that search?
Speaking as someone who has been adding new modes fairly recently
but never had any need for that ...
> > 1) This code is executed for each individual cipher call, i.e. it's in the
> > critical path. Having just 1 compare-and-branch there is better for
> > performance than having many.
>
> Not sure about what the impact really is.
>
Well, the driver is not exactly extracting maximum performance from
our engine for small blocks at the moment, so I would say any cycle
gained matters. Software overhead is the Achilles heel of hardware
acceleration. And if you don't accelerate, your hardware is defacto
useless. So anything in the critical path should be carefully
considered. (which is also why I object so strongly to all these API
corner cases we're forced to support, but now I digress ...)
You can debate about significance, but at some point it all adds up.
Better to not add the cycles - if it's no effort at all! - then to
have to optimize them away later. Speaking as someone who has spent
a major part of his professional life squeezing out those cycles to
meet very real and hard performance requirements.
> > 2) Generally, all else being equal, having less code is easier to maintain
> > than having more code.
>
> That really depends, having readable code is easier to maintain :)
>
But how would it be more readable? "Do this for any mode other than
ECB" seems pretty clear and readable to me? And is exactly what should
happen here. Also, less code of the same complexity level (i.e. the
same simple compares!) is still more readable than more code.
> > 4) If there is anything unclear about an otherwise fine code construct,
> > then you clarify it by adding a comment, not by rewriting it to be inefficient
> > and redundant ;-)
>
> Fair point.
>
> Thanks,
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
