* [PATCH 0/2] hw/misc: Model ASPEED hash and crpyto engine
@ 2021-03-03 7:03 Joel Stanley
2021-03-03 7:03 ` [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine Joel Stanley
2021-03-03 7:03 ` [PATCH 2/2] aspeed: Integrate HACE Joel Stanley
0 siblings, 2 replies; 7+ messages in thread
From: Joel Stanley @ 2021-03-03 7:03 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Andrew Jeffery, Peter Maydell, qemu-arm, qemu-devel
This adds a model for the ASPEED hash and crypto engine (HACE) found on
all supported ASPEED SoCs.
The model uses Qemu's gcrypto API to perform the SHA and MD5 hashing
directly in the machine's emulated memory space, which I found a neat
use of Qemu's features.
It has been tested using u-boot and from Linux userspace.
Joel Stanley (2):
hw: Model ASPEED's Hash and Crypto Engine
aspeed: Integrate HACE
docs/system/arm/aspeed.rst | 2 +-
include/hw/arm/aspeed_soc.h | 3 +
include/hw/misc/aspeed_hace.h | 33 ++++
hw/arm/aspeed_ast2600.c | 14 ++
hw/arm/aspeed_soc.c | 15 ++
hw/misc/aspeed_hace.c | 302 ++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 2 +-
7 files changed, 369 insertions(+), 2 deletions(-)
create mode 100644 include/hw/misc/aspeed_hace.h
create mode 100644 hw/misc/aspeed_hace.c
--
2.30.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine
2021-03-03 7:03 [PATCH 0/2] hw/misc: Model ASPEED hash and crpyto engine Joel Stanley
@ 2021-03-03 7:03 ` Joel Stanley
2021-03-09 1:01 ` Andrew Jeffery
2021-03-09 9:40 ` Philippe Mathieu-Daudé
2021-03-03 7:03 ` [PATCH 2/2] aspeed: Integrate HACE Joel Stanley
1 sibling, 2 replies; 7+ messages in thread
From: Joel Stanley @ 2021-03-03 7:03 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Andrew Jeffery, Peter Maydell, qemu-arm, qemu-devel
The HACE (Hash and Crpyto Engine) is a device that offloads MD5, SHA1,
SHA2, RSA and other cryptographic algorithms.
This initial model implements a subset of the device's functionality;
currently only direct access (non-scatter gather) hashing.
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/misc/aspeed_hace.h | 33 ++++
hw/misc/aspeed_hace.c | 302 ++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 2 +-
3 files changed, 336 insertions(+), 1 deletion(-)
create mode 100644 include/hw/misc/aspeed_hace.h
create mode 100644 hw/misc/aspeed_hace.c
diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
new file mode 100644
index 000000000000..e1fce670ef9e
--- /dev/null
+++ b/include/hw/misc/aspeed_hace.h
@@ -0,0 +1,33 @@
+/*
+ * ASPEED Hash and Crypto Engine
+ *
+ * Copyright (C) 2021 IBM Corp.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_HACE_H
+#define ASPEED_HACE_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_HACE "aspeed.hace"
+#define ASPEED_HACE(obj) OBJECT_CHECK(AspeedHACEState, (obj), TYPE_ASPEED_HACE)
+
+#define ASPEED_HACE_NR_REGS (0x64 >> 2)
+
+typedef struct AspeedHACEState {
+ /* <private> */
+ SysBusDevice parent;
+
+ /*< public >*/
+ MemoryRegion iomem;
+ qemu_irq irq;
+
+ uint32_t regs[ASPEED_HACE_NR_REGS];
+
+ MemoryRegion *dram_mr;
+ AddressSpace dram_as;
+} AspeedHACEState;
+
+#endif /* _ASPEED_HACE_H_ */
diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
new file mode 100644
index 000000000000..0e402a0adea9
--- /dev/null
+++ b/hw/misc/aspeed_hace.c
@@ -0,0 +1,302 @@
+/*
+ * ASPEED Hash and Crypto Engine
+ *
+ * Copyright (C) 2021 IBM Corp.
+ *
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_hace.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "crypto/hash.h"
+#include "hw/qdev-properties.h"
+#include "hw/irq.h"
+
+#define R_STATUS (0x1c / 4)
+#define HASH_IRQ BIT(9)
+#define CRYPT_IRQ BIT(12)
+#define TAG_IRQ BIT(15)
+
+#define R_HASH_CMD (0x30 / 4)
+/* Hash algorithim selection */
+#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
+#define HASH_ALGO_MD5 0
+#define HASH_ALGO_SHA1 BIT(5)
+#define HASH_ALGO_SHA224 BIT(6)
+#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
+#define HASH_ALGO_SHA512_SERIES (BIT(5) | BIT(6))
+/* SHA512 algorithim selection */
+#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
+#define HASH_ALGO_SHA512_SHA512 0
+#define HASH_ALGO_SHA512_SHA384 BIT(10)
+#define HASH_ALGO_SHA512_SHA256 BIT(11)
+#define HASH_ALGO_SHA512_SHA224 (BIT(10) | BIT(11))
+/* HMAC modes */
+#define HASH_HMAC_MASK (BIT(7) | BIT(8))
+#define HASH_DIGEST 0
+#define HASH_DIGEST_HMAC BIT(7)
+#define HASH_DIGEST_ACCUM BIT(8)
+#define HASH_HMAC_KEY (BIT(7) | BIT(8))
+/* Cascscaed operation modes */
+#define HASH_ONLY 0
+#define HASH_ONLY2 BIT(0)
+#define HASH_CRYPT_THEN_HASH BIT(1)
+#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
+/* Other cmd bits */
+#define HASH_IRQ_EN BIT(9)
+#define HASH_SG_EN BIT(18)
+
+#define R_CRYPT_CMD (0x10 / 4)
+
+#define R_HASH_SRC (0x20 / 4)
+#define R_HASH_DEST (0x24 / 4)
+#define R_HASH_SRC_LEN (0x2c / 4)
+
+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;
+ 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) {
+ 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,
+ &error_fatal);
+ if (rc < 0) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
+ return rc;
+ }
+
+ rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
+ digest_buf, digest_len);
+ if (rc) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: address space write failed\n", __func__);
+ }
+ g_free(digest_buf);
+
+ address_space_unmap(&s->dram_as, src_buf, len, false, len);
+
+ /*
+ * Set status bits to indicate completion. Testing shows hardware sets
+ * these irrespective of HASH_IRQ_EN.
+ */
+ s->regs[R_STATUS] |= HASH_IRQ;
+
+ return 0;
+}
+
+
+static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
+{
+ AspeedHACEState *s = ASPEED_HACE(opaque);
+
+ addr >>= 2;
+
+ if (addr >= ASPEED_HACE_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+ __func__, addr << 2);
+ return 0;
+ }
+
+ return s->regs[addr];
+}
+
+static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned int size)
+{
+ AspeedHACEState *s = ASPEED_HACE(opaque);
+
+ addr >>= 2;
+
+ if (addr >= ASPEED_HACE_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+ __func__, addr << 2);
+ return;
+ }
+
+ switch (addr) {
+ case R_STATUS:
+ if (data & HASH_IRQ) {
+ data &= ~HASH_IRQ;
+
+ if (s->regs[addr] & HASH_IRQ) {
+ qemu_irq_lower(s->irq);
+ }
+ }
+ break;
+ case R_HASH_CMD: {
+ int algo = -1;
+ if ((data & HASH_HMAC_MASK)) {
+ qemu_log_mask(LOG_UNIMP,
+ "%s: HMAC engine command mode %ld 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",
+ __func__);
+ }
+ switch (data & HASH_ALGO_MASK) {
+ case HASH_ALGO_MD5:
+ algo = QCRYPTO_HASH_ALG_MD5;
+ break;
+ case HASH_ALGO_SHA1:
+ algo = QCRYPTO_HASH_ALG_SHA1;
+ break;
+ case HASH_ALGO_SHA224:
+ algo = QCRYPTO_HASH_ALG_SHA224;
+ break;
+ case HASH_ALGO_SHA256:
+ algo = QCRYPTO_HASH_ALG_SHA256;
+ break;
+ case HASH_ALGO_SHA512_SERIES:
+ switch (data & SHA512_HASH_ALGO_MASK) {
+ case HASH_ALGO_SHA512_SHA512:
+ algo = QCRYPTO_HASH_ALG_SHA512;
+ break;
+ case HASH_ALGO_SHA512_SHA384:
+ algo = QCRYPTO_HASH_ALG_SHA384;
+ break;
+ case HASH_ALGO_SHA512_SHA256:
+ algo = QCRYPTO_HASH_ALG_SHA256;
+ break;
+ case HASH_ALGO_SHA512_SHA224:
+ algo = QCRYPTO_HASH_ALG_SHA224;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid sha512 hash algorithm selection 0x%03lx\n",
+ __func__, data & SHA512_HASH_ALGO_MASK);
+ break;
+ }
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid hash algorithm selection 0x%03lx\n",
+ __func__, data & HASH_ALGO_MASK);
+ break;
+ }
+ if (algo >= 0) {
+ do_hash_operation(s, algo);
+
+ if (data & HASH_IRQ_EN) {
+ qemu_irq_raise(s->irq);
+ }
+ }
+
+ break;
+ }
+ case R_CRYPT_CMD:
+ qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not implemented\n",
+ __func__);
+ break;
+ default:
+ break;
+ }
+
+ s->regs[addr] = data;
+}
+
+static const MemoryRegionOps aspeed_hace_ops = {
+ .read = aspeed_hace_read,
+ .write = aspeed_hace_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ },
+};
+
+static void aspeed_hace_reset(DeviceState *dev)
+{
+ struct AspeedHACEState *s = ASPEED_HACE(dev);
+
+ memset(s->regs, 0, sizeof(s->regs));
+}
+
+static void aspeed_hace_realize(DeviceState *dev, Error **errp)
+{
+ AspeedHACEState *s = ASPEED_HACE(dev);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+ sysbus_init_irq(sbd, &s->irq);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_hace_ops, s,
+ TYPE_ASPEED_HACE, 0x1000);
+
+ if (!s->dram_mr) {
+ error_setg(errp, TYPE_ASPEED_HACE ": 'dram' link not set");
+ return;
+ }
+
+ address_space_init(&s->dram_as, s->dram_mr, "dram");
+
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static Property aspeed_hace_properties[] = {
+ DEFINE_PROP_LINK("dram", AspeedHACEState, dram_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+
+static const VMStateDescription vmstate_aspeed_hace = {
+ .name = TYPE_ASPEED_HACE,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(regs, AspeedHACEState, ASPEED_HACE_NR_REGS),
+ VMSTATE_END_OF_LIST(),
+ }
+};
+
+static void aspeed_hace_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = aspeed_hace_realize;
+ dc->reset = aspeed_hace_reset;
+ device_class_set_props(dc, aspeed_hace_properties);
+ dc->desc = "Aspeed Hash and Crypto Engine",
+ dc->vmsd = &vmstate_aspeed_hace;
+}
+
+static const TypeInfo aspeed_hace_info = {
+ .name = TYPE_ASPEED_HACE,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedHACEState),
+ .class_init = aspeed_hace_class_init,
+};
+
+static void aspeed_hace_register_types(void)
+{
+ type_register_static(&aspeed_hace_info);
+}
+
+type_init(aspeed_hace_register_types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 629283957fcc..e0f55b37f882 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -102,7 +102,7 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c'))
softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
-softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
+softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_hace.c', 'aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
--
2.30.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] aspeed: Integrate HACE
2021-03-03 7:03 [PATCH 0/2] hw/misc: Model ASPEED hash and crpyto engine Joel Stanley
2021-03-03 7:03 ` [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine Joel Stanley
@ 2021-03-03 7:03 ` Joel Stanley
2021-03-09 1:07 ` Andrew Jeffery
1 sibling, 1 reply; 7+ messages in thread
From: Joel Stanley @ 2021-03-03 7:03 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Andrew Jeffery, Peter Maydell, qemu-arm, qemu-devel
Add the hash and crypto engine model to the aspeed socs.
Signed-off-by: Joel Stanley <joel@jms.id.au>
[ clg: documentation update ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
docs/system/arm/aspeed.rst | 2 +-
include/hw/arm/aspeed_soc.h | 3 +++
hw/arm/aspeed_ast2600.c | 14 ++++++++++++++
hw/arm/aspeed_soc.c | 15 +++++++++++++++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 690bada7842b..ec30cad88a58 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -48,6 +48,7 @@ Supported devices
* UART
* Ethernet controllers
* Front LEDs (PCA9552 on I2C bus)
+ * Hash/Crypto Engine (HACE) - SHA support only, no scatter-gather
Missing devices
@@ -59,7 +60,6 @@ Missing devices
* LPC Bus Controller
* Slave GPIO Controller
* Super I/O Controller
- * Hash/Crypto Engine
* PCI-Express 1 Controller
* Graphic Display Controller
* PECI Controller
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 11cfe6e3585b..a8c9a22e5882 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -21,6 +21,7 @@
#include "hw/rtc/aspeed_rtc.h"
#include "hw/i2c/aspeed_i2c.h"
#include "hw/ssi/aspeed_smc.h"
+#include "hw/misc/aspeed_hace.h"
#include "hw/watchdog/wdt_aspeed.h"
#include "hw/net/ftgmac100.h"
#include "target/arm/cpu.h"
@@ -49,6 +50,7 @@ struct AspeedSoCState {
AspeedTimerCtrlState timerctrl;
AspeedI2CState i2c;
AspeedSCUState scu;
+ AspeedHACEState hace;
AspeedXDMAState xdma;
AspeedSMCState fmc;
AspeedSMCState spi[ASPEED_SPIS_NUM];
@@ -130,6 +132,7 @@ enum {
ASPEED_DEV_SDRAM,
ASPEED_DEV_XDMA,
ASPEED_DEV_EMMC,
+ ASPEED_DEV_HACE,
};
#endif /* ASPEED_SOC_H */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bf31ca351feb..7aba3effd7bc 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -42,6 +42,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_ETH4] = 0x1E690000,
[ASPEED_DEV_VIC] = 0x1E6C0000,
+ [ASPEED_DEV_HACE] = 0x1E6D0000,
[ASPEED_DEV_SDMC] = 0x1E6E0000,
[ASPEED_DEV_SCU] = 0x1E6E2000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
@@ -102,6 +103,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
+ [ASPEED_DEV_HACE] = 4,
[ASPEED_DEV_ETH3] = 32,
[ASPEED_DEV_ETH4] = 33,
@@ -211,6 +213,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "emmc-controller.sdhci", &s->emmc.slots[0],
TYPE_SYSBUS_SDHCI);
+
+ object_initialize_child(obj, "hace", &s->hace, TYPE_ASPEED_HACE);
}
/*
@@ -469,6 +473,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_mmio_map(SYS_BUS_DEVICE(&s->emmc), 0, sc->memmap[ASPEED_DEV_EMMC]);
sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
+ /* HACE */
+ object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(s->dram_mr),
+ &error_abort);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->hace), 0, sc->memmap[ASPEED_DEV_HACE]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07a..ce9b8f8c5d6f 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -34,6 +34,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
[ASPEED_DEV_VIC] = 0x1E6C0000,
[ASPEED_DEV_SDMC] = 0x1E6E0000,
[ASPEED_DEV_SCU] = 0x1E6E2000,
+ [ASPEED_DEV_HACE] = 0x1E6E3000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
[ASPEED_DEV_VIDEO] = 0x1E700000,
[ASPEED_DEV_ADC] = 0x1E6E9000,
@@ -65,6 +66,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
[ASPEED_DEV_VIC] = 0x1E6C0000,
[ASPEED_DEV_SDMC] = 0x1E6E0000,
[ASPEED_DEV_SCU] = 0x1E6E2000,
+ [ASPEED_DEV_HACE] = 0x1E6E3000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
[ASPEED_DEV_ADC] = 0x1E6E9000,
[ASPEED_DEV_VIDEO] = 0x1E700000,
@@ -118,6 +120,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_XDMA] = 6,
[ASPEED_DEV_SDHCI] = 26,
+ [ASPEED_DEV_HACE] = 4,
};
#define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
@@ -211,6 +214,8 @@ static void aspeed_soc_init(Object *obj)
object_initialize_child(obj, "sdhci[*]", &s->sdhci.slots[i],
TYPE_SYSBUS_SDHCI);
}
+
+ object_initialize_child(obj, "hace", &s->hace, TYPE_ASPEED_HACE);
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -393,6 +398,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
sc->memmap[ASPEED_DEV_SDHCI]);
sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
+ /* HACE */
+ object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(s->dram_mr),
+ &error_abort);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->hace), 0, sc->memmap[ASPEED_DEV_HACE]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
--
2.30.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine
2021-03-03 7:03 ` [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine Joel Stanley
@ 2021-03-09 1:01 ` Andrew Jeffery
2021-03-11 6:17 ` Joel Stanley
2021-03-09 9:40 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Jeffery @ 2021-03-09 1:01 UTC (permalink / raw)
To: Joel Stanley, Cédric Le Goater
Cc: Peter Maydell, qemu-arm, Cameron Esfahani via
On Wed, 3 Mar 2021, at 17:33, Joel Stanley wrote:
> The HACE (Hash and Crpyto Engine) is a device that offloads MD5, SHA1,
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/misc/aspeed_hace.h | 33 ++++
> hw/misc/aspeed_hace.c | 302 ++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 2 +-
> 3 files changed, 336 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
>
> diff --git a/include/hw/misc/aspeed_hace.h
> b/include/hw/misc/aspeed_hace.h
> new file mode 100644
> index 000000000000..e1fce670ef9e
> --- /dev/null
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -0,0 +1,33 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ASPEED_HACE_H
> +#define ASPEED_HACE_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_HACE "aspeed.hace"
> +#define ASPEED_HACE(obj) OBJECT_CHECK(AspeedHACEState, (obj),
> TYPE_ASPEED_HACE)
> +
> +#define ASPEED_HACE_NR_REGS (0x64 >> 2)
0x64 is the offset of the last defined register, so this should be (0x68 >> 2)
> +
> +typedef struct AspeedHACEState {
> + /* <private> */
> + SysBusDevice parent;
> +
> + /*< public >*/
> + MemoryRegion iomem;
> + qemu_irq irq;
> +
> + uint32_t regs[ASPEED_HACE_NR_REGS];
> +
> + MemoryRegion *dram_mr;
> + AddressSpace dram_as;
> +} AspeedHACEState;
> +
> +#endif /* _ASPEED_HACE_H_ */
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> new file mode 100644
> index 000000000000..0e402a0adea9
> --- /dev/null
> +++ b/hw/misc/aspeed_hace.c
> @@ -0,0 +1,302 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_hace.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "crypto/hash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/irq.h"
> +
> +#define R_STATUS (0x1c / 4)
> +#define HASH_IRQ BIT(9)
> +#define CRYPT_IRQ BIT(12)
> +#define TAG_IRQ BIT(15)
> +
> +#define R_HASH_CMD (0x30 / 4)
> +/* Hash algorithim selection */
> +#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
Hmm, feels GENMASK()-y, but looks like the tree is in a bit of a weird
state in that respect:
$ git grep GENMASK
include/hw/usb/dwc2-regs.h:#define GSNPSID_ID_MASK GENMASK(31, 16)
include/standard-headers/asm-x86/kvm_para.h:#define KVM_ASYNC_PF_VEC_MASK GENMASK(7, 0)
$
> +#define HASH_ALGO_MD5 0
> +#define HASH_ALGO_SHA1 BIT(5)
> +#define HASH_ALGO_SHA224 BIT(6)
> +#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
Not something you need to fix necessarily, but it would feel more
intuitive to me to order these MSB to LSB, so e.g. (BIT(6) | BIT(4)).
That way I can "see" the value without having to reverse the bits.
> +#define HASH_ALGO_SHA512_SERIES (BIT(5) | BIT(6))
> +/* SHA512 algorithim selection */
> +#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
> +#define HASH_ALGO_SHA512_SHA512 0
> +#define HASH_ALGO_SHA512_SHA384 BIT(10)
> +#define HASH_ALGO_SHA512_SHA256 BIT(11)
> +#define HASH_ALGO_SHA512_SHA224 (BIT(10) | BIT(11))
> +/* HMAC modes */
> +#define HASH_HMAC_MASK (BIT(7) | BIT(8))
> +#define HASH_DIGEST 0
> +#define HASH_DIGEST_HMAC BIT(7)
> +#define HASH_DIGEST_ACCUM BIT(8)
> +#define HASH_HMAC_KEY (BIT(7) | BIT(8))
> +/* Cascscaed operation modes */
> +#define HASH_ONLY 0
> +#define HASH_ONLY2 BIT(0)
> +#define HASH_CRYPT_THEN_HASH BIT(1)
> +#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
> +/* Other cmd bits */
> +#define HASH_IRQ_EN BIT(9)
> +#define HASH_SG_EN BIT(18)
> +
> +#define R_CRYPT_CMD (0x10 / 4)
> +
> +#define R_HASH_SRC (0x20 / 4)
> +#define R_HASH_DEST (0x24 / 4)
> +#define R_HASH_SRC_LEN (0x2c / 4)
In general, ordering the registers and fields in datasheet-order is
helpful to me as a reviewer...
> +
> +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;
> + int rc;
> +
> + src = s->regs[R_HASH_SRC];
> + len = s->regs[R_HASH_SRC_LEN];
> + dest = s->regs[R_HASH_DEST];
These values should be masked according to the fields described in the
datasheet. That doesn't appear to be done in the write() callback,
though it probably should be as the reserved bits are read-only.
> +
> + src_buf = address_space_map(&s->dram_as, src, &len, false,
> + MEMTXATTRS_UNSPECIFIED);
> + if (!src_buf) {
> + 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,
> + &error_fatal);
Neat :)
> + if (rc < 0) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n",
> __func__);
> + return rc;
> + }
> +
> + rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
> + digest_buf, digest_len);
> + if (rc) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: address space write failed\n", __func__);
> + }
> + g_free(digest_buf);
> +
> + address_space_unmap(&s->dram_as, src_buf, len, false, len);
> +
> + /*
> + * Set status bits to indicate completion. Testing shows hardware
> sets
> + * these irrespective of HASH_IRQ_EN.
> + */
> + s->regs[R_STATUS] |= HASH_IRQ;
> +
> + return 0;
> +}
> +
> +
> +static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned
> int size)
> +{
> + AspeedHACEState *s = ASPEED_HACE(opaque);
> +
> + addr >>= 2;
> +
> + if (addr >= ASPEED_HACE_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds read at offset 0x%"
> HWADDR_PRIx "\n",
> + __func__, addr << 2);
> + return 0;
> + }
> +
> + return s->regs[addr];
> +}
> +
> +static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned int size)
> +{
> + AspeedHACEState *s = ASPEED_HACE(opaque);
> +
> + addr >>= 2;
> +
> + if (addr >= ASPEED_HACE_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds write at offset 0x%"
> HWADDR_PRIx "\n",
> + __func__, addr << 2);
> + return;
> + }
> +
> + switch (addr) {
> + case R_STATUS:
> + if (data & HASH_IRQ) {
> + data &= ~HASH_IRQ;
So the datasheet claims 'Writing "1" to this bit will clear this
register'. Have you followed up on whether they really mean what they
say here? Because that's not what's implemented (having said that, what
you have implemented is at least intuitive).
> +
> + if (s->regs[addr] & HASH_IRQ) {
> + qemu_irq_lower(s->irq);
> + }
> + }
> + break;
> + case R_HASH_CMD: {
> + int algo = -1;
> + if ((data & HASH_HMAC_MASK)) {
> + qemu_log_mask(LOG_UNIMP,
> + "%s: HMAC engine command mode %ld 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)) {
Why leave this bit without a name?
> + qemu_log_mask(LOG_UNIMP,
> + "%s: Cascaded mode not implemented",
> + __func__);
> + }
> + switch (data & HASH_ALGO_MASK) {
> + case HASH_ALGO_MD5:
> + algo = QCRYPTO_HASH_ALG_MD5;
> + break;
> + case HASH_ALGO_SHA1:
> + algo = QCRYPTO_HASH_ALG_SHA1;
> + break;
> + case HASH_ALGO_SHA224:
> + algo = QCRYPTO_HASH_ALG_SHA224;
> + break;
> + case HASH_ALGO_SHA256:
> + algo = QCRYPTO_HASH_ALG_SHA256;
> + break;
> + case HASH_ALGO_SHA512_SERIES:
> + switch (data & SHA512_HASH_ALGO_MASK) {
> + case HASH_ALGO_SHA512_SHA512:
> + algo = QCRYPTO_HASH_ALG_SHA512;
> + break;
> + case HASH_ALGO_SHA512_SHA384:
> + algo = QCRYPTO_HASH_ALG_SHA384;
> + break;
> + case HASH_ALGO_SHA512_SHA256:
> + algo = QCRYPTO_HASH_ALG_SHA256;
> + break;
> + case HASH_ALGO_SHA512_SHA224:
> + algo = QCRYPTO_HASH_ALG_SHA224;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid sha512 hash algorithm selection
> 0x%03lx\n",
> + __func__, data & SHA512_HASH_ALGO_MASK);
> + break;
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid hash algorithm selection 0x%03lx\n",
> + __func__, data & HASH_ALGO_MASK);
> + break;
> + }
The nested switches get a bit hectic. I feel like it would be worth
pulling them out to helper functions.
Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] aspeed: Integrate HACE
2021-03-03 7:03 ` [PATCH 2/2] aspeed: Integrate HACE Joel Stanley
@ 2021-03-09 1:07 ` Andrew Jeffery
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Jeffery @ 2021-03-09 1:07 UTC (permalink / raw)
To: Joel Stanley, Cédric Le Goater
Cc: Peter Maydell, qemu-arm, Cameron Esfahani via
On Wed, 3 Mar 2021, at 17:33, Joel Stanley wrote:
> Add the hash and crypto engine model to the aspeed socs.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> [ clg: documentation update ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine
2021-03-03 7:03 ` [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine Joel Stanley
2021-03-09 1:01 ` Andrew Jeffery
@ 2021-03-09 9:40 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-09 9:40 UTC (permalink / raw)
To: Joel Stanley, Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, qemu-devel
On 3/3/21 8:03 AM, Joel Stanley wrote:
> The HACE (Hash and Crpyto Engine) is a device that offloads MD5, SHA1,
Typo "Crypto"
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/misc/aspeed_hace.h | 33 ++++
> hw/misc/aspeed_hace.c | 302 ++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 2 +-
> 3 files changed, 336 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine
2021-03-09 1:01 ` Andrew Jeffery
@ 2021-03-11 6:17 ` Joel Stanley
0 siblings, 0 replies; 7+ messages in thread
From: Joel Stanley @ 2021-03-11 6:17 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Peter Maydell, qemu-arm, Cédric Le Goater, Cameron Esfahani via
On Tue, 9 Mar 2021 at 01:02, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 3 Mar 2021, at 17:33, Joel Stanley wrote:
> > The HACE (Hash and Crpyto Engine) is a device that offloads MD5, SHA1,
> > SHA2, RSA and other cryptographic algorithms.
> >
> > This initial model implements a subset of the device's functionality;
> > currently only direct access (non-scatter gather) hashing.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > include/hw/misc/aspeed_hace.h | 33 ++++
> > hw/misc/aspeed_hace.c | 302 ++++++++++++++++++++++++++++++++++
> > hw/misc/meson.build | 2 +-
> > 3 files changed, 336 insertions(+), 1 deletion(-)
> > create mode 100644 include/hw/misc/aspeed_hace.h
> > create mode 100644 hw/misc/aspeed_hace.c
> >
> > diff --git a/include/hw/misc/aspeed_hace.h
> > b/include/hw/misc/aspeed_hace.h
> > new file mode 100644
> > index 000000000000..e1fce670ef9e
> > --- /dev/null
> > +++ b/include/hw/misc/aspeed_hace.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * ASPEED Hash and Crypto Engine
> > + *
> > + * Copyright (C) 2021 IBM Corp.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#ifndef ASPEED_HACE_H
> > +#define ASPEED_HACE_H
> > +
> > +#include "hw/sysbus.h"
> > +
> > +#define TYPE_ASPEED_HACE "aspeed.hace"
> > +#define ASPEED_HACE(obj) OBJECT_CHECK(AspeedHACEState, (obj),
> > TYPE_ASPEED_HACE)
> > +
> > +#define ASPEED_HACE_NR_REGS (0x64 >> 2)
>
> 0x64 is the offset of the last defined register, so this should be (0x68 >> 2)
>
> > +
> > +typedef struct AspeedHACEState {
> > + /* <private> */
> > + SysBusDevice parent;
> > +
> > + /*< public >*/
> > + MemoryRegion iomem;
> > + qemu_irq irq;
> > +
> > + uint32_t regs[ASPEED_HACE_NR_REGS];
> > +
> > + MemoryRegion *dram_mr;
> > + AddressSpace dram_as;
> > +} AspeedHACEState;
> > +
> > +#endif /* _ASPEED_HACE_H_ */
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> > new file mode 100644
> > index 000000000000..0e402a0adea9
> > --- /dev/null
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -0,0 +1,302 @@
> > +/*
> > + * ASPEED Hash and Crypto Engine
> > + *
> > + * Copyright (C) 2021 IBM Corp.
> > + *
> > + * Joel Stanley <joel@jms.id.au>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/misc/aspeed_hace.h"
> > +#include "qapi/error.h"
> > +#include "migration/vmstate.h"
> > +#include "crypto/hash.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/irq.h"
> > +
> > +#define R_STATUS (0x1c / 4)
> > +#define HASH_IRQ BIT(9)
> > +#define CRYPT_IRQ BIT(12)
> > +#define TAG_IRQ BIT(15)
> > +
> > +#define R_HASH_CMD (0x30 / 4)
> > +/* Hash algorithim selection */
> > +#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
>
> Hmm, feels GENMASK()-y, but looks like the tree is in a bit of a weird
> state in that respect:
>
> $ git grep GENMASK
> include/hw/usb/dwc2-regs.h:#define GSNPSID_ID_MASK GENMASK(31, 16)
> include/standard-headers/asm-x86/kvm_para.h:#define KVM_ASYNC_PF_VEC_MASK GENMASK(7, 0)
> $
>
> > +#define HASH_ALGO_MD5 0
> > +#define HASH_ALGO_SHA1 BIT(5)
> > +#define HASH_ALGO_SHA224 BIT(6)
> > +#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
>
> Not something you need to fix necessarily, but it would feel more
> intuitive to me to order these MSB to LSB, so e.g. (BIT(6) | BIT(4)).
> That way I can "see" the value without having to reverse the bits.
>
> > +#define HASH_ALGO_SHA512_SERIES (BIT(5) | BIT(6))
> > +/* SHA512 algorithim selection */
> > +#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
> > +#define HASH_ALGO_SHA512_SHA512 0
> > +#define HASH_ALGO_SHA512_SHA384 BIT(10)
> > +#define HASH_ALGO_SHA512_SHA256 BIT(11)
> > +#define HASH_ALGO_SHA512_SHA224 (BIT(10) | BIT(11))
> > +/* HMAC modes */
> > +#define HASH_HMAC_MASK (BIT(7) | BIT(8))
> > +#define HASH_DIGEST 0
> > +#define HASH_DIGEST_HMAC BIT(7)
> > +#define HASH_DIGEST_ACCUM BIT(8)
> > +#define HASH_HMAC_KEY (BIT(7) | BIT(8))
> > +/* Cascscaed operation modes */
> > +#define HASH_ONLY 0
> > +#define HASH_ONLY2 BIT(0)
> > +#define HASH_CRYPT_THEN_HASH BIT(1)
> > +#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
> > +/* Other cmd bits */
> > +#define HASH_IRQ_EN BIT(9)
> > +#define HASH_SG_EN BIT(18)
> > +
> > +#define R_CRYPT_CMD (0x10 / 4)
> > +
> > +#define R_HASH_SRC (0x20 / 4)
> > +#define R_HASH_DEST (0x24 / 4)
> > +#define R_HASH_SRC_LEN (0x2c / 4)
>
> In general, ordering the registers and fields in datasheet-order is
> helpful to me as a reviewer...
I changed the order.
>
> > +
> > +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;
> > + int rc;
> > +
> > + src = s->regs[R_HASH_SRC];
> > + len = s->regs[R_HASH_SRC_LEN];
> > + dest = s->regs[R_HASH_DEST];
>
> These values should be masked according to the fields described in the
> datasheet. That doesn't appear to be done in the write() callback,
> though it probably should be as the reserved bits are read-only.
I added masking at write time as it matches hardware.
> > + switch (addr) {
> > + case R_STATUS:
> > + if (data & HASH_IRQ) {
> > + data &= ~HASH_IRQ;
>
> So the datasheet claims 'Writing "1" to this bit will clear this
> register'. Have you followed up on whether they really mean what they
> say here? Because that's not what's implemented (having said that, what
> you have implemented is at least intuitive).
Yeah, it does what you expect it to, and that's what I've implemented.
>
> > +
> > + if (s->regs[addr] & HASH_IRQ) {
> > + qemu_irq_lower(s->irq);
> > + }
> > + }
> > + break;
> > + case R_HASH_CMD: {
> > + int algo = -1;
> > + if ((data & HASH_HMAC_MASK)) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: HMAC engine command mode %ld 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)) {
>
> Why leave this bit without a name?
We are explicitly checking bit 1; if this is set then the machine is
in one of two modes that aren't implemented.
>
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: Cascaded mode not implemented",
> > + __func__);
> > + }
> > + switch (data & HASH_ALGO_MASK) {
> > + case HASH_ALGO_MD5:
> > + algo = QCRYPTO_HASH_ALG_MD5;
> > + break;
> > + case HASH_ALGO_SHA1:
> > + algo = QCRYPTO_HASH_ALG_SHA1;
> > + break;
> > + case HASH_ALGO_SHA224:
> > + algo = QCRYPTO_HASH_ALG_SHA224;
> > + break;
> > + case HASH_ALGO_SHA256:
> > + algo = QCRYPTO_HASH_ALG_SHA256;
> > + break;
> > + case HASH_ALGO_SHA512_SERIES:
> > + switch (data & SHA512_HASH_ALGO_MASK) {
> > + case HASH_ALGO_SHA512_SHA512:
> > + algo = QCRYPTO_HASH_ALG_SHA512;
> > + break;
> > + case HASH_ALGO_SHA512_SHA384:
> > + algo = QCRYPTO_HASH_ALG_SHA384;
> > + break;
> > + case HASH_ALGO_SHA512_SHA256:
> > + algo = QCRYPTO_HASH_ALG_SHA256;
> > + break;
> > + case HASH_ALGO_SHA512_SHA224:
> > + algo = QCRYPTO_HASH_ALG_SHA224;
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Invalid sha512 hash algorithm selection
> > 0x%03lx\n",
> > + __func__, data & SHA512_HASH_ALGO_MASK);
> > + break;
> > + }
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Invalid hash algorithm selection 0x%03lx\n",
> > + __func__, data & HASH_ALGO_MASK);
> > + break;
> > + }
>
> The nested switches get a bit hectic. I feel like it would be worth
> pulling them out to helper functions.
I tried that but then the log message is less helpful.
Thanks for the review.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-11 6:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 7:03 [PATCH 0/2] hw/misc: Model ASPEED hash and crpyto engine Joel Stanley
2021-03-03 7:03 ` [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine Joel Stanley
2021-03-09 1:01 ` Andrew Jeffery
2021-03-11 6:17 ` Joel Stanley
2021-03-09 9:40 ` Philippe Mathieu-Daudé
2021-03-03 7:03 ` [PATCH 2/2] aspeed: Integrate HACE Joel Stanley
2021-03-09 1:07 ` Andrew Jeffery
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.