[PATCH 0/3] aspeed: HACE hash Scatter-Gather support

[PATCH 0/3] aspeed: HACE hash Scatter-Gather support

[Klaus Heinrich Kiwi]

This series adds support for scatter-gather sha256 and sha512 operations
on Aspeed's HACE (Hash And Crypto Engine) to the Aspeed model. These
operations are supported on AST2600 series of machines.

 -Klaus

[PATCH 1/3] aspeed: Coding Style cleanups on do_hash_operation

[Klaus Heinrich Kiwi]

Basically using camelCase for some variables...

Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
---
 hw/misc/aspeed_hace.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 6e5b447a48..93313d2b80 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -86,24 +86,24 @@ static int hash_algo_lookup(uint32_t mask)
 
 static int do_hash_operation(AspeedHACEState *s, int algo)
 {
-    hwaddr src, len, dest;
-    uint8_t *digest_buf = NULL;
-    size_t digest_len = 0;
-    char *src_buf;
+    uint32_t src, len, dest;
+    uint8_t *digestBuf = NULL;
+    size_t digestLen = 0;
+    char *srcBuf;
     int rc;
 
     src = s->regs[R_HASH_SRC];
     len = s->regs[R_HASH_SRC_LEN];
     dest = s->regs[R_HASH_DEST];
 
-    src_buf = address_space_map(&s->dram_as, src, &len, false,
-                                MEMTXATTRS_UNSPECIFIED);
-    if (!src_buf) {
+    srcBuf = address_space_map(&s->dram_as, src, (hwaddr *) &len,
+                               false, MEMTXATTRS_UNSPECIFIED);
+    if (!srcBuf) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map dram\n", __func__);
         return -EACCES;
     }
 
-    rc = qcrypto_hash_bytes(algo, src_buf, len, &digest_buf, &digest_len,
+    rc = qcrypto_hash_bytes(algo, srcBuf, len, &digestBuf, &digestLen,
                             &error_fatal);
     if (rc < 0) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
@@ -111,14 +111,14 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
     }
 
     rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
-                             digest_buf, digest_len);
+                             digestBuf, digestLen);
     if (rc) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: address space write failed\n", __func__);
     }
-    g_free(digest_buf);
+    g_free(digestBuf);
 
-    address_space_unmap(&s->dram_as, src_buf, len, false, len);
+    address_space_unmap(&s->dram_as, srcBuf, len, false, len);
 
     /*
      * Set status bits to indicate completion. Testing shows hardware sets
-- 
2.25.1

[PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Klaus Heinrich Kiwi]

Complement the Aspeed HACE support with Scatter-Gather hash support for
sha256 and sha512. Scatter-Gather is only supported on AST2600-series.

Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
---
 hw/misc/aspeed_hace.c         | 127 ++++++++++++++++++++++++++++++++--
 include/hw/misc/aspeed_hace.h |   6 ++
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 93313d2b80..8a37b1d961 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -57,6 +57,10 @@
 /* Other cmd bits */
 #define  HASH_IRQ_EN                    BIT(9)
 #define  HASH_SG_EN                     BIT(18)
+/* Scatter-gather data list */
+#define  SG_LIST_LAST                   BIT(31)
+#define  SG_LIST_LEN_MASK               0x7fffffff
+#define  SG_LIST_ADDR_MASK              0x7ffffff8  /* 8-byte aligned */
 
 static const struct {
     uint32_t mask;
@@ -129,6 +133,117 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
     return 0;
 }
 
+static int do_hash_sg_operation(AspeedHACEState *s, int algo)
+{
+    uint32_t src, dest, reqSize;
+    hwaddr len;
+    const size_t reqLen = sizeof(struct aspeed_sg_list);
+    struct iovec iov[ASPEED_HACE_MAX_SG];
+    unsigned int i = 0;
+    unsigned int isLast = 0;
+    uint8_t *digestBuf = NULL;
+    size_t digestLen = 0, size = 0;
+    struct aspeed_sg_list *sgList;
+    int rc;
+
+    reqSize = s->regs[R_HASH_SRC_LEN];
+    dest = s->regs[R_HASH_DEST];
+
+    while (!isLast && i < ASPEED_HACE_MAX_SG) {
+        src = s->regs[R_HASH_SRC] + (i * reqLen);
+        len = reqLen;
+        sgList = (struct aspeed_sg_list *) address_space_map(&s->dram_as,
+                                                                     src,
+                                                         (hwaddr *) &len,
+                                                                   false,
+                                                 MEMTXATTRS_UNSPECIFIED);
+        if (!sgList) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+             "%s: failed to map dram for SG Array entry '%u' for address '0x%0x'\n",
+             __func__, i, src);
+            rc = -EACCES;
+            goto cleanup;
+        }
+        if (len != reqLen)
+            qemu_log_mask(LOG_GUEST_ERROR,
+             "%s:  Warning: dram map for SG array entry '%u' requested size '%lu' != mapped size '%lu'\n",
+             __func__, i, reqLen, len);
+
+        isLast = sgList->len & SG_LIST_LAST;
+
+        iov[i].iov_len = (hwaddr) (sgList->len & SG_LIST_LEN_MASK);
+        iov[i].iov_base = address_space_map(&s->dram_as,
+                            sgList->phy_addr & SG_LIST_ADDR_MASK,
+                            &iov[i].iov_len, false,
+                            MEMTXATTRS_UNSPECIFIED);
+        if (!iov[i].iov_base) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+             "%s: failed to map dram for SG array entry '%u' for region '0x%x', len '%u'\n",
+             __func__, i, sgList->phy_addr & SG_LIST_ADDR_MASK,
+             sgList->len & SG_LIST_LEN_MASK);
+            rc = -EACCES;
+            goto cleanup;
+        }
+        if (iov[i].iov_len != (sgList->len & SG_LIST_LEN_MASK))
+            qemu_log_mask(LOG_GUEST_ERROR,
+             "%s:  Warning: dram map for SG region entry %u requested size %u != mapped size %lu\n",
+             __func__, i, (sgList->len & SG_LIST_LEN_MASK), iov[i].iov_len);
+
+
+        address_space_unmap(&s->dram_as, (void *) sgList, len, false,
+                            len);
+        size += iov[i].iov_len;
+        i++;
+    }
+
+    if (!isLast) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                     "%s: Error: Exhausted maximum of '%u' SG array entries\n",
+                     __func__, ASPEED_HACE_MAX_SG);
+        rc = -ENOTSUP;
+        goto cleanup;
+    }
+
+    if (size != reqSize)
+        qemu_log_mask(LOG_GUEST_ERROR,
+         "%s: Warning: requested SG total size %u != actual size %lu\n",
+         __func__, reqSize, size);
+
+    rc = qcrypto_hash_bytesv(algo, iov, i, &digestBuf, &digestLen,
+                            &error_fatal);
+    if (rc < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n",
+                      __func__);
+        goto cleanup;
+    }
+
+    rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
+                             digestBuf, digestLen);
+    if (rc)
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: address space write failed\n", __func__);
+    g_free(digestBuf);
+
+cleanup:
+
+    for (; i > 0; i--) {
+        address_space_unmap(&s->dram_as, iov[i - 1].iov_base,
+                            iov[i - 1].iov_len, false,
+                            iov[i - 1].iov_len);
+    }
+
+    /*
+     * Set status bits to indicate completion. Testing shows hardware sets
+     * these irrespective of HASH_IRQ_EN.
+     */
+    if (!rc) {
+        s->regs[R_STATUS] |= HASH_IRQ;
+    }
+
+    return rc;
+}
+
+
 
 static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
 {
@@ -187,11 +302,6 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
                           "%s: HMAC engine command mode %"PRIx64" not implemented",
                           __func__, (data & HASH_HMAC_MASK) >> 8);
         }
