Re: [PATCH 4/7] crypto: sun4i-ss: handle BigEndian for cipher

[Herbert Xu]

On Thu, Sep 17, 2020 at 06:35:55PM +0000, Corentin Labbe wrote:
> Ciphers produce invalid results on BE.
> Key and IV need to be written in LE.
> Furthermore, the non-optimized function is too complicated to convert,
> let's simply fallback on BE for the moment.
>
> Fixes: 6298e948215f2 ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Corentin Labbe <cla...@baylibre.com>
> ---
> .../crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)

Does the BE failure get caught by the selftest?

If so please just leave it enabled so that it can be fixed properly.

Thanks,
--
Email: Herbert Xu <her...@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[Herbert Xu]

On Fri, Sep 18, 2020 at 10:06:58AM +0200, LABBE Corentin wrote:
>
> But I think only me will see it and since I already have this on my TODO list, I dont see any interest to leave it failing.
> Furthermore, having a clean BE boot will permit to enable BE boots for thoses SoCs on kernelCI.

I'll happily accept patches that fix the actual bug but not ones
just papering over it.

[Arnd Bergmann]

On Sun, Sep 20, 2020 at 8:37 PM Corentin Labbe <cla...@baylibre.com> wrote:
>
> Ciphers produce invalid results on BE.
> Key and IV need to be written in LE.
>

> Fixes: 6298e948215f2 ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Corentin Labbe <cla...@baylibre.com>
> ---

> drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> index c6c25204780d..a05889745097 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> @@ -52,13 +52,13 @@ static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
>
> spin_lock_irqsave(&ss->slock, flags);
>
> - for (i = 0; i < op->keylen; i += 4)
> - writel(*(op->key + i / 4), ss->base + SS_KEY0 + i);
> + for (i = 0; i < op->keylen / 4; i++)
> + writel(cpu_to_le32(op->key[i]), ss->base + SS_KEY0 + i * 4);

I suspect what you actually want here is writesl() in place of the
loop. This skips the byteswap on big-endian, rather than swapping
each word twice.

The point is that this register seems to act as a FIFO for a byte-stream
rather than a 32-bit fixed-endian register.

Arnd

[Arnd Bergmann]

On Wed, Sep 23, 2020 at 8:08 PM LABBE Corentin <cla...@baylibre.com> wrote:

> Thanks, using writesl() fixes the warning, but I need to keep the loop
> since the register is different each time.

Ah, I see. I thought we had an interface for that as well, but I can't
find it now. I see memcpy_toio32() in one driver, but that implementation
appears to be wrong here (and probably also wrong for the machine
it was meant for)

There is the regular memcpy_toio(), but on big-endian Arm that
turns into a per-byte copy, which might either not work on your
hardware or be too slow.

There is also __iowrite32_copy(), which is not what I had remembered
but does seem to do what you want here.

> Or does it is better to use directly __raw_writel() ?

__raw_writel() is not very portable, so I would avoid that in normal
device drivers even when you only run them on specific hardware.

Arnd