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

[LABBE Corentin]

Le Thu, Apr 21, 2022 at 01:38:00PM +0100, Peter Maydell a écrit :
> 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

Hello

I will do it for next iteration

> 
> > 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.

I plan to add at least one other hw/crypto device (allwinner H3 sun8i-ce).
I have another one already ready (rockchip rk3288) but I delay it since there 
are no related SoC in qemu yet.

> 
> > 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.

Problem is that current qemu crypto backends do not have necessary functions 
needed by this driver, I need to do basic MD5 transform with custom IV.
I will check if it can be added in qemu crypto API.

> 
> > +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?

I will fix this.

> 
> > +        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

Thanks for your review.

Regards