-        if (data & HASH_SG_EN) {
-            qemu_log_mask(LOG_UNIMP,
-                          "%s: Hash scatter gather mode not implemented",
-                          __func__);
-        }
         if (data & BIT(1)) {
             qemu_log_mask(LOG_UNIMP,
                           "%s: Cascaded mode not implemented",
@@ -204,7 +314,12 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
                         __func__, data & ahc->hash_mask);
                 break;
         }
-        do_hash_operation(s, algo);
+        if (data & HASH_SG_EN) {
+            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;
+            do_hash_sg_operation(s, algo);
+        } else {
+            do_hash_operation(s, algo);
+        }
 
         if (data & HASH_IRQ_EN) {
             qemu_irq_raise(s->irq);
diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
index 94d5ada95f..ead46afda9 100644
--- a/include/hw/misc/aspeed_hace.h
+++ b/include/hw/misc/aspeed_hace.h
@@ -40,4 +40,10 @@ struct AspeedHACEClass {
     uint32_t hash_mask;
 };
 
+#define ASPEED_HACE_MAX_SG      256
+struct aspeed_sg_list {
+        uint32_t len;
+        uint32_t phy_addr;
+} __attribute__ ((__packed__));
+
 #endif /* _ASPEED_HACE_H_ */
-- 
2.25.1

[PATCH 3/3] tests: Aspeed HACE Scatter-Gather tests

[Klaus Heinrich Kiwi]

Expand current Aspeed HACE testsuite to also include Scatter-Gather of
sha256 and sha512 operations.

Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
---
 tests/qtest/aspeed_hace-test.c | 164 ++++++++++++++++++++++++++++++---
 1 file changed, 153 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/aspeed_hace-test.c b/tests/qtest/aspeed_hace-test.c
index 2b624b6b09..ae3f963449 100644
--- a/tests/qtest/aspeed_hace-test.c
+++ b/tests/qtest/aspeed_hace-test.c
@@ -35,6 +35,11 @@
 #define HACE_HASH_DATA_LEN       0x2c
 #define HACE_HASH_CMD            0x30
 
+struct aspeed_sg_list {
+        uint32_t len;
+        uint32_t phy_addr;
+} __attribute__ ((__packed__));
+
 /*
  * Test vector is the ascii "abc"
  *
@@ -63,6 +68,33 @@ static const uint8_t test_result_md5[] = {
     0x90, 0x01, 0x50, 0x98, 0x3c, 0xd2, 0x4f, 0xb0, 0xd6, 0x96, 0x3f, 0x7d,
     0x28, 0xe1, 0x7f, 0x72};
 
+/*
+ * The Scatter-Gather Test vector is the ascii "abc" "def" "ghi", broken
+ * into blocks of 3 characters as shown
+ *
+ * Expected results were generated using command line utitiles:
+ *
+ *  echo -n -e 'abcdefghi' | dd of=/tmp/test
+ *  for hash in sha512sum sha256sum; do $hash /tmp/test; done
+ *
+ */
+static const uint8_t test_vector_sg1[] = {0x61, 0x62, 0x63};
+static const uint8_t test_vector_sg2[] = {0x64, 0x65, 0x66};
+static const uint8_t test_vector_sg3[] = {0x67, 0x68, 0x69};
+
+static const uint8_t test_result_sg_sha512[] = {
+    0xf2, 0x2d, 0x51, 0xd2, 0x52, 0x92, 0xca, 0x1d, 0x0f, 0x68, 0xf6, 0x9a,
+    0xed, 0xc7, 0x89, 0x70, 0x19, 0x30, 0x8c, 0xc9, 0xdb, 0x46, 0xef, 0xb7,
+    0x5a, 0x03, 0xdd, 0x49, 0x4f, 0xc7, 0xf1, 0x26, 0xc0, 0x10, 0xe8, 0xad,
+    0xe6, 0xa0, 0x0a, 0x0c, 0x1a, 0x5f, 0x1b, 0x75, 0xd8, 0x1e, 0x0e, 0xd5,
+    0xa9, 0x3c, 0xe9, 0x8d, 0xc9, 0xb8, 0x33, 0xdb, 0x78, 0x39, 0x24, 0x7b,
+    0x1d, 0x9c, 0x24, 0xfe};
+
+static const uint8_t test_result_sg_sha256[] = {
+    0x19, 0xcc, 0x02, 0xf2, 0x6d, 0xf4, 0x3c, 0xc5, 0x71, 0xbc, 0x9e, 0xd7,
+    0xb0, 0xc4, 0xd2, 0x92, 0x24, 0xa3, 0xec, 0x22, 0x95, 0x29, 0x22, 0x17,
+    0x25, 0xef, 0x76, 0xd0, 0x21, 0xc8, 0x32, 0x6f};
+
 
 static void write_regs(QTestState *s, uint32_t base, uint32_t src,
                        uint32_t length, uint32_t out, uint32_t method)
@@ -167,28 +199,124 @@ static void test_sha512(const char *machine, const uint32_t base,
                     test_result_sha512, sizeof(digest));
 }
 
