Message-ID: <CACPK8Xc+J0PbCdgheRxJbOVZ=OyyfsCA=cwkneMoboJLzC8TZQ@mail.gmail.com>
Date:   Wed, 22 Sep 2021 02:02:01 +0000
From:   Joel Stanley <joel@....id.au>
To:     Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
Cc:     Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Paul Mackerras <paulus@...ba.org>,
        Ash Logan <ash@...quark.com>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jonathan Neuschäfer <j.ne@...teo.net>
Subject: Re: [PATCH 1/4] crypto: nintendo-aes - add a new AES driver

On Tue, 21 Sept 2021 at 21:47, Emmanuel Gil Peyrot
<linkmauve@...kmauve.fr> wrote:
>
> This engine implements AES in CBC mode, using 128-bit keys only.  It is
> present on both the Wii and the Wii U, and is apparently identical in
> both consoles.
>
> The hardware is capable of firing an interrupt when the operation is
> done, but this driver currently uses a busy loop, I’m not too sure
> whether it would be preferable to switch, nor how to achieve that.
>
> It also supports a mode where no operation is done, and thus could be
> used as a DMA copy engine, but I don’t know how to expose that to the
> kernel or whether it would even be useful.
>
> In my testing, on a Wii U, this driver reaches 80.7 MiB/s, while the
> aes-generic driver only reaches 30.9 MiB/s, so it is a quite welcome
> speedup.
>
> This driver was written based on reversed documentation, see:
> https://wiibrew.org/wiki/Hardware/AES
>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
> Tested-by: Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>  # on Wii U
> ---
>  drivers/crypto/Kconfig        |  11 ++
>  drivers/crypto/Makefile       |   1 +
>  drivers/crypto/nintendo-aes.c | 273 ++++++++++++++++++++++++++++++++++
>  3 files changed, 285 insertions(+)
>  create mode 100644 drivers/crypto/nintendo-aes.c
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 9a4c275a1335..adc94ad7462d 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -871,4 +871,15 @@ config CRYPTO_DEV_SA2UL
>
>  source "drivers/crypto/keembay/Kconfig"
>
> +config CRYPTO_DEV_NINTENDO
> +       tristate "Support for the Nintendo Wii U AES engine"
> +       depends on WII || WIIU || COMPILE_TEST

This current seteup will allow the driver to be compile tested for
non-powerpc, which will fail on the dcbf instructions.

Perhaps use this instead:

       depends on WII || WIIU || (COMPILE_TEST && PPC)

> +       select CRYPTO_AES
> +       help
> +         Say 'Y' here to use the Nintendo Wii or Wii U on-board AES
> +         engine for the CryptoAPI AES algorithm.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called nintendo-aes.
> +
>  endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index fa22cb19e242..004dae7bbf39 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_CRYPTO_DEV_MARVELL) += marvell/
>  obj-$(CONFIG_CRYPTO_DEV_MXS_DCP) += mxs-dcp.o
>  obj-$(CONFIG_CRYPTO_DEV_NIAGARA2) += n2_crypto.o
>  n2_crypto-y := n2_core.o n2_asm.o
> +obj-$(CONFIG_CRYPTO_DEV_NINTENDO) += nintendo-aes.o
>  obj-$(CONFIG_CRYPTO_DEV_NX) += nx/
>  obj-$(CONFIG_CRYPTO_DEV_OMAP) += omap-crypto.o
>  obj-$(CONFIG_CRYPTO_DEV_OMAP_AES) += omap-aes-driver.o
> diff --git a/drivers/crypto/nintendo-aes.c b/drivers/crypto/nintendo-aes.c
> new file mode 100644
> index 000000000000..79ae77500999
> --- /dev/null
> +++ b/drivers/crypto/nintendo-aes.c
> @@ -0,0 +1,273 @@
> +/*
> + * Copyright (C) 2021 Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

The  kernel uses the SDPX header instead of pasting the text.

> +static int
> +do_crypt(const void *src, void *dst, u32 len, u32 flags)
> +{
> +       u32 blocks = ((len >> 4) - 1) & AES_CTRL_BLOCK;
> +       u32 status;
> +       u32 counter = OP_TIMEOUT;
> +       u32 i;
> +
> +       /* Flush out all of src, we can’t know whether any of it is in cache */
> +       for (i = 0; i < len; i += 32)
> +               __asm__("dcbf 0, %0" : : "r" (src + i));
> +       __asm__("sync" : : : "memory");

This could be flush_dcache_range, from asm/cacheflush.h

> +
> +       /* Set the addresses for DMA */
> +       iowrite32be(virt_to_phys((void *)src), base + AES_SRC);
> +       iowrite32be(virt_to_phys(dst), base + AES_DEST);
> +
> +       /* Start the operation */
> +       iowrite32be(flags | blocks, base + AES_CTRL);
> +
> +       /* TODO: figure out how to use interrupts here, this will probably
> +        * lower throughput but let the CPU do other things while the AES
> +        * engine is doing its work. */
> +       do {
> +               status = ioread32be(base + AES_CTRL);
> +               cpu_relax();
> +       } while ((status & AES_CTRL_EXEC) && --counter);

You could add a msleep in here?

Consider using readl_poll_timeout().

Cheers,

Joel

> +
> +       /* Do we ever get called with dst ≠ src?  If so we have to invalidate
> +        * dst in addition to the earlier flush of src. */
> +       if (unlikely(dst != src)) {
> +               for (i = 0; i < len; i += 32)
> +                       __asm__("dcbi 0, %0" : : "r" (dst + i));
> +               __asm__("sync" : : : "memory");
> +       }
> +
> +       return counter ? 0 : 1;
> +}

Powered by blists - more mailing lists