All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
@ 2022-04-10 19:12 Corentin Labbe
  2022-04-21 12:38 ` Peter Maydell
  0 siblings, 1 reply; 9+ messages in thread
From: Corentin Labbe @ 2022-04-10 19:12 UTC (permalink / raw)
  To: peter.maydell, b.galvani, qemu-devel, qemu-arm; +Cc: Corentin LABBE

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>
---
 MAINTAINERS                                |   8 +
 docs/system/arm/cubieboard.rst             |   1 +
 docs/system/devices/allwinner-sun4i-ss.rst |  31 +
 hw/Kconfig                                 |   1 +
 hw/arm/Kconfig                             |   1 +
 hw/arm/allwinner-a10.c                     |  11 +
 hw/crypto/Kconfig                          |   2 +
 hw/crypto/allwinner-sun4i-ss.c             | 688 +++++++++++++++++++++
 hw/crypto/meson.build                      |   3 +
 hw/crypto/trace-events                     |   3 +
 hw/crypto/trace.h                          |   1 +
 hw/meson.build                             |   1 +
 include/hw/arm/allwinner-a10.h             |   2 +
 include/hw/crypto/allwinner-sun4i-ss.h     |  72 +++
 meson.build                                |   1 +
 15 files changed, 826 insertions(+)
 create mode 100644 docs/system/devices/allwinner-sun4i-ss.rst
 create mode 100644 hw/crypto/Kconfig
 create mode 100644 hw/crypto/allwinner-sun4i-ss.c
 create mode 100644 hw/crypto/meson.build
 create mode 100644 hw/crypto/trace-events
 create mode 100644 hw/crypto/trace.h
 create mode 100644 include/hw/crypto/allwinner-sun4i-ss.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ad2451e03..f07d9713af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -583,6 +583,14 @@ F: include/hw/*/allwinner-h3*
 F: hw/arm/orangepi.c
 F: docs/system/arm/orangepi.rst
 
+Allwinner sun4i-ss cryptographic offloader
+M: Corentin LABBE <clabbe@baylibre.com>
+L: qemu-arm@nongnu.org
+S: Maintained
+F: hw/crypto/allwinner-*
+F: include/hw/crypto/allwinner-*
+F: docs/system/arm/orangepi.rst
+
 ARM PrimeCell and CMSDK devices
 M: Peter Maydell <peter.maydell@linaro.org>
 L: qemu-arm@nongnu.org
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
+==================
+
+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.
+
+Emulation limit
+---------------
+
+PRNG:
+The PRNG is not really emulated as its internal is not known.
+It is replaced by the g_random_int function from glib.
+
+SPEED:
+The emulated hardware is ""faster"" than real hw as any write
+already give results immediatly instead of a few delay in real HW.
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
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
+
     object_initialize_child(obj, "sata", &s->sata, TYPE_ALLWINNER_AHCI);
 
     if (machine_usb(current_machine)) {
@@ -115,6 +121,11 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
     sysbus_connect_irq(sysbusdev, 0, qdev_get_gpio_in(dev, 55));
 
+#if defined CONFIG_NETTLE
+    sysbus_realize(SYS_BUS_DEVICE(&s->crypto), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->crypto), 0, AW_A10_CRYPTO_BASE);
+#endif
+
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->sata), errp)) {
         return;
     }
diff --git a/hw/crypto/Kconfig b/hw/crypto/Kconfig
new file mode 100644
index 0000000000..5ecfe44b10
--- /dev/null
+++ b/hw/crypto/Kconfig
@@ -0,0 +1,2 @@
+config ALLWINNER_CRYPTO_SUN4I_SS
+    bool
diff --git a/hw/crypto/allwinner-sun4i-ss.c b/hw/crypto/allwinner-sun4i-ss.c
new file mode 100644
index 0000000000..e7d85646e6
--- /dev/null
+++ b/hw/crypto/allwinner-sun4i-ss.c
@@ -0,0 +1,688 @@
+/*
+ * Allwinner sun4i-ss cryptographic offloader emulation
+ *
+ * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "hw/irq.h"
+#include "qemu/log.h"
+#include "trace.h"
+#include "qemu/module.h"
+#include "exec/cpu-common.h"
+#include "hw/crypto/allwinner-sun4i-ss.h"
+
+#include <nettle/aes.h>
+#include <nettle/cbc.h>
+#include <nettle/des.h>
+#include <nettle/md5.h>
+#include <nettle/sha1.h>
+
+#define SS_IV_ARBITRARY (1 << 14)
+
+/* SS operation mode - bits 12-13 */
+#define SS_ECB (0 << 12)
+#define SS_CBC (1 << 12)
+
+/* Key size for AES - bits 8-9 */
+#define SS_AES_128BITS (0 << 8)
+#define SS_AES_192BITS (1 << 8)
+#define SS_AES_256BITS (2 << 8)
+
+/* Operation direction - bit 7 */
+#define SS_ENCRYPTION  (0 << 7)
+#define SS_DECRYPTION  (1 << 7)
+
+/* SS Method - bits 4-6 */
+#define SS_OP_AES      (0 << 4)
+#define SS_OP_DES      (1 << 4)
+#define SS_OP_3DES     (2 << 4)
+#define SS_OP_SHA1     (3 << 4)
+#define SS_OP_MD5      (4 << 4)
+#define SS_OP_PRNG     (5 << 4)
+
+/* Data end bit - bit 2 */
+#define SS_DATA_END (1 << 2)
+
+/* SS Enable bit - bit 0 */
+#define SS_ENABLED (1 << 0)
+
+enum {
+    REG_CTL        = 0x0000,
+    REG_KEY_0      = 0x0004,
+    REG_KEY_1      = 0x0008,
+    REG_KEY_2      = 0x000c,
+    REG_KEY_3      = 0x0010,
+    REG_KEY_4      = 0x0014,
+    REG_KEY_5      = 0x0018,
+    REG_KEY_6      = 0x001c,
+    REG_KEY_7      = 0x0020,
+    REG_IV_0       = 0x0024,
+    REG_IV_1       = 0x0028,
+    REG_IV_2       = 0x002c,
+    REG_IV_3       = 0x0030,
+    REG_IV_4       = 0x0034,
+    REG_FCSR       = 0x0044,
+    REG_ICSR       = 0x0048,
+    REG_MD0        = 0x004c,
+    REG_MD1        = 0x0050,
+    REG_MD2        = 0x0054,
+    REG_MD3        = 0x0058,
+    REG_MD4        = 0x005c,
+    REG_RXFIFO     = 0x0200,
+    REG_TXFIFO     = 0x0204,
+};
+
+static void allwinner_sun4i_ss_try_work(AwSun4iSSState *s);
+
+/* return number of possible operation wih block size=bs */
+static unsigned int can_work(AwSun4iSSState *s, unsigned int bs)
+{
+    unsigned int avail_rx = s->rxc / (bs / 4);
+    unsigned int free_space_tx = (SS_TX_MAX - s->txc) / (bs / 4);
+
+    if (avail_rx > free_space_tx) {
+        return free_space_tx;
+    }
+    return avail_rx;
+}
+
+/*
+ * Without any knowledge on the PRNG, the only solution is
+ * to emulate it via g_random_int()
+ */
+static void do_prng(AwSun4iSSState *s)
+{
+    unsigned int size = 20;
+    int i;
+
+    for (i = 0; i < size / 4; i++) {
+        s->tx[i] = g_random_int();
+    }
+    s->txc += size / 4;
+}
+
+/* remove pop u32 words from RX */
+static void rx_pop(AwSun4iSSState *s, unsigned int pop)
+{
+    uint32_t *rx = (uint32_t *)s->rx;
+    int i;
+
+    for (i = 0; i < s->rxc; i++) {
+        rx[i] = rx[i + pop];
+    }
+}
+
+static void do_md5(AwSun4iSSState *s)
+{
+    unsigned int size = MD5_BLOCK_SIZE;
+    unsigned char *src = s->rx;
+
+    nettle_md5_compress(s->md, src);
+
+    s->rxc -= size / 4;
+    if (s->rxc > 0) {
+        rx_pop(s, size / 4);
+        allwinner_sun4i_ss_try_work(s);
+    }
+}
+
+static void do_sha1(AwSun4iSSState *s)
+{
+    unsigned int size = SHA1_BLOCK_SIZE;
+    unsigned char *src = s->rx;
+
+    nettle_sha1_compress(s->md, src);
+
+    s->rxc -= size / 4;
+    if (s->rxc > 0) {
+        rx_pop(s, size / 4);
+        allwinner_sun4i_ss_try_work(s);
+    }
+}
+
+static void do_des(AwSun4iSSState *s)
+{
+    struct CBC_CTX(struct des_ctx, DES_BLOCK_SIZE) cdes;
+    struct des_ctx des;
+    unsigned char *src = s->rx;
+    unsigned char *dst = s->tx + s->txc * 4;
+    unsigned char *key = (unsigned char *)s->key;
+    unsigned int size = DES_BLOCK_SIZE;
+    unsigned char biv[DES_BLOCK_SIZE];
+
+    if (s->ctl & SS_DECRYPTION) {
+        memcpy(biv, src, DES_BLOCK_SIZE);
+    }
+
+    if (s->ctl & SS_CBC) {
+        CBC_SET_IV(&cdes, s->iv);
+        if (s->ctl & SS_DECRYPTION) {
+            des_set_key(&cdes.ctx, key);
+            CBC_DECRYPT(&cdes, des_decrypt, size, dst, src);
+        } else {
+            des_set_key(&cdes.ctx, key);
+            CBC_ENCRYPT(&cdes, des_encrypt, size, dst, src);
+        }
+        /* Copy next IV in registers */
+        if (s->ctl & SS_DECRYPTION) {
+            memcpy(s->iv, biv, DES_BLOCK_SIZE);
+        } else {
+            memcpy(s->iv, dst, DES_BLOCK_SIZE);
+        }
+    } else {
+        if (s->ctl & SS_DECRYPTION) {
+            des_set_key(&des, key);
+            des_decrypt(&des, size, dst, src);
+        } else {
+                des_set_key(&des, key);
+                des_encrypt(&des, size, dst, src);
+            }
+    }
+    s->txc += size / 4;
+    s->rxc -= size / 4;
+
+    if (s->rxc > 0) {
+        rx_pop(s, size / 4);
+        allwinner_sun4i_ss_try_work(s);
+    }
+}
+
+static void do_des3(AwSun4iSSState *s)
+{
+    struct CBC_CTX(struct des3_ctx, DES3_BLOCK_SIZE) cdes3;
+    struct des3_ctx des3;
+    unsigned char *src = s->rx;
+    unsigned char *dst = s->tx + s->txc * 4;
+    unsigned char *key = (unsigned char *)s->key;
+    unsigned int size = DES3_BLOCK_SIZE;
+    unsigned char biv[DES3_BLOCK_SIZE];
+
+    if (s->ctl & SS_DECRYPTION) {
+        memcpy(biv, src, DES3_BLOCK_SIZE);
+    }
+
+    if (s->ctl & SS_CBC) {
+        CBC_SET_IV(&cdes3, s->iv);
+        if (s->ctl & SS_DECRYPTION) {
+            des3_set_key(&cdes3.ctx, key);
+            CBC_DECRYPT(&cdes3, des3_decrypt, size, dst, src);
+        } else {
+            des3_set_key(&cdes3.ctx, key);
+            CBC_ENCRYPT(&cdes3, des3_encrypt, size, dst, src);
+        }
+        /* Copy next IV in registers */
+        if (s->ctl & SS_DECRYPTION) {
+            memcpy(s->iv, biv, DES3_BLOCK_SIZE);
+        } else {
+            memcpy(s->iv, dst, DES3_BLOCK_SIZE);
+        }
+    } else {
+        if (s->ctl & SS_DECRYPTION) {
+            des3_set_key(&des3, key);
+            des3_decrypt(&des3, size, dst, src);
+        } else {
+            des3_set_key(&des3, key);
+            des3_encrypt(&des3, size, dst, src);
+        }
+    }
+    s->txc += size / 4;
+    s->rxc -= size / 4;
+
+    if (s->rxc > 0) {
+        rx_pop(s, size / 4);
+        allwinner_sun4i_ss_try_work(s);
+    }
+}
+
+static void do_aes(AwSun4iSSState *s)
+{
+    struct CBC_CTX(struct aes128_ctx, AES_BLOCK_SIZE) aes128;
+    struct CBC_CTX(struct aes192_ctx, AES_BLOCK_SIZE) aes192;
+    struct CBC_CTX(struct aes256_ctx, AES_BLOCK_SIZE) aes256;
+    struct aes128_ctx ecb128;
+    struct aes192_ctx ecb192;
+    struct aes256_ctx ecb256;
+    unsigned char *src = s->rx;
+    unsigned char *dst = s->tx + s->txc * 4;
+    unsigned char *key = (unsigned char *)s->key;
+    unsigned int size = AES_BLOCK_SIZE;
+    unsigned char biv[AES_BLOCK_SIZE];
+
+    if (s->ctl & SS_DECRYPTION) {
+        memcpy(biv, src, AES_BLOCK_SIZE);
+    }
+
+    if (s->ctl & SS_CBC) {
+        switch (s->ctl & 0x300) {
+        case SS_AES_128BITS:
+            CBC_SET_IV(&aes128, s->iv);
+
+            if (s->ctl & SS_DECRYPTION) {
+                aes128_set_decrypt_key(&aes128.ctx, key);
+                CBC_DECRYPT(&aes128, aes128_decrypt, size, dst, src);
+            } else {
+                aes128_set_encrypt_key(&aes128.ctx, key);
+                CBC_ENCRYPT(&aes128, aes128_encrypt, size, dst, src);
+            }
+            break;
+        case SS_AES_192BITS:
+            CBC_SET_IV(&aes192, s->iv);
+
+            if (s->ctl & SS_DECRYPTION) {
+                aes192_set_decrypt_key(&aes192.ctx, key);
+                CBC_DECRYPT(&aes192, aes192_decrypt, size, dst, src);
+            } else {
+                aes192_set_encrypt_key(&aes192.ctx, key);
+                CBC_ENCRYPT(&aes192, aes192_encrypt, size, dst, src);
+            }
+            break;
+        case SS_AES_256BITS:
+            CBC_SET_IV(&aes256, s->iv);
+
+            if (s->ctl & SS_DECRYPTION) {
+                aes256_set_decrypt_key(&aes256.ctx, key);
+                CBC_DECRYPT(&aes256, aes256_decrypt, size, dst, src);
+            } else {
+                aes256_set_encrypt_key(&aes256.ctx, key);
+                CBC_ENCRYPT(&aes256, aes256_encrypt, size, dst, src);
+            }
+            break;
+        }
+        /* Copy next IV in registers */
+        if (s->ctl & SS_DECRYPTION) {
+            memcpy(s->iv, biv, AES_BLOCK_SIZE);
+        } else {
+            memcpy(s->iv, dst, AES_BLOCK_SIZE);
+        }
+    } else {
+        switch (s->ctl & 0x300) {
+        case SS_AES_128BITS:
+            if (s->ctl & SS_DECRYPTION) {
+                aes128_set_decrypt_key(&ecb128, key);
+                aes128_decrypt(&ecb128, size, dst, src);
+            } else {
+                aes128_set_encrypt_key(&ecb128, key);
+                aes128_encrypt(&ecb128, size, dst, src);
+            }
+            break;
+        case SS_AES_192BITS:
+            if (s->ctl & SS_DECRYPTION) {
+                aes192_set_decrypt_key(&ecb192, key);
+                aes192_decrypt(&ecb192, size, dst, src);
+            } else {
+                aes192_set_encrypt_key(&ecb192, key);
+                aes192_encrypt(&ecb192, size, dst, src);
+            }
+            break;
+        case SS_AES_256BITS:
+            if (s->ctl & SS_DECRYPTION) {
+                aes256_set_decrypt_key(&ecb256, (const unsigned char *) s->key);
+                aes256_decrypt(&ecb256, size, dst, src);
+            } else {
+                aes256_set_encrypt_key(&ecb256, (const unsigned char *) s->key);
+                aes256_encrypt(&ecb256, size, dst, src);
+            }
+            break;
+        }
+    }
+    s->txc += size / 4;
+    s->rxc -= size / 4;
+
+    if (s->rxc > 0) {
+        rx_pop(s, size / 4);
+        allwinner_sun4i_ss_try_work(s);
+    }
+}
+
+static void allwinner_sun4i_ss_update_fcsr(AwSun4iSSState *s)
+{
+    s->fcsr = (s->txc << 16) | ((32 - s->rxc) << 24);
+}
+
+static void allwinner_sun4i_ss_try_work(AwSun4iSSState *s)
+{
+    if (!(s->ctl & SS_ENABLED)) {
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_AES && can_work(s, AES_BLOCK_SIZE)) {
+        do_aes(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_DES && can_work(s, DES_BLOCK_SIZE)) {
+        do_des(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_3DES && can_work(s, DES3_BLOCK_SIZE)) {
+        do_des3(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_MD5 && s->rxc >= MD5_BLOCK_SIZE / 4) {
+        do_md5(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_SHA1 && s->rxc >= SHA1_BLOCK_SIZE / 4) {
+        do_sha1(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+    if ((s->ctl & 0x70) == SS_OP_PRNG) {
+        do_prng(s);
+        allwinner_sun4i_ss_update_fcsr(s);
+        return;
+    }
+}
+
+static uint32_t tx_pop(AwSun4iSSState *s)
+{
+    uint32_t *tx = (uint32_t *)s->tx;
+    uint32_t v = 0;
+    int i;
+
+    if (s->txc > 0) {
+        v = tx[0];
+        s->txc--;
+        for (i = 0; i < s->txc; i++) {
+            tx[i] = tx[i + 1];
+        }
+        allwinner_sun4i_ss_update_fcsr(s);
+        allwinner_sun4i_ss_try_work(s);
+    }
+    return v;
+}
+
+static void allwinner_sun4i_ss_reset_common(AwSun4iSSState *s)
+{
+    s->ctl = 0;
+    s->txc = 0;
+    s->rxc = 0;
+    allwinner_sun4i_ss_update_fcsr(s);
+}
+
+static void allwinner_sun4i_ss_reset(DeviceState *dev)
+{
+    AwSun4iSSState *s = AW_SUN4I_SS(dev);
+
+    trace_allwinner_sun4i_ss_reset();
+
+    allwinner_sun4i_ss_reset_common(s);
+}
+
+static uint64_t allwinner_sun4i_ss_read(void *opaque, hwaddr offset,
+                                          unsigned size)
+{
+    AwSun4iSSState *s = AW_SUN4I_SS(opaque);
+    uint64_t value = 0;
+
+    switch (offset) {
+    case REG_CTL:
+        value = s->ctl;
+        break;
+    case REG_IV_0:
+        value = s->iv[0];
+        break;
+    case REG_IV_1:
+        value = s->iv[1];
+        break;
+    case REG_IV_2:
+        value = s->iv[2];
+        break;
+    case REG_IV_3:
+        value = s->iv[3];
+        break;
+    case REG_IV_4:
+        value = s->iv[4];
+        break;
+    case REG_FCSR:
+        value = s->fcsr;
+        break;
+    case REG_KEY_0:
+        value = s->key[0];
+        break;
+    case REG_KEY_1:
+        value = s->key[1];
+        break;
+    case REG_KEY_2:
+        value = s->key[2];
+        break;
+    case REG_KEY_3:
+        value = s->key[3];
+        break;
+    case REG_KEY_4:
+        value = s->key[4];
+        break;
+    case REG_KEY_5:
+        value = s->key[5];
+        break;
+    case REG_KEY_6:
+        value = s->key[6];
+        break;
+    case REG_KEY_7:
+        value = s->key[7];
+        break;
+    case REG_MD0:
+        value = s->md[0];
+        break;
+    case REG_MD1:
+        value = s->md[1];
+        break;
+    case REG_MD2:
+        value = s->md[2];
+        break;
+    case REG_MD3:
+        value = s->md[3];
+        break;
+    case REG_MD4:
+        value = s->md[4];
+        break;
+    case REG_TXFIFO:
+        value = tx_pop(s);
+        break;
+    case REG_RXFIFO:
+        value = s->rx[0];
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "allwinner_sun4i_ss: read access to unknown "
+                                 "CRYPTO register 0x" TARGET_FMT_plx "\n",
+                                  offset);
+    }
+
+    trace_allwinner_sun4i_ss_read(offset, value);
+    return value;
+}
+
+static void rx_push(AwSun4iSSState *s, uint32_t value)
+{
+    uint32_t *rx = (uint32_t *)s->rx;
+
+    if (!(s->ctl & SS_ENABLED)) {
+        return;
+    }
+    if (s->rxc > SS_RX_MAX) {
+        return;
+    }
+    rx[s->rxc] = value;
+    s->rxc++;
+    allwinner_sun4i_ss_update_fcsr(s);
+    allwinner_sun4i_ss_try_work(s);
+
+    return;
+}
+
+static void allwinner_sun4i_ss_write(void *opaque, hwaddr offset,
+                                       uint64_t value, unsigned size)
+{
+    AwSun4iSSState *s = AW_SUN4I_SS(opaque);
+    bool was_disabled = !(s->ctl & SS_ENABLED);
+
+    trace_allwinner_sun4i_ss_write(offset, value);
+
+    switch (offset) {
+    case REG_CTL:
+        s->ctl = value;
+        if (!(s->ctl & SS_ENABLED)) {
+            allwinner_sun4i_ss_reset_common(s);
+            break;
+        }
+        if (was_disabled) {
+            if (s->ctl & SS_IV_ARBITRARY) {
+                s->md[0] = s->iv[0];
+                s->md[1] = s->iv[1];
+                s->md[2] = s->iv[2];
+                s->md[3] = s->iv[3];
+                s->md[4] = s->iv[4];
+            } else {
+                if ((s->ctl & 0x70) == SS_OP_MD5) {
+                    s->md[0] = 0x67452301;
+                    s->md[1] = 0xefcdab89;
+                    s->md[2] = 0x98badcfe;
+                    s->md[3] = 0x10325476;
+                } else {
+                    s->md[0] = 0x67452301;
+                    s->md[1] = 0xefcdab89;
+                    s->md[2] = 0x98badcfe;
+                    s->md[3] = 0x10325476;
+                    s->md[4] = 0xC3D2E1F0;
+                }
+            }
+        }
+        if ((s->ctl & 0x70) == SS_OP_PRNG) {
+            do_prng(s);
+            allwinner_sun4i_ss_update_fcsr(s);
+        }
+        if ((s->ctl & 0x70) == SS_OP_MD5 && s->ctl & SS_DATA_END) {
+            s->ctl &= ~SS_DATA_END;
+            return;
+        }
+        if ((s->ctl & 0x70) == SS_OP_SHA1 && s->ctl & SS_DATA_END) {
+            s->ctl &= ~SS_DATA_END;
+            return;
+        }
+        break;
+    case REG_IV_0:
+        s->iv[0] = value;
+        break;
+    case REG_IV_1:
+        s->iv[1] = value;
+        break;
+    case REG_IV_2:
+        s->iv[2] = value;
+        break;
+    case REG_IV_3:
+        s->iv[3] = value;
+        break;
+    case REG_IV_4:
+        s->iv[4] = value;
+        break;
+    case REG_KEY_0:
+        s->key[0] = value;
+        break;
+    case REG_KEY_1:
+        s->key[1] = value;
+        break;
+    case REG_KEY_2:
+        s->key[2] = value;
+        break;
+    case REG_KEY_3:
+        s->key[3] = value;
+        break;
+    case REG_KEY_4:
+        s->key[4] = value;
+        break;
+    case REG_KEY_5:
+        s->key[5] = value;
+        break;
+    case REG_KEY_6:
+        s->key[6] = value;
+        break;
+    case REG_KEY_7:
+        s->key[7] = value;
+        break;
+    case REG_RXFIFO:
+        rx_push(s, value);
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "allwinner_sun4i_ss: write access to unknown "
+                                 "CRYPTO register 0x" TARGET_FMT_plx "\n",
+                                  offset);
+    }
+}
+
+static const MemoryRegionOps allwinner_sun4i_ss_mem_ops = {
+    .read = allwinner_sun4i_ss_read,
+    .write = allwinner_sun4i_ss_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .impl.min_access_size = 4,
+};
+
+static void allwinner_sun4i_ss_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    AwSun4iSSState *s = AW_SUN4I_SS(obj);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &allwinner_sun4i_ss_mem_ops,
+                           s, TYPE_AW_SUN4I_SS, 4 * KiB);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+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),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void allwinner_sun4i_ss_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = allwinner_sun4i_ss_reset;
+    dc->vmsd = &vmstate_allwinner_sun4i_ss;
+}
+
+static const TypeInfo allwinner_sun4i_ss_info = {
+    .name           = TYPE_AW_SUN4I_SS,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(AwSun4iSSState),
+    .instance_init  = allwinner_sun4i_ss_init,
+    .class_init     = allwinner_sun4i_ss_class_init,
+};
+
+static void allwinner_sun4i_ss_register_types(void)
+{
+    type_register_static(&allwinner_sun4i_ss_info);
+}
+
+type_init(allwinner_sun4i_ss_register_types)
diff --git a/hw/crypto/meson.build b/hw/crypto/meson.build
new file mode 100644
index 0000000000..4368e1e401
--- /dev/null
+++ b/hw/crypto/meson.build
@@ -0,0 +1,3 @@
+if nettle.found()
+    softmmu_ss.add(when: 'CONFIG_ALLWINNER_CRYPTO_SUN4I_SS', if_true: files('allwinner-sun4i-ss.c'))
+endif
diff --git a/hw/crypto/trace-events b/hw/crypto/trace-events
new file mode 100644
index 0000000000..39739b317b
--- /dev/null
+++ b/hw/crypto/trace-events
@@ -0,0 +1,3 @@
+allwinner_sun4i_ss_reset(void) "HW reset"
+allwinner_sun4i_ss_read(uint64_t offset, uint64_t val) "MMIO read: offset=0x%" PRIx64 " value=0x%" PRIx64
+allwinner_sun4i_ss_write(uint64_t offset, uint64_t val) "MMIO write: offset=0x%" PRIx64 " value=0x%" PRIx64
diff --git a/hw/crypto/trace.h b/hw/crypto/trace.h
new file mode 100644
index 0000000000..36d01d937d
--- /dev/null
+++ b/hw/crypto/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_crypto.h"
diff --git a/hw/meson.build b/hw/meson.build
index b3366c888e..b8d2cdcc38 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -6,6 +6,7 @@ subdir('block')
 subdir('char')
 subdir('core')
 subdir('cpu')
+subdir('crypto')
 subdir('display')
 subdir('dma')
 subdir('gpio')
diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index a76dc7b84d..d5b1d3f616 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -4,6 +4,7 @@
 #include "qemu/error-report.h"
 #include "hw/char/serial.h"
 #include "hw/arm/boot.h"
+#include "hw/crypto/allwinner-sun4i-ss.h"
 #include "hw/timer/allwinner-a10-pit.h"
 #include "hw/intc/allwinner-a10-pic.h"
 #include "hw/net/allwinner_emac.h"
@@ -33,6 +34,7 @@ struct AwA10State {
     AwA10PITState timer;
     AwA10PICState intc;
     AwEmacState emac;
+    AwSun4iSSState crypto;
     AllwinnerAHCIState sata;
     AwSdHostState mmc0;
     AwRtcState rtc;
diff --git a/include/hw/crypto/allwinner-sun4i-ss.h b/include/hw/crypto/allwinner-sun4i-ss.h
new file mode 100644
index 0000000000..e604819ad0
--- /dev/null
+++ b/include/hw/crypto/allwinner-sun4i-ss.h
@@ -0,0 +1,72 @@
+/*
+ * Allwinner sun4i-ss cryptographic offloader emulation
+ *
+ * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_CRYPTO_ALLWINNER_SUN4I_SS_H
+#define HW_CRYPTO_ALLWINNER_SUN4I_SS_H
+
+#include "qom/object.h"
+#include "hw/sysbus.h"
+
+#define SS_RX_MAX     32
+#define SS_RX_DEFAULT SS_RX_MAX
+#define SS_TX_MAX     33
+
+/**
+ * Object model
+ * @{
+ */
+
+#define TYPE_AW_SUN4I_SS "allwinner-sun4i-ss"
+OBJECT_DECLARE_SIMPLE_TYPE(AwSun4iSSState, AW_SUN4I_SS)
+
+/** @} */
+
+/**
+ * Allwinner sun4i-ss crypto object instance state
+ */
+struct AwSun4iSSState {
+    /*< private >*/
+    SysBusDevice  parent_obj;
+    /*< public >*/
+
+    /** Maps I/O registers in physical memory */
+    MemoryRegion iomem;
+
+    /** @} */
+    unsigned char rx[SS_RX_MAX * 4];
+    unsigned int rxc;
+    unsigned char tx[SS_TX_MAX * 4];
+    unsigned int txc;
+
+    /**
+     * @name Hardware Registers
+     * @{
+     */
+
+    uint32_t    ctl;    /**< Control register */
+    uint32_t    fcsr;   /**< FIFO control register */
+    uint32_t    iv[5];  /**< IV registers */
+    uint32_t    key[8]; /**< KEY registers */
+    uint32_t    md[5];  /**< Message Digest registers */
+
+    /** @} */
+
+};
+
+#endif /* HW_CRYPTO_ALLWINNER_SUN4I_SS_H */
diff --git a/meson.build b/meson.build
index 861de93c4f..cf0bf07bf4 100644
--- a/meson.build
+++ b/meson.build
@@ -2718,6 +2718,7 @@ if have_system
     'hw/block',
     'hw/block/dataplane',
     'hw/char',
+    'hw/crypto',
     'hw/display',
     'hw/dma',
     'hw/hppa',
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-10 19:12 [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device Corentin Labbe
@ 2022-04-21 12:38 ` Peter Maydell
  2022-04-21 13:18   ` Daniel P. Berrangé
  2022-04-24 19:10   ` LABBE Corentin
  0 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2022-04-21 12:38 UTC (permalink / raw)
  To: Corentin Labbe; +Cc: b.galvani, qemu-arm, Daniel P. Berrange, qemu-devel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-21 12:38 ` Peter Maydell
@ 2022-04-21 13:18   ` Daniel P. Berrangé
  2022-04-24 19:10   ` LABBE Corentin
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-04-21 13:18 UTC (permalink / raw)
  To: Peter Maydell; +Cc: b.galvani, qemu-arm, Corentin Labbe, qemu-devel

On Thu, Apr 21, 2022 at 01:38:00PM +0100, Peter Maydell wrote:
> 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>
> 

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



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

The include/crypto/{cipher,hash}.h files should provide APIs that
cover these uses cases from what I see in this patch.

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



^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-21 12:38 ` Peter Maydell
  2022-04-21 13:18   ` Daniel P. Berrangé
@ 2022-04-24 19:10   ` LABBE Corentin
  2022-04-25 10:19     ` Daniel P. Berrangé
  1 sibling, 1 reply; 9+ messages in thread
From: LABBE Corentin @ 2022-04-24 19:10 UTC (permalink / raw)
  To: Peter Maydell; +Cc: b.galvani, qemu-arm, Daniel P. Berrange, qemu-devel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-24 19:10   ` LABBE Corentin