+static void test_sha256_sg(const char *machine, const uint32_t base,
+                        const uint32_t src_addr)
+{
+    QTestState *s = qtest_init(machine);
+
+    const uint32_t src_addr_1 = src_addr + 0x1000000;
+    const uint32_t src_addr_2 = src_addr + 0x2000000;
+    const uint32_t src_addr_3 = src_addr + 0x3000000;
+    const uint32_t digest_addr = src_addr + 0x4000000;
+    uint8_t digest[32] = {0};
+    struct aspeed_sg_list array[] = {
+            { sizeof(test_vector_sg1),              src_addr_1},
+            { sizeof(test_vector_sg2),              src_addr_2},
+            { sizeof(test_vector_sg3) | 1u << 31,   src_addr_3},
+        };
+
+    /* Check engine is idle, no busy or irq bits set */
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+    /* Write test vector into memory */
+    qtest_memwrite(s, src_addr_1, test_vector_sg1, sizeof(test_vector_sg1));
+    qtest_memwrite(s, src_addr_2, test_vector_sg2, sizeof(test_vector_sg2));
+    qtest_memwrite(s, src_addr_3, test_vector_sg3, sizeof(test_vector_sg3));
+    qtest_memwrite(s, src_addr, array, sizeof(array));
+
+    write_regs(s, base, src_addr,
+               (sizeof(test_vector_sg1)
+                + sizeof(test_vector_sg2)
+                + sizeof(test_vector_sg3)),
+               digest_addr, HACE_ALGO_SHA256 | HACE_SG_EN);
+
+    /* Check hash IRQ status is asserted */
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0x00000200);
+
+    /* Clear IRQ status and check status is deasserted */
+    qtest_writel(s, base + HACE_STS, 0x00000200);
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+    /* Read computed digest from memory */
+    qtest_memread(s, digest_addr, digest, sizeof(digest));
+
+    /* Check result of computation */
+    g_assert_cmpmem(digest, sizeof(digest),
+                    test_result_sg_sha256, sizeof(digest));
+}
+
+static void test_sha512_sg(const char *machine, const uint32_t base,
+                        const uint32_t src_addr)
+{
+    QTestState *s = qtest_init(machine);
+
+    const uint32_t src_addr_1 = src_addr + 0x1000000;
+    const uint32_t src_addr_2 = src_addr + 0x2000000;
+    const uint32_t src_addr_3 = src_addr + 0x3000000;
+    const uint32_t digest_addr = src_addr + 0x4000000;
+    uint8_t digest[64] = {0};
+    struct aspeed_sg_list array[] = {
+            { sizeof(test_vector_sg1),              src_addr_1},
+            { sizeof(test_vector_sg2),              src_addr_2},
+            { sizeof(test_vector_sg3) | 1u << 31,   src_addr_3},
+        };
+
+    /* Check engine is idle, no busy or irq bits set */
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+    /* Write test vector into memory */
+    qtest_memwrite(s, src_addr_1, test_vector_sg1, sizeof(test_vector_sg1));
+    qtest_memwrite(s, src_addr_2, test_vector_sg2, sizeof(test_vector_sg2));
+    qtest_memwrite(s, src_addr_3, test_vector_sg3, sizeof(test_vector_sg3));
+    qtest_memwrite(s, src_addr, array, sizeof(array));
+
+    write_regs(s, base, src_addr,
+               (sizeof(test_vector_sg1)
+                + sizeof(test_vector_sg2)
+                + sizeof(test_vector_sg3)),
+               digest_addr, HACE_ALGO_SHA512 | HACE_SG_EN);
+
+    /* Check hash IRQ status is asserted */
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0x00000200);
+
+    /* Clear IRQ status and check status is deasserted */
+    qtest_writel(s, base + HACE_STS, 0x00000200);
+    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+    /* Read computed digest from memory */
+    qtest_memread(s, digest_addr, digest, sizeof(digest));
+
+    /* Check result of computation */
+    g_assert_cmpmem(digest, sizeof(digest),
+                    test_result_sg_sha512, sizeof(digest));
+}
+
 struct masks {
-    uint32_t src;
+    uint32_t src_direct;
+    uint32_t src_sg;
     uint32_t dest;
     uint32_t len;
 };
 
 static const struct masks ast2600_masks = {
-    .src  = 0x7fffffff,
-    .dest = 0x7ffffff8,
-    .len  = 0x0fffffff,
+    .src_direct  = 0x7fffffff,
+    .src_sg      = 0x7ffffff8,
+    .dest        = 0x7ffffff8,
+    .len         = 0x0fffffff,
 };
 
 static const struct masks ast2500_masks = {
-    .src  = 0x3fffffff,
-    .dest = 0x3ffffff8,
-    .len  = 0x0fffffff,
+    .src_direct  = 0x3fffffff,
+    .src_sg      = 0x0,         /* SG mode not supported */
+    .dest        = 0x3ffffff8,
+    .len         = 0x0fffffff,
 };
 
 static const struct masks ast2400_masks = {
-    .src  = 0x0fffffff,
-    .dest = 0x0ffffff8,
-    .len  = 0x0fffffff,
+    .src_direct  = 0x0fffffff,
+    .src_sg      = 0x0,         /* SG mode not supported */
+    .dest        = 0x0ffffff8,
+    .len         = 0x0fffffff,
 };
 
 static void test_addresses(const char *machine, const uint32_t base,
@@ -208,7 +336,8 @@ static void test_addresses(const char *machine, const uint32_t base,
 
     /* Check that the address masking is correct */
     qtest_writel(s, base + HACE_HASH_SRC, 0xffffffff);
-    g_assert_cmphex(qtest_readl(s, base + HACE_HASH_SRC), ==, expected->src);
+    g_assert_cmphex(qtest_readl(s, base + HACE_HASH_SRC), ==,
+                    expected->src_direct);
 
     qtest_writel(s, base + HACE_HASH_DIGEST, 0xffffffff);
     g_assert_cmphex(qtest_readl(s, base + HACE_HASH_DIGEST), ==, expected->dest);
@@ -238,11 +367,21 @@ static void test_sha256_ast2600(void)
     test_sha256("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
 }
 
+static void test_sha256_sg_ast2600(void)
+{
+    test_sha256_sg("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
+}
+
 static void test_sha512_ast2600(void)
 {
     test_sha512("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
 }
 
+static void test_sha512_sg_ast2600(void)
+{
+    test_sha512_sg("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
+}
+
 static void test_addresses_ast2600(void)
 {
     test_addresses("-machine ast2600-evb", 0x1e6d0000, &ast2600_masks);
@@ -299,6 +438,9 @@ int main(int argc, char **argv)
     qtest_add_func("ast2600/hace/sha256", test_sha256_ast2600);
     qtest_add_func("ast2600/hace/md5", test_md5_ast2600);
 
+    qtest_add_func("ast2600/hace/sha512_sg", test_sha512_sg_ast2600);
+    qtest_add_func("ast2600/hace/sha256_sg", test_sha256_sg_ast2600);
+
     qtest_add_func("ast2500/hace/addresses", test_addresses_ast2500);
     qtest_add_func("ast2500/hace/sha512", test_sha512_ast2500);
     qtest_add_func("ast2500/hace/sha256", test_sha256_ast2500);
-- 
2.25.1

Re: [PATCH 1/3] aspeed: Coding Style cleanups on do_hash_operation

[Cédric Le Goater]

On 3/24/21 11:38 PM, Klaus Heinrich Kiwi wrote:
> Basically using camelCase for some variables...

I don't think CamelCase applies to variables, only types.

  https://qemu.readthedocs.io/en/latest/devel/style.html#variable-naming-conventions

C.

 
> 
> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> ---
>  hw/misc/aspeed_hace.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 6e5b447a48..93313d2b80 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -86,24 +86,24 @@ static int hash_algo_lookup(uint32_t mask)
>  
>  static int do_hash_operation(AspeedHACEState *s, int algo)
>  {
> -    hwaddr src, len, dest;
> -    uint8_t *digest_buf = NULL;
> -    size_t digest_len = 0;
> -    char *src_buf;
> +    uint32_t src, len, dest;
> +    uint8_t *digestBuf = NULL;
> +    size_t digestLen = 0;
> +    char *srcBuf;
>      int rc;
>  
>      src = s->regs[R_HASH_SRC];
>      len = s->regs[R_HASH_SRC_LEN];
>      dest = s->regs[R_HASH_DEST];
>  
> -    src_buf = address_space_map(&s->dram_as, src, &len, false,
> -                                MEMTXATTRS_UNSPECIFIED);
> -    if (!src_buf) {
> +    srcBuf = address_space_map(&s->dram_as, src, (hwaddr *) &len,
> +                               false, MEMTXATTRS_UNSPECIFIED);
> +    if (!srcBuf) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map dram\n", __func__);
>          return -EACCES;
>      }
>  
> -    rc = qcrypto_hash_bytes(algo, src_buf, len, &digest_buf, &digest_len,
> +    rc = qcrypto_hash_bytes(algo, srcBuf, len, &digestBuf, &digestLen,
>                              &error_fatal);
>      if (rc < 0) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
> @@ -111,14 +111,14 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
>      }
>  
>      rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
> -                             digest_buf, digest_len);
> +                             digestBuf, digestLen);
>      if (rc) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: address space write failed\n", __func__);
>      }
> -    g_free(digest_buf);
> +    g_free(digestBuf);
>  
> -    address_space_unmap(&s->dram_as, src_buf, len, false, len);
> +    address_space_unmap(&s->dram_as, srcBuf, len, false, len);
>  
>      /*
>       * Set status bits to indicate completion. Testing shows hardware sets
> 

Re: [PATCH 1/3] aspeed: Coding Style cleanups on do_hash_operation

[Klaus Heinrich Kiwi]

Hi Cedric,

On 3/24/2021 7:57 PM, Cédric Le Goater wrote:
> On 3/24/21 11:38 PM, Klaus Heinrich Kiwi wrote:
>> Basically using camelCase for some variables...
> I don't think CamelCase applies to variables, only types.

Thanks, I think I mis-interpreted your comment on this.

Will re-factor the patches with that in mind, but let's see
if there are other comments first.

  -Klaus

-- 
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>

Re: [PATCH 3/3] tests: Aspeed HACE Scatter-Gather tests

[Joel Stanley]

On Wed, 24 Mar 2021 at 22:39, Klaus Heinrich Kiwi
<klaus@linux.vnet.ibm.com> wrote:
>
> Expand current Aspeed HACE testsuite to also include Scatter-Gather of
> sha256 and sha512 operations.
>
> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> ---
>  tests/qtest/aspeed_hace-test.c | 164 ++++++++++++++++++++++++++++++---
>  1 file changed, 153 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qtest/aspeed_hace-test.c b/tests/qtest/aspeed_hace-test.c
> index 2b624b6b09..ae3f963449 100644
> --- a/tests/qtest/aspeed_hace-test.c
> +++ b/tests/qtest/aspeed_hace-test.c
> @@ -35,6 +35,11 @@
>  #define HACE_HASH_DATA_LEN       0x2c
>  #define HACE_HASH_CMD            0x30
>
> +struct aspeed_sg_list {
> +        uint32_t len;
> +        uint32_t phy_addr;
> +} __attribute__ ((__packed__));
> +
>  /*
>   * Test vector is the ascii "abc"
>   *
> @@ -63,6 +68,33 @@ static const uint8_t test_result_md5[] = {
>      0x90, 0x01, 0x50, 0x98, 0x3c, 0xd2, 0x4f, 0xb0, 0xd6, 0x96, 0x3f, 0x7d,
>      0x28, 0xe1, 0x7f, 0x72};
>
> +/*
> + * The Scatter-Gather Test vector is the ascii "abc" "def" "ghi", broken
> + * into blocks of 3 characters as shown
> + *
> + * Expected results were generated using command line utitiles:
> + *
> + *  echo -n -e 'abcdefghi' | dd of=/tmp/test
> + *  for hash in sha512sum sha256sum; do $hash /tmp/test; done
> + *
> + */
> +static const uint8_t test_vector_sg1[] = {0x61, 0x62, 0x63};
> +static const uint8_t test_vector_sg2[] = {0x64, 0x65, 0x66};
> +static const uint8_t test_vector_sg3[] = {0x67, 0x68, 0x69};
> +
> +static const uint8_t test_result_sg_sha512[] = {
> +    0xf2, 0x2d, 0x51, 0xd2, 0x52, 0x92, 0xca, 0x1d, 0x0f, 0x68, 0xf6, 0x9a,
> +    0xed, 0xc7, 0x89, 0x70, 0x19, 0x30, 0x8c, 0xc9, 0xdb, 0x46, 0xef, 0xb7,
> +    0x5a, 0x03, 0xdd, 0x49, 0x4f, 0xc7, 0xf1, 0x26, 0xc0, 0x10, 0xe8, 0xad,
> +    0xe6, 0xa0, 0x0a, 0x0c, 0x1a, 0x5f, 0x1b, 0x75, 0xd8, 0x1e, 0x0e, 0xd5,
> +    0xa9, 0x3c, 0xe9, 0x8d, 0xc9, 0xb8, 0x33, 0xdb, 0x78, 0x39, 0x24, 0x7b,
> +    0x1d, 0x9c, 0x24, 0xfe};
> +
> +static const uint8_t test_result_sg_sha256[] = {
> +    0x19, 0xcc, 0x02, 0xf2, 0x6d, 0xf4, 0x3c, 0xc5, 0x71, 0xbc, 0x9e, 0xd7,
> +    0xb0, 0xc4, 0xd2, 0x92, 0x24, 0xa3, 0xec, 0x22, 0x95, 0x29, 0x22, 0x17,
> +    0x25, 0xef, 0x76, 0xd0, 0x21, 0xc8, 0x32, 0x6f};
> +
>
>  static void write_regs(QTestState *s, uint32_t base, uint32_t src,
>                         uint32_t length, uint32_t out, uint32_t method)
> @@ -167,28 +199,124 @@ static void test_sha512(const char *machine, const uint32_t base,
>                      test_result_sha512, sizeof(digest));
>  }
>
> +static void test_sha256_sg(const char *machine, const uint32_t base,
> +                        const uint32_t src_addr)
> +{
> +    QTestState *s = qtest_init(machine);
> +
> +    const uint32_t src_addr_1 = src_addr + 0x1000000;
> +    const uint32_t src_addr_2 = src_addr + 0x2000000;
> +    const uint32_t src_addr_3 = src_addr + 0x3000000;
> +    const uint32_t digest_addr = src_addr + 0x4000000;
> +    uint8_t digest[32] = {0};
> +    struct aspeed_sg_list array[] = {
> +            { sizeof(test_vector_sg1),              src_addr_1},
> +            { sizeof(test_vector_sg2),              src_addr_2},
> +            { sizeof(test_vector_sg3) | 1u << 31,   src_addr_3},

These sizeofs are always going to be 3.

I assume 1 << 31 is to indicate the final entry? Perhaps add a define for it.

> +        };
> +
> +    /* Check engine is idle, no busy or irq bits set */
> +    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
> +
> +    /* Write test vector into memory */
> +    qtest_memwrite(s, src_addr_1, test_vector_sg1, sizeof(test_vector_sg1));
> +    qtest_memwrite(s, src_addr_2, test_vector_sg2, sizeof(test_vector_sg2));
> +    qtest_memwrite(s, src_addr_3, test_vector_sg3, sizeof(test_vector_sg3));

It would simplify your test case if you wrote the test vector to the
one memory location.

> +    qtest_memwrite(s, src_addr, array, sizeof(array));
> +
> +    write_regs(s, base, src_addr,
> +               (sizeof(test_vector_sg1)
> +                + sizeof(test_vector_sg2)
> +                + sizeof(test_vector_sg3)),
> +               digest_addr, HACE_ALGO_SHA256 | HACE_SG_EN);
> +
> +    /* Check hash IRQ status is asserted */
> +    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0x00000200);
> +
> +    /* Clear IRQ status and check status is deasserted */
> +    qtest_writel(s, base + HACE_STS, 0x00000200);
> +    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
> +
> +    /* Read computed digest from memory */
> +    qtest_memread(s, digest_addr, digest, sizeof(digest));
> +
> +    /* Check result of computation */
> +    g_assert_cmpmem(digest, sizeof(digest),
> +                    test_result_sg_sha256, sizeof(digest));
> +}
> +


>  struct masks {
> -    uint32_t src;
> +    uint32_t src_direct;

You could leave this one the same.

> +    uint32_t src_sg;

You add this, but haven't written a new test to use it.

>      uint32_t dest;
>      uint32_t len;
>  };
>
>  static const struct masks ast2600_masks = {
> -    .src  = 0x7fffffff,
> -    .dest = 0x7ffffff8,
> -    .len  = 0x0fffffff,
> +    .src_direct  = 0x7fffffff,
> +    .src_sg      = 0x7ffffff8,
> +    .dest        = 0x7ffffff8,
> +    .len         = 0x0fffffff,
>  };
>
>  static const struct masks ast2500_masks = {
> -    .src  = 0x3fffffff,
> -    .dest = 0x3ffffff8,
> -    .len  = 0x0fffffff,
> +    .src_direct  = 0x3fffffff,
> +    .src_sg      = 0x0,         /* SG mode not supported */

There's no need to add it in this case.

> +    .dest        = 0x3ffffff8,
> +    .len         = 0x0fffffff,
>  };
>
>  static const struct masks ast2400_masks = {
> -    .src  = 0x0fffffff,
> -    .dest = 0x0ffffff8,
> -    .len  = 0x0fffffff,
> +    .src_direct  = 0x0fffffff,
> +    .src_sg      = 0x0,         /* SG mode not supported */
> +    .dest        = 0x0ffffff8,
> +    .len         = 0x0fffffff,
>  };
>
>  static void test_addresses(const char *machine, const uint32_t base,
> @@ -208,7 +336,8 @@ static void test_addresses(const char *machine, const uint32_t base,
>
>      /* Check that the address masking is correct */
>      qtest_writel(s, base + HACE_HASH_SRC, 0xffffffff);
> -    g_assert_cmphex(qtest_readl(s, base + HACE_HASH_SRC), ==, expected->src);
> +    g_assert_cmphex(qtest_readl(s, base + HACE_HASH_SRC), ==,
> +                    expected->src_direct);
>
>      qtest_writel(s, base + HACE_HASH_DIGEST, 0xffffffff);
>      g_assert_cmphex(qtest_readl(s, base + HACE_HASH_DIGEST), ==, expected->dest);
> @@ -238,11 +367,21 @@ static void test_sha256_ast2600(void)
>      test_sha256("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
>  }
>
> +static void test_sha256_sg_ast2600(void)
> +{
> +    test_sha256_sg("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
> +}
> +
>  static void test_sha512_ast2600(void)
>  {
>      test_sha512("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
>  }
>
> +static void test_sha512_sg_ast2600(void)
> +{
> +    test_sha512_sg("-machine ast2600-evb", 0x1e6d0000, 0x80000000);
> +}
> +
>  static void test_addresses_ast2600(void)
>  {
>      test_addresses("-machine ast2600-evb", 0x1e6d0000, &ast2600_masks);
> @@ -299,6 +438,9 @@ int main(int argc, char **argv)
>      qtest_add_func("ast2600/hace/sha256", test_sha256_ast2600);
>      qtest_add_func("ast2600/hace/md5", test_md5_ast2600);
>
> +    qtest_add_func("ast2600/hace/sha512_sg", test_sha512_sg_ast2600);
> +    qtest_add_func("ast2600/hace/sha256_sg", test_sha256_sg_ast2600);
> +
>      qtest_add_func("ast2500/hace/addresses", test_addresses_ast2500);
>      qtest_add_func("ast2500/hace/sha512", test_sha512_ast2500);
>      qtest_add_func("ast2500/hace/sha256", test_sha256_ast2500);
> --
> 2.25.1
>

Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Joel Stanley]

On Wed, 24 Mar 2021 at 22:39, Klaus Heinrich Kiwi
<klaus@linux.vnet.ibm.com> wrote:
>
> Complement the Aspeed HACE support with Scatter-Gather hash support for
> sha256 and sha512. Scatter-Gather is only supported on AST2600-series.

Please update the documentation at docs/system/arm/aspeed.rst too.

>
> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> ---
>  hw/misc/aspeed_hace.c         | 127 ++++++++++++++++++++++++++++++++--
>  include/hw/misc/aspeed_hace.h |   6 ++
>  2 files changed, 127 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 93313d2b80..8a37b1d961 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -57,6 +57,10 @@
>  /* Other cmd bits */
>  #define  HASH_IRQ_EN                    BIT(9)
>  #define  HASH_SG_EN                     BIT(18)
> +/* Scatter-gather data list */
> +#define  SG_LIST_LAST                   BIT(31)
> +#define  SG_LIST_LEN_MASK               0x7fffffff
> +#define  SG_LIST_ADDR_MASK              0x7ffffff8  /* 8-byte aligned */
>
>  static const struct {
>      uint32_t mask;
> @@ -129,6 +133,117 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
>      return 0;
>  }
>
> +static int do_hash_sg_operation(AspeedHACEState *s, int algo)
> +{
> +    uint32_t src, dest, reqSize;
> +    hwaddr len;
> +    const size_t reqLen = sizeof(struct aspeed_sg_list);

It would be more descriptive to use this sizeof where you need it,
instead of assigning to a constant.

> +    struct iovec iov[ASPEED_HACE_MAX_SG];
> +    unsigned int i = 0;
> +    unsigned int isLast = 0;

This sounds like it's a boolean.

> +    uint8_t *digestBuf = NULL;
> +    size_t digestLen = 0, size = 0;
> +    struct aspeed_sg_list *sgList;
> +    int rc;

This needs some work to match qemu coding style.

> +
> +    reqSize = s->regs[R_HASH_SRC_LEN];
> +    dest = s->regs[R_HASH_DEST];
> +
> +    while (!isLast && i < ASPEED_HACE_MAX_SG) {
> +        src = s->regs[R_HASH_SRC] + (i * reqLen);
> +        len = reqLen;
> +        sgList = (struct aspeed_sg_list *) address_space_map(&s->dram_as,

You can remove this cast as the function returns a void pointer.

> +                                                                     src,
> +                                                         (hwaddr *) &len,

You can remove this cast as the variable is already a hwaddr type.

> +                                                                   false,
> +                                                 MEMTXATTRS_UNSPECIFIED);

In the direct access code, we use address_space_map to save copying
the memory contents that is to be hashed. That's not the case for the
scatter gather list.

Instead of creating mappings to read the sg list, you could load the
addr, len pairs using address_space_ldl_le. This would give you the
pointer to create mappings for.

> +        if (!sgList) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s: failed to map dram for SG Array entry '%u' for address '0x%0x'\n",
> +             __func__, i, src);
> +            rc = -EACCES;
> +            goto cleanup;
> +        }
> +        if (len != reqLen)
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s:  Warning: dram map for SG array entry '%u' requested size '%lu' != mapped size '%lu'\n",
> +             __func__, i, reqLen, len);
> +
> +        isLast = sgList->len & SG_LIST_LAST;

You could drop the isLast variable, and perform this test at the
bottom of the while loop:

if (sgList->len & SG_LIST_LAST)
   break;

> +
> +        iov[i].iov_len = (hwaddr) (sgList->len & SG_LIST_LEN_MASK);
> +        iov[i].iov_base = address_space_map(&s->dram_as,
> +                            sgList->phy_addr & SG_LIST_ADDR_MASK,
> +                            &iov[i].iov_len, false,
> +                            MEMTXATTRS_UNSPECIFIED);
> +        if (!iov[i].iov_base) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s: failed to map dram for SG array entry '%u' for region '0x%x', len '%u'\n",
> +             __func__, i, sgList->phy_addr & SG_LIST_ADDR_MASK,
> +             sgList->len & SG_LIST_LEN_MASK);
> +            rc = -EACCES;
> +            goto cleanup;
> +        }
> +        if (iov[i].iov_len != (sgList->len & SG_LIST_LEN_MASK))
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s:  Warning: dram map for SG region entry %u requested size %u != mapped size %lu\n",
> +             __func__, i, (sgList->len & SG_LIST_LEN_MASK), iov[i].iov_len);
> +
> +
> +        address_space_unmap(&s->dram_as, (void *) sgList, len, false,
> +                            len);
> +        size += iov[i].iov_len;
> +        i++;
> +    }
> +
> +    if (!isLast) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                     "%s: Error: Exhausted maximum of '%u' SG array entries\n",
> +                     __func__, ASPEED_HACE_MAX_SG);
> +        rc = -ENOTSUP;
> +        goto cleanup;
> +    }
> +
> +    if (size != reqSize)
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +         "%s: Warning: requested SG total size %u != actual size %lu\n",
> +         __func__, reqSize, size);
> +
> +    rc = qcrypto_hash_bytesv(algo, iov, i, &digestBuf, &digestLen,
> +                            &error_fatal);
> +    if (rc < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n",
> +                      __func__);
> +        goto cleanup;
> +    }
> +
> +    rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
> +                             digestBuf, digestLen);
> +    if (rc)
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: address space write failed\n", __func__);
> +    g_free(digestBuf);
> +
> +cleanup:
> +
> +    for (; i > 0; i--) {
> +        address_space_unmap(&s->dram_as, iov[i - 1].iov_base,
> +                            iov[i - 1].iov_len, false,
> +                            iov[i - 1].iov_len);
> +    }
> +
> +    /*
> +     * Set status bits to indicate completion. Testing shows hardware sets
> +     * these irrespective of HASH_IRQ_EN.

This is the same comment from the direct method. Have you confirmed
this is true on hardware?

> +     */
> +    if (!rc) {
> +        s->regs[R_STATUS] |= HASH_IRQ;
> +    }
> +
> +    return rc;
> +}
> +
> +
>
>  static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
>  {
> @@ -187,11 +302,6 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                            "%s: HMAC engine command mode %"PRIx64" not implemented",
>                            __func__, (data & HASH_HMAC_MASK) >> 8);
>          }
> -        if (data & HASH_SG_EN) {
> -            qemu_log_mask(LOG_UNIMP,
> -                          "%s: Hash scatter gather mode not implemented",
> -                          __func__);
> -        }
>          if (data & BIT(1)) {
>              qemu_log_mask(LOG_UNIMP,
>                            "%s: Cascaded mode not implemented",
> @@ -204,7 +314,12 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                          __func__, data & ahc->hash_mask);
>                  break;
>          }
> -        do_hash_operation(s, algo);
> +        if (data & HASH_SG_EN) {
> +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;

This is setting (0x20 / 4) >> 2 == 2, which is Crypto Data
Destination. I suspect you wanted R_HASH_SRC, so you can omit the
right shift.

However I suggest you check that hardware masks the register when the
write occurs, and if it does, implement that in the write callback for
R_HASH_SRC. That way a guest code doing a read-write will see the
correct thing.

> +            do_hash_sg_operation(s, algo);
> +        } else {
> +            do_hash_operation(s, algo);
> +        }
>
>          if (data & HASH_IRQ_EN) {
>              qemu_irq_raise(s->irq);
> diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
> index 94d5ada95f..ead46afda9 100644
> --- a/include/hw/misc/aspeed_hace.h
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -40,4 +40,10 @@ struct AspeedHACEClass {
>      uint32_t hash_mask;
>  };
>
> +#define ASPEED_HACE_MAX_SG      256
> +struct aspeed_sg_list {
> +        uint32_t len;
> +        uint32_t phy_addr;

Does "phy" mean physical? If so, we could call it phys_addr to avoid
confusion with the addresses of PHYs.

Alternatively, call it 'addr'.

> +} __attribute__ ((__packed__));
> +
>  #endif /* _ASPEED_HACE_H_ */
> --
> 2.25.1
>

Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Cédric Le Goater]

