Le Thu, Apr 21, 2022 at 01:38:00PM +0100, Peter Maydell a écrit :
> On Sun, 10 Apr 2022 at 20:12, Corentin Labbe <cla...@baylibre.com> wrote:
> >
> > From: Corentin LABBE <cla...@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 <cla...@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