@ 2022-04-25 10:19     ` Daniel P. Berrangé
  2022-04-25 13:03       ` LABBE Corentin
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-04-25 10:19 UTC (permalink / raw)
  To: LABBE Corentin; +Cc: b.galvani, Peter Maydell, qemu-arm, qemu-devel

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



^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-25 10:19     ` Daniel P. Berrangé
@ 2022-04-25 13:03       ` LABBE Corentin
  2022-04-25 14:29         ` Daniel P. Berrangé
  0 siblings, 1 reply; 9+ messages in thread
From: LABBE Corentin @ 2022-04-25 13:03 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: b.galvani, Peter Maydell, qemu-arm, qemu-devel

Le Mon, Apr 25, 2022 at 11:19:16AM +0100, Daniel P. Berrangé a écrit :
> 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.
> 

Hello

Cipher operations are converted to the qemu API without problem, qemu API permits to even made it simplier. (same code path for des/3des/aes).

But for hashs, only nettle has the needed function in its API. (I checked gcrypt, gnutls and glib).
I have added hashcompress in crypto API but not sure if it is worth it if only nettle support it.
Note that nettle export compress functions in its header only for sha1 and md5, but I will send a fix to nettle to include others shaXXX (which will be needed for crypto/sun8i-ce).

Regards

diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 1ca1a41062..b9342b4fe1 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -26,10 +26,22 @@
 #include <nettle/sha.h>
 #include <nettle/ripemd160.h>
 
+#ifndef nettle_sha256_compress
+#define nettle_sha256_compress _nettle_sha256_compress
+void _nettle_sha256_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
+#endif
+
+#ifndef nettle_sha512_compress
+#define nettle_sha512_compress _nettle_sha512_compress
+void _nettle_sha512_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
+#endif
+
 typedef void (*qcrypto_nettle_init)(void *ctx);
 typedef void (*qcrypto_nettle_write)(void *ctx,
                                      size_t len,
                                      const uint8_t *buf);
+typedef void (*qcrypto_nettle_compress)(uint32_t *state,
+                                     const uint8_t *buf);
 typedef void (*qcrypto_nettle_result)(void *ctx,
                                       size_t len,
                                       uint8_t *buf);
@@ -47,18 +59,21 @@ union qcrypto_hash_ctx {
 struct qcrypto_hash_alg {
     qcrypto_nettle_init init;
     qcrypto_nettle_write write;
+    qcrypto_nettle_compress compress;
     qcrypto_nettle_result result;
     size_t len;
 } qcrypto_hash_alg_map[] = {
     [QCRYPTO_HASH_ALG_MD5] = {
         .init = (qcrypto_nettle_init)md5_init,
         .write = (qcrypto_nettle_write)md5_update,
+        .compress = (qcrypto_nettle_compress)nettle_md5_compress,
         .result = (qcrypto_nettle_result)md5_digest,
         .len = MD5_DIGEST_SIZE,
     },
     [QCRYPTO_HASH_ALG_SHA1] = {
         .init = (qcrypto_nettle_init)sha1_init,
         .write = (qcrypto_nettle_write)sha1_update,
+        .compress = (qcrypto_nettle_compress)nettle_sha1_compress,
         .result = (qcrypto_nettle_result)sha1_digest,
         .len = SHA1_DIGEST_SIZE,
     },
@@ -70,6 +85,7 @@ struct qcrypto_hash_alg {
     },
     [QCRYPTO_HASH_ALG_SHA256] = {
         .init = (qcrypto_nettle_init)sha256_init,
+        .compress = (qcrypto_nettle_compress)_nettle_sha256_compress,
         .write = (qcrypto_nettle_write)sha256_update,
         .result = (qcrypto_nettle_result)sha256_digest,
         .len = SHA256_DIGEST_SIZE,
@@ -82,6 +98,7 @@ struct qcrypto_hash_alg {
     },
     [QCRYPTO_HASH_ALG_SHA512] = {
         .init = (qcrypto_nettle_init)sha512_init,
+        .compress = (qcrypto_nettle_compress)_nettle_sha512_compress,
         .write = (qcrypto_nettle_write)sha512_update,
         .result = (qcrypto_nettle_result)sha512_digest,
         .len = SHA512_DIGEST_SIZE,
@@ -156,6 +173,32 @@ qcrypto_nettle_hash_bytesv(QCryptoHashAlgorithm alg,
 }
 
 
+static int
+qcrypto_nettle_compress_bytesv(QCryptoHashAlgorithm alg,
+                           const struct iovec *iov,
+                           size_t niov,
+                           uint32_t *state,
+                           Error **errp)
+{
+    size_t i;
+
+    if (!qcrypto_hash_supports(alg)) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+        return -1;
+    }
+
+    for (i = 0; i < niov; i++) {
+        uint8_t *data = iov[i].iov_base;
+        qcrypto_hash_alg_map[alg].compress(state, data);
+    }
+
+    return 0;
+}
+
+
 QCryptoHashDriver qcrypto_hash_lib_driver = {
     .hash_bytesv = qcrypto_nettle_hash_bytesv,
+    .compress_bytesv = qcrypto_nettle_compress_bytesv,
 };
diff --git a/crypto/hash.c b/crypto/hash.c
index b0f8228bdc..27f1ce08ee 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -142,3 +142,24 @@ int qcrypto_hash_base64(QCryptoHashAlgorithm alg,
 
     return qcrypto_hash_base64v(alg, &iov, 1, base64, errp);
 }
+
+int qcrypto_compress_bytesv(QCryptoHashAlgorithm alg,
+                        const struct iovec *iov,
+                        size_t niov,
+                        uint32_t *state,
+                        Error **errp)
+{
+    return qcrypto_hash_lib_driver.compress_bytesv(alg, iov, niov, state, errp);
+}
+
+int qcrypto_compress_bytes(QCryptoHashAlgorithm alg,
+                       const char *buf,
+                       size_t len,
+                       uint32_t *state,
+                       Error **errp)
+{
+    struct iovec iov = { .iov_base = (char *)buf,
+                         .iov_len = len };
+    return qcrypto_compress_bytesv(alg, &iov, 1, state, errp);
+}
+
diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
index cee26ccb47..b1d6a3d82f 100644
--- a/crypto/hashpriv.h
+++ b/crypto/hashpriv.h
@@ -24,6 +24,11 @@ struct QCryptoHashDriver {
                        uint8_t **result,
                        size_t *resultlen,
                        Error **errp);
+int qcrypto_compress_bytes(QCryptoHashAlgorithm alg,
+                       const char *buf,
+                       size_t len,
+                       uint32_t *state,
+                       Error **errp)
+{
+    struct iovec iov = { .iov_base = (char *)buf,
+                         .iov_len = len };
+    return qcrypto_compress_bytesv(alg, &iov, 1, state, errp);
+}
+
diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
index cee26ccb47..b1d6a3d82f 100644
--- a/crypto/hashpriv.h
+++ b/crypto/hashpriv.h
@@ -24,6 +24,11 @@ struct QCryptoHashDriver {
                        uint8_t **result,
                        size_t *resultlen,
                        Error **errp);
+    int (*compress_bytesv)(QCryptoHashAlgorithm alg,
+                       const struct iovec *iov,
+                       size_t niov,
+                       uint32_t *state,
+                       Error **errp);
 };
 
 extern QCryptoHashDriver qcrypto_hash_lib_driver;
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 54d87aa2a1..770622919d 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -189,4 +189,15 @@ int qcrypto_hash_base64(QCryptoHashAlgorithm alg,
                         char **base64,
                         Error **errp);
 
+int qcrypto_compress_bytesv(QCryptoHashAlgorithm alg,
+                        const struct iovec *iov,
+                        size_t niov,
+                        uint32_t *state,
+                        Error **errp);
+
+int qcrypto_compress_bytes(QCryptoHashAlgorithm alg,
+                       const char *buf,
+                       size_t len,
+                       uint32_t *state,
+                       Error **errp);
 #endif /* QCRYPTO_HASH_H */



^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-25 13:03       ` LABBE Corentin
@ 2022-04-25 14:29         ` Daniel P. Berrangé
  2022-04-27  8:34           ` LABBE Corentin
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-04-25 14:29 UTC (permalink / raw)
  To: LABBE Corentin; +Cc: b.galvani, Peter Maydell, qemu-arm, qemu-devel