On 3/24/21 11:38 PM, Klaus Heinrich Kiwi wrote:
> Complement the Aspeed HACE support with Scatter-Gather hash support for
> sha256 and sha512. Scatter-Gather is only supported on AST2600-series.
> 
> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>

this looks good. A few extra comments,

> ---
>  hw/misc/aspeed_hace.c         | 127 ++++++++++++++++++++++++++++++++--
>  include/hw/misc/aspeed_hace.h |   6 ++
>  2 files changed, 127 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 93313d2b80..8a37b1d961 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -57,6 +57,10 @@
>  /* Other cmd bits */
>  #define  HASH_IRQ_EN                    BIT(9)
>  #define  HASH_SG_EN                     BIT(18)
> +/* Scatter-gather data list */
> +#define  SG_LIST_LAST                   BIT(31)
> +#define  SG_LIST_LEN_MASK               0x7fffffff
> +#define  SG_LIST_ADDR_MASK              0x7ffffff8  /* 8-byte aligned */
>  
>  static const struct {
>      uint32_t mask;
> @@ -129,6 +133,117 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
>      return 0;
>  }
>  
> +static int do_hash_sg_operation(AspeedHACEState *s, int algo)

Do we really care about the return value ? 

> +{
> +    uint32_t src, dest, reqSize;
> +    hwaddr len;
> +    const size_t reqLen = sizeof(struct aspeed_sg_list);
> +    struct iovec iov[ASPEED_HACE_MAX_SG];
> +    unsigned int i = 0;
> +    unsigned int isLast = 0;
> +    uint8_t *digestBuf = NULL;
> +    size_t digestLen = 0, size = 0;
> +    struct aspeed_sg_list *sgList;
> +    int rc;
> +
> +    reqSize = s->regs[R_HASH_SRC_LEN];
> +    dest = s->regs[R_HASH_DEST];
> +
> +    while (!isLast && i < ASPEED_HACE_MAX_SG) {
> +        src = s->regs[R_HASH_SRC] + (i * reqLen);
> +        len = reqLen;
> +        sgList = (struct aspeed_sg_list *) address_space_map(&s->dram_as,
> +                                                                     src,
> +                                                         (hwaddr *) &len,
> +                                                                   false,
> +                                                 MEMTXATTRS_UNSPECIFIED);

This should be doing LE loads.

> +        if (!sgList) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s: failed to map dram for SG Array entry '%u' for address '0x%0x'\n",
> +             __func__, i, src);
> +            rc = -EACCES;
> +            goto cleanup;
> +        }
> +        if (len != reqLen)
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s:  Warning: dram map for SG array entry '%u' requested size '%lu' != mapped size '%lu'\n",
> +             __func__, i, reqLen, len);
> +
> +        isLast = sgList->len & SG_LIST_LAST;
> +
> +        iov[i].iov_len = (hwaddr) (sgList->len & SG_LIST_LEN_MASK);
> +        iov[i].iov_base = address_space_map(&s->dram_as,
> +                            sgList->phy_addr & SG_LIST_ADDR_MASK,
> +                            &iov[i].iov_len, false,
> +                            MEMTXATTRS_UNSPECIFIED);
> +        if (!iov[i].iov_base) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s: failed to map dram for SG array entry '%u' for region '0x%x', len '%u'\n",
> +             __func__, i, sgList->phy_addr & SG_LIST_ADDR_MASK,
> +             sgList->len & SG_LIST_LEN_MASK);
> +            rc = -EACCES;
> +            goto cleanup;
> +        }
> +        if (iov[i].iov_len != (sgList->len & SG_LIST_LEN_MASK))
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +             "%s:  Warning: dram map for SG region entry %u requested size %u != mapped size %lu\n",
> +             __func__, i, (sgList->len & SG_LIST_LEN_MASK), iov[i].iov_len);
> +
> +
> +        address_space_unmap(&s->dram_as, (void *) sgList, len, false,
> +                            len);
> +        size += iov[i].iov_len;
> +        i++;
> +    }
> +
> +    if (!isLast) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                     "%s: Error: Exhausted maximum of '%u' SG array entries\n",
> +                     __func__, ASPEED_HACE_MAX_SG);
> +        rc = -ENOTSUP;
> +        goto cleanup;
> +    }
> +
> +    if (size != reqSize)
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +         "%s: Warning: requested SG total size %u != actual size %lu\n",
> +         __func__, reqSize, size);
> +
> +    rc = qcrypto_hash_bytesv(algo, iov, i, &digestBuf, &digestLen,
> +                            &error_fatal);
> +    if (rc < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n",
> +                      __func__);
> +        goto cleanup;
> +    }
> +
> +    rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
> +                             digestBuf, digestLen);
> +    if (rc)
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: address space write failed\n", __func__);
> +    g_free(digestBuf);
> +
> +cleanup:
> +
> +    for (; i > 0; i--) {
> +        address_space_unmap(&s->dram_as, iov[i - 1].iov_base,
> +                            iov[i - 1].iov_len, false,
> +                            iov[i - 1].iov_len);
> +    }
> +
> +    /*
> +     * Set status bits to indicate completion. Testing shows hardware sets
> +     * these irrespective of HASH_IRQ_EN.
> +     */
> +    if (!rc) {
> +        s->regs[R_STATUS] |= HASH_IRQ;
> +    }
> +
> +    return rc;
> +}
> +
> +
>  
>  static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
>  {
> @@ -187,11 +302,6 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                            "%s: HMAC engine command mode %"PRIx64" not implemented",
>                            __func__, (data & HASH_HMAC_MASK) >> 8);
>          }
> -        if (data & HASH_SG_EN) {
> -            qemu_log_mask(LOG_UNIMP,
> -                          "%s: Hash scatter gather mode not implemented",
> -                          __func__);
> -        }
>          if (data & BIT(1)) {
>              qemu_log_mask(LOG_UNIMP,
>                            "%s: Cascaded mode not implemented",
> @@ -204,7 +314,12 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                          __func__, data & ahc->hash_mask);
>                  break;
>          }
> -        do_hash_operation(s, algo);
> +        if (data & HASH_SG_EN) {
> +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;

I think Joel introduced a class mask for this value.

> +            do_hash_sg_operation(s, algo);
> +        } else {
> +            do_hash_operation(s, algo);
> +        }
>  
>          if (data & HASH_IRQ_EN) {
>              qemu_irq_raise(s->irq);
> diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
> index 94d5ada95f..ead46afda9 100644
> --- a/include/hw/misc/aspeed_hace.h
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -40,4 +40,10 @@ struct AspeedHACEClass {
>      uint32_t hash_mask;
>  };
>  
> +#define ASPEED_HACE_MAX_SG      256
> +struct aspeed_sg_list {
> +        uint32_t len;
> +        uint32_t phy_addr;
> +} __attribute__ ((__packed__));
> +

May be keep the definition in the .c file 

Thanks,

C. 

>  #endif /* _ASPEED_HACE_H_ */
> 

Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Klaus Heinrich Kiwi]

