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

[Daniel P . Berrangé]

On Sun, Apr 24, 2022 at 09:10:23PM +0200, LABBE Corentin wrote:
> 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.

If you don't want to/can't extend the crypto API, then at least
use the crypto API for all the pieces where it is possible to do
so. That way, we can clearly see where the gaps remain for this
device.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|