On Mon, Apr 25, 2022 at 03:03:11PM +0200, LABBE Corentin wrote:
> diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
> index 1ca1a41062..b9342b4fe1 100644
> --- a/crypto/hash-nettle.c
> +++ b/crypto/hash-nettle.c
> @@ -26,10 +26,22 @@
>  #include <nettle/sha.h>
>  #include <nettle/ripemd160.h>
>  
> +#ifndef nettle_sha256_compress
> +#define nettle_sha256_compress _nettle_sha256_compress
> +void _nettle_sha256_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> +#endif
> +
> +#ifndef nettle_sha512_compress
> +#define nettle_sha512_compress _nettle_sha512_compress
> +void _nettle_sha512_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> +#endif

The 'nettle_sha256_compress' function doesn't exist in any header file
from nettle that I've looked at.

The '_nettle_sha256_compress' function exists as a symbol in the .so
library, but it is clearly not intended for public usage:

$ nm -a -D /usr/lib64/libnettle.so | grep sha256_compress
0000000000027730 T _nettle_sha256_compress@@NETTLE_INTERNAL_8_4

So this #define magic is definitely not something we can do.

IOW, unless I'm missing something, I don't think we can even
use nettle for your desired goal, which leaves us no impl at
all.


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



^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-25 14:29         ` Daniel P. Berrangé
@ 2022-04-27  8:34           ` LABBE Corentin
  2022-04-27  8:42             ` Daniel P. Berrangé
  0 siblings, 1 reply; 9+ messages in thread
From: LABBE Corentin @ 2022-04-27  8:34 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: b.galvani, Peter Maydell, qemu-arm, qemu-devel

Le Mon, Apr 25, 2022 at 03:29:32PM +0100, Daniel P. Berrangé a écrit :
> On Mon, Apr 25, 2022 at 03:03:11PM +0200, LABBE Corentin wrote:
> > diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
> > index 1ca1a41062..b9342b4fe1 100644
> > --- a/crypto/hash-nettle.c
> > +++ b/crypto/hash-nettle.c
> > @@ -26,10 +26,22 @@
> >  #include <nettle/sha.h>
> >  #include <nettle/ripemd160.h>
> >  
> > +#ifndef nettle_sha256_compress
> > +#define nettle_sha256_compress _nettle_sha256_compress
> > +void _nettle_sha256_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> > +#endif
> > +
> > +#ifndef nettle_sha512_compress
> > +#define nettle_sha512_compress _nettle_sha512_compress
> > +void _nettle_sha512_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> > +#endif
> 
> The 'nettle_sha256_compress' function doesn't exist in any header file
> from nettle that I've looked at.
> 
> The '_nettle_sha256_compress' function exists as a symbol in the .so
> library, but it is clearly not intended for public usage:
> 
> $ nm -a -D /usr/lib64/libnettle.so | grep sha256_compress
> 0000000000027730 T _nettle_sha256_compress@@NETTLE_INTERNAL_8_4
> 
> So this #define magic is definitely not something we can do.
> 
> IOW, unless I'm missing something, I don't think we can even
> use nettle for your desired goal, which leaves us no impl at
> all.

sha256 is only necessary for the upcomming sun8i-ce, so it do not block sun4i-ss.
I try to contact nettle to add necessary headers.

So for sun4i-ss, what do you think about my qcrypto_nettle_compress implementation ?


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device
  2022-04-27  8:34           ` LABBE Corentin