Hi Joel,

On 3/25/2021 12:40 AM, Joel Stanley wrote:
> On Wed, 24 Mar 2021 at 22:39, Klaus Heinrich Kiwi
> <klaus@linux.vnet.ibm.com> wrote:
>>
>> Complement the Aspeed HACE support with Scatter-Gather hash support for
>> sha256 and sha512. Scatter-Gather is only supported on AST2600-series.
> 
> Please update the documentation at docs/system/arm/aspeed.rst too.
>
I've removed the 'no scatter-gather' from the doc.

>> +                                                                   false,
>> +                                                 MEMTXATTRS_UNSPECIFIED);
> 
> In the direct access code, we use address_space_map to save copying
> the memory contents that is to be hashed. That's not the case for the
> scatter gather list.
> 
> Instead of creating mappings to read the sg list, you could load the
> addr, len pairs using address_space_ldl_le. This would give you the
> pointer to create mappings for.

I've reworked the code to use address_space_ldl_le, also removed the redundant
isLast variable.

  
>> +    /*
>> +     * Set status bits to indicate completion. Testing shows hardware sets
>> +     * these irrespective of HASH_IRQ_EN.
> 
> This is the same comment from the direct method. Have you confirmed
> this is true on hardware?
> 
Yes, I was able to confirm that on real hardware (AST2600-A1)

>> -        do_hash_operation(s, algo);
>> +        if (data & HASH_SG_EN) {
>> +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;
> 
> This is setting (0x20 / 4) >> 2 == 2, which is Crypto Data
> Destination. I suspect you wanted R_HASH_SRC, so you can omit the
> right shift.
> 
> However I suggest you check that hardware masks the register when the
> write occurs, and if it does, implement that in the write callback for
> R_HASH_SRC. That way a guest code doing a read-write will see the
> correct thing.

I was able to check on real hardware that, even when requesting a
HASH_SG_EN operation, the masking on the src address register is only
0x7fffffff, so I removed the masking specific to HASH_SG_EN.


>>   };
>>
>> +#define ASPEED_HACE_MAX_SG      256
>> +struct aspeed_sg_list {
>> +        uint32_t len;
>> +        uint32_t phy_addr;
> 
> Does "phy" mean physical? If so, we could call it phys_addr to avoid
> confusion with the addresses of PHYs.
> 
> Alternatively, call it 'addr'.

Since I'm not using address_map_ldl_le to access these, I decided to
do so based on #defines offsets, so I no longer need a Struct here.


Will send a V2 soon.

Thanks,

  -Klaus
-- 
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>

Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Klaus Heinrich Kiwi]

On 3/25/2021 4:55 AM, Cédric Le Goater wrote:
> On 3/24/21 11:38 PM, Klaus Heinrich Kiwi wrote:
>> Complement the Aspeed HACE support with Scatter-Gather hash support for
>> sha256 and sha512. Scatter-Gather is only supported on AST2600-series.
>>
>> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> 
> this looks good. A few extra comments,
> 
>
>> @@ -129,6 +133,117 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
>>       return 0;
>>   }
>>   
>> +static int do_hash_sg_operation(AspeedHACEState *s, int algo)
> 
> Do we really care about the return value ?

I'm keeping it consistent with do_hash_operation() above it. Maybe the underlying
Qemu layers could use it?

  
>> +        sgList = (struct aspeed_sg_list *) address_space_map(&s->dram_as,
>> +                                                                     src,
>> +                                                         (hwaddr *) &len,
>> +                                                                   false,
>> +                                                 MEMTXATTRS_UNSPECIFIED);
> 
> This should be doing LE loads.

ack. I'm using address_space_ldl_le() now.

  
>> -        do_hash_operation(s, algo);
>> +        if (data & HASH_SG_EN) {
>> +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;
> 
> I think Joel introduced a class mask for this value.

Turns out that the hardware doesn't do any additional masking on the src
register for the HASH_SG_EN operation, so I'll just remove it.

  
>>   
>> +#define ASPEED_HACE_MAX_SG      256
>> +struct aspeed_sg_list {
>> +        uint32_t len;
>> +        uint32_t phy_addr;
>> +} __attribute__ ((__packed__));
>> +> 
> May be keep the definition in the .c file

I actually opted to use #define offsets now that I'm using ldl_le

Thanks,

  -Klaus

-- 
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>

Re: [PATCH 3/3] tests: Aspeed HACE Scatter-Gather tests

[Klaus Heinrich Kiwi]

Hi Joel,


On 3/24/2021 11:18 PM, Joel Stanley wrote:

>> +    const uint32_t src_addr_1 = src_addr + 0x1000000;
>> +    const uint32_t src_addr_2 = src_addr + 0x2000000;
>> +    const uint32_t src_addr_3 = src_addr + 0x3000000;
>> +    const uint32_t digest_addr = src_addr + 0x4000000;
>> +    uint8_t digest[32] = {0};
>> +    struct aspeed_sg_list array[] = {
>> +            { sizeof(test_vector_sg1),              src_addr_1},
>> +            { sizeof(test_vector_sg2),              src_addr_2},
>> +            { sizeof(test_vector_sg3) | 1u << 31,   src_addr_3},
> 
> These sizeofs are always going to be 3.

I was trying to not hard-code 3 here and perhaps allow for more clarity and
extensibility, in case we ever decide to use larger vectors?

  
> I assume 1 << 31 is to indicate the final entry? Perhaps add a define for it.
ack

> 
>> +        };
>> +
>> +    /* Check engine is idle, no busy or irq bits set */
>> +    g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
>> +
>> +    /* Write test vector into memory */
>> +    qtest_memwrite(s, src_addr_1, test_vector_sg1, sizeof(test_vector_sg1));
>> +    qtest_memwrite(s, src_addr_2, test_vector_sg2, sizeof(test_vector_sg2));
>> +    qtest_memwrite(s, src_addr_3, test_vector_sg3, sizeof(test_vector_sg3));
> 
> It would simplify your test case if you wrote the test vector to the
> one memory location.

I guess I like that each vector is significantly displaced from each other, as this
is supported by the hardware.

> 
>>   struct masks {
>> -    uint32_t src;
>> +    uint32_t src_direct;
> 
> You could leave this one the same.
> 
>> +    uint32_t src_sg;
> 
> You add this, but haven't written a new test to use it.

Turns out there's nothing special about src/dst/len masking
when using HASH_SG_EN, so I'll remove those.

Thanks,

  -Klaus

-- 
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>