Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device

[Peter Maydell]

On Sun, 10 Apr 2022 at 20:12, Corentin Labbe <clabbe@baylibre.com> wrote:
>
> From: Corentin LABBE <clabbe@baylibre.com>
>
> The Allwinner A10 has a cryptographic offloader device which
> could be easily emulated.
> The emulated device is tested with Linux only as any of BSD does not
> support it.
>
> Signed-off-by: Corentin LABBE <clabbe@baylibre.com>

Hi; thanks for this patch, and sorry it's taken me a while to get
to reviewing it.

(Daniel, I cc'd you since this device model is making use of crypto
related APIs.)

Firstly, a note on patch structure. This is quite a large patch,
and I think it would be useful to split it at least into two parts:
 (1) add the new device model
 (2) change the allwinner SoC to create that new device

> diff --git a/docs/system/arm/cubieboard.rst b/docs/system/arm/cubieboard.rst
> index 344ff8cef9..7836643ba4 100644
> --- a/docs/system/arm/cubieboard.rst
> +++ b/docs/system/arm/cubieboard.rst
> @@ -14,3 +14,4 @@ Emulated devices:
>  - SDHCI
>  - USB controller
>  - SATA controller
> +- crypto
> diff --git a/docs/system/devices/allwinner-sun4i-ss.rst 
> b/docs/system/devices/allwinner-sun4i-ss.rst
> new file mode 100644
> index 0000000000..6e7d2142b5
> --- /dev/null
> +++ b/docs/system/devices/allwinner-sun4i-ss.rst
> @@ -0,0 +1,31 @@
> +Allwinner sun4i-ss
> +==================

If you create a new rst file in docs, you need to put it into the
manual by adding it to some table of contents. Otherwise sphinx
will complain when you build the documentation, and users won't be
able to find it. (If you pass 'configure' the --enable-docs option
that will check that you have everything installed to be able to
build the docs.)

There are two options here: you can have this document, and
add it to the toctree in docs/system/device-emulation.rst, and
make the "crypto" bullet point in cubieboard.rst be a hyperlink to
the device-emulation.rst file. Or you can compress the information
down and put it into orangepi.rst.

> +The ``sun4i-ss`` emulates the Allwinner cryptographic offloader
> +present on early Allwinner SoCs (A10, A10s, A13, A20, A33)
> +In qemu only A10 via the cubieboard machine is supported.
> +
> +The emulated hardware is capable of handling the following algorithms:
> +- SHA1 and MD5 hash algorithms
> +- AES/DES/DES3 in CBC/ECB
> +- PRNG
> +
> +The emulated hardware does not handle yet:
> +- CTS for AES
> +- CTR for AES/DES/DES3
> +- IRQ and DMA mode
> +Anyway the Linux driver also does not handle them yet.
> +
> +The emulation needs a real crypto backend, for the moment only gnutls/nettle 
> is supported.
> +So the device emulation needs qemu to be compiled with optionnal gnutls.

> diff --git a/hw/Kconfig b/hw/Kconfig
> index ad20cce0a9..43bd7fc14d 100644
> --- a/hw/Kconfig
> +++ b/hw/Kconfig
> @@ -6,6 +6,7 @@ source audio/Kconfig
>  source block/Kconfig
>  source char/Kconfig
>  source core/Kconfig
> +source crypto/Kconfig
>  source display/Kconfig
>  source dma/Kconfig
>  source gpio/Kconfig

I don't think we really need a new subdirectory of hw/
for a single device. If you can find two other devices that
already exist in QEMU that would also belong in hw/crypto/
then we can create it. Otherwise just put this device in
hw/misc.

> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 97f3b38019..fd8232b1d4 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -317,6 +317,7 @@ config ALLWINNER_A10
>      select AHCI
>      select ALLWINNER_A10_PIT
>      select ALLWINNER_A10_PIC
> +    select ALLWINNER_CRYPTO_SUN4I_SS
>      select ALLWINNER_EMAC
>      select SERIAL
>      select UNIMP
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index 05e84728cb..e9104ee028 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -23,6 +23,7 @@
>  #include "hw/misc/unimp.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/boards.h"
> +#include "hw/crypto/allwinner-sun4i-ss.h"
>  #include "hw/usb/hcd-ohci.h"
>
>  #define AW_A10_MMC0_BASE        0x01c0f000
> @@ -32,6 +33,7 @@
>  #define AW_A10_EMAC_BASE        0x01c0b000
>  #define AW_A10_EHCI_BASE        0x01c14000
>  #define AW_A10_OHCI_BASE        0x01c14400
> +#define AW_A10_CRYPTO_BASE      0x01c15000
>  #define AW_A10_SATA_BASE        0x01c18000
>  #define AW_A10_RTC_BASE         0x01c20d00
>
> @@ -48,6 +50,10 @@ static void aw_a10_init(Object *obj)
>
>      object_initialize_child(obj, "emac", &s->emac, TYPE_AW_EMAC);
>
> +#if defined CONFIG_NETTLE
> +    object_initialize_child(obj, "crypto", &s->crypto, TYPE_AW_SUN4I_SS);
> +#endif

Don't put this kind of ifdef into device/SoC code, please.
The device emulation needs to work regardless of what
the specific crypto backends that got compiled into QEMU are.

> +#include <nettle/aes.h>
> +#include <nettle/cbc.h>
> +#include <nettle/des.h>
> +#include <nettle/md5.h>
> +#include <nettle/sha1.h>

Similarly, don't directly include nettle headers. The device needs
to use the backend-agnostic headers from include/crypto. To the
extent that they aren't sufficient to implement this device we
can look at enhancing them.

> +static const VMStateDescription vmstate_allwinner_sun4i_ss = {
> +    .name = "allwinner-sun4i-ss",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(ctl, AwSun4iSSState),
> +        VMSTATE_UINT32(fcsr, AwSun4iSSState),
> +        VMSTATE_UINT32_ARRAY(iv, AwSun4iSSState, 5),
> +        VMSTATE_UINT32_ARRAY(key, AwSun4iSSState, 8),
> +        VMSTATE_UINT32_ARRAY(md, AwSun4iSSState, 5),

Shouldn't this also include rx, rxc, tx, txc ? Or do they
never contain live data at the point where we're migrating?

> +        VMSTATE_END_OF_LIST()
> +    }
> +};


> +/**
> + * Object model
> + * @{
> + */

We don't use these @{ ... @} markers in our doc comments,
so you can delete all of those.

I haven't looked at the rest of the code in detail, but I
skimmed over it and didn't see anything that looked wrong.

thanks
-- PMM