@ 2022-04-27  8:42             ` Daniel P. Berrangé
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-04-27  8:42 UTC (permalink / raw)
  To: LABBE Corentin; +Cc: b.galvani, Peter Maydell, qemu-arm, qemu-devel

On Wed, Apr 27, 2022 at 10:34:24AM +0200, LABBE Corentin wrote:
> Le Mon, Apr 25, 2022 at 03:29:32PM +0100, Daniel P. Berrangé a écrit :
> > On Mon, Apr 25, 2022 at 03:03:11PM +0200, LABBE Corentin wrote:
> > > diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
> > > index 1ca1a41062..b9342b4fe1 100644
> > > --- a/crypto/hash-nettle.c
> > > +++ b/crypto/hash-nettle.c
> > > @@ -26,10 +26,22 @@
> > >  #include <nettle/sha.h>
> > >  #include <nettle/ripemd160.h>
> > >  
> > > +#ifndef nettle_sha256_compress
> > > +#define nettle_sha256_compress _nettle_sha256_compress
> > > +void _nettle_sha256_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> > > +#endif
> > > +
> > > +#ifndef nettle_sha512_compress
> > > +#define nettle_sha512_compress _nettle_sha512_compress
> > > +void _nettle_sha512_compress(uint32_t *state, const uint8_t *input, const uint32_t *k);
> > > +#endif
> > 
> > The 'nettle_sha256_compress' function doesn't exist in any header file
> > from nettle that I've looked at.
> > 
> > The '_nettle_sha256_compress' function exists as a symbol in the .so
> > library, but it is clearly not intended for public usage:
> > 
> > $ nm -a -D /usr/lib64/libnettle.so | grep sha256_compress
> > 0000000000027730 T _nettle_sha256_compress@@NETTLE_INTERNAL_8_4
> > 
> > So this #define magic is definitely not something we can do.
> > 
> > IOW, unless I'm missing something, I don't think we can even
> > use nettle for your desired goal, which leaves us no impl at
> > all.
> 
> sha256 is only necessary for the upcomming sun8i-ce, so it do not block sun4i-ss.
> I try to contact nettle to add necessary headers.
> 
> So for sun4i-ss, what do you think about my qcrypto_nettle_compress implementation ?

It is reasonable in general.

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



^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-27  8:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-10 19:12 [PATCH] hw/crypto: add Allwinner sun4i-ss crypto device Corentin Labbe
2022-04-21 12:38 ` Peter Maydell
2022-04-21 13:18   ` Daniel P. Berrangé
2022-04-24 19:10   ` LABBE Corentin
2022-04-25 10:19     ` Daniel P. Berrangé
2022-04-25 13:03       ` LABBE Corentin
2022-04-25 14:29         ` Daniel P. Berrangé
2022-04-27  8:34           ` LABBE Corentin
2022-04-27  8:42             ` Daniel P. Berrangé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.