RE: [PATCH v2 01/17] hw/misc/aspeed_hace: Support the crypto command in direct access mode

Hi Kane,

> Subject: RE: [PATCH v2 01/17] hw/misc/aspeed_hace: Support the crypto
> command in direct access mode
> 
> > -----Original Message-----
> > From: Jamin Lin <[email protected]>
> > Sent: Wednesday, July 15, 2026 11:33 AM
> > To: Daniel P. Berrangé <[email protected]>; Cédric Le Goater
> > <[email protected]>; Peter Maydell <[email protected]>; Steven Lee
> > <[email protected]>; Troy Lee <[email protected]>; Kane Chen
> > <[email protected]>; Andrew Jeffery
> > <[email protected]>; Joel Stanley <[email protected]>; Eric
> > Blake <[email protected]>; Markus Armbruster <[email protected]>;
> > Fabiano Rosas <[email protected]>; Laurent Vivier <[email protected]>;
> > Paolo Bonzini <[email protected]>; open list:All patches CC here
> > <[email protected]>; open list:ASPEED BMCs
> <[email protected]>
> > Cc: Jamin Lin <[email protected]>; Troy Lee
> > <[email protected]>
> > Subject: [PATCH v2 01/17] hw/misc/aspeed_hace: Support the crypto
> > command in direct access mode
> >
> > The crypt command register was previously stubbed out. Implement it
> > for the direct access mode, where HACE00/HACE04 point directly at
> > contiguous source and destination buffers. AES-128/192/256, DES and
> > 3DES are supported in ECB and CBC modes via the qcrypto cipher API;
> > the IV and key are read from the context buffer (HACE08) and, for CBC,
> > the resulting chaining IV is written back to the context.
> >
> > The completion interrupt is now raised for every HACE variant as the
> > hardware does, which fixes the crypt command hang on the AST2500,
> > AST2600 and AST1030. The AST2700 crypto engine still needs 64-bit DMA
> > and AES-GCM, which are added later, so it keeps its temporary
> > interrupt-only workaround until then.
> >
> > For debugging, the context, source and destination buffers are dumped
> > through the existing aspeed_hace_hexdump trace event (disabled by
> > default). CTR mode, scatter-gather mode and AES-GCM are added
> separately.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> >  hw/misc/aspeed_hace.c | 283
> > +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 269 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > c61efe50c4..de4399f322 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -18,11 +18,44 @@
> >  #include "qapi/error.h"
> >  #include "migration/vmstate.h"
> >  #include "crypto/hash.h"
> > +#include "crypto/cipher.h"
> >  #include "hw/core/qdev-properties.h"
> >  #include "hw/core/irq.h"
> >  #include "trace.h"
> >
> > -#define R_CRYPT_CMD     (0x10 / 4)
> > +/* Crypto engine registers */
> > +#define R_CRYPT_SRC         (0x00 / 4)
> > +#define R_CRYPT_DEST        (0x04 / 4)
> > +#define R_CRYPT_CONTEXT     (0x08 / 4)
> > +#define R_CRYPT_DATA_LEN    (0x0c / 4)
> > +/* HACE0C[27:0] holds the crypto data length */
> > +#define  CRYPT_DATA_LEN_MASK    0x0FFFFFFF
> > +#define R_CRYPT_CMD         (0x10 / 4)
> > +/* Crypto engine command register (HACE10) bits */
> > +#define  CRYPT_CMD_ENCRYPT          BIT(7)
> > +#define  CRYPT_CMD_ISR_EN           BIT(12)
> 
> Regarding CRYPT_CMD_ISR_EN, the current code already has similar
> definitions, such as CRYPT_IRQ and CRYPT_IRQ_EN. Would it make sense to
> migrate these definitions together for consistency?
> 
Thanks for the review and suggestion.

CRYPT_IRQ is used to indicate the CRYPT ISR status in the HAC Engine Status Register (0x1C).
Both CRYPT_CMD_ISR_EN and CRYPT_IRQ_EN are used to enable the CRYPT IRQ in the Crypto Engine Command Register (0x10).
I will remove the duplicate CRYPT_IRQ_EN definition.

> > +#define  CRYPT_CMD_DES_SELECT       BIT(16)
> > +#define  CRYPT_CMD_TRIPLE_DES       BIT(17)
> > +#define  CRYPT_CMD_SRC_SG_CTRL      BIT(18)
> > +#define  CRYPT_CMD_DST_SG_CTRL      BIT(19)
> 
> CRYPT_CMD_DST_SG_CTRL is currently defined but not used anywhere. Could
> we defer adding this definition until it is actually needed?
> 

Will remove it

Thanks,
Jamin

> > +/* Operation mode HACE10[6:4] */
> > +#define  CRYPT_CMD_OP_MODE_MASK     (0x7 << 4)
> > +#define  CRYPT_CMD_ECB              (0x0 << 4)
> > +#define  CRYPT_CMD_CBC              (0x1 << 4)
> > +/* AES key length HACE10[3:2] */
> > +#define  CRYPT_CMD_AES_KEY_LEN_MASK (0x3 << 2)
> > +#define  CRYPT_CMD_AES256           (0x2 << 2)
> > +#define  CRYPT_CMD_AES192           (0x1 << 2)
> > +#define  CRYPT_CMD_AES128           (0x0 << 2)
> > +
> > +/*
> > + * Crypto context buffer layout (HACE08). The IV is at the start of
> > +the buffer
> > + * (DES places its 8 byte IV at offset 8) and the cipher key at offset 0x10.
> > + */
> > +#define CRYPT_CTX_IV_OFFSET         0x00
> > +#define CRYPT_CTX_DES_IV_OFFSET     0x08
> > +#define CRYPT_CTX_KEY_OFFSET        0x10
> > +#define CRYPT_CTX_SIZE              0x30
> >
> >  #define R_STATUS        (0x1c / 4)
> >  #define HASH_IRQ        BIT(9)
> > @@ -501,6 +534,216 @@ static void do_hash_operation(AspeedHACEState
> > *s, int algo, bool sg_mode,
> >      }
> >  }
> >
> > +static bool crypt_aes_alg(uint32_t cmd, QCryptoCipherAlgo *alg,
> > +size_t
> > +*keylen) {
> > +    switch (cmd & CRYPT_CMD_AES_KEY_LEN_MASK) {
> > +    case CRYPT_CMD_AES128:
> > +        *alg = QCRYPTO_CIPHER_ALGO_AES_128;
> > +        *keylen = 16;
> > +        break;
> > +    case CRYPT_CMD_AES192:
> > +        *alg = QCRYPTO_CIPHER_ALGO_AES_192;
> > +        *keylen = 24;
> > +        break;
> > +    case CRYPT_CMD_AES256:
> > +        *alg = QCRYPTO_CIPHER_ALGO_AES_256;
> > +        *keylen = 32;
> > +        break;
> > +    default:
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +/*
> > + * Decode the crypto command register into a libqcrypto
> > +algorithm/mode pair
> > + * and the block/IV geometry. Returns false for unsupported selections.
> > + */
> > +static bool crypt_decode_cmd(uint32_t cmd, QCryptoCipherAlgo *alg,
> > +                             QCryptoCipherMode *mode, size_t
> > *keylen,
> > +                             size_t *blocklen, size_t *iv_offset) {
> > +    if (cmd & CRYPT_CMD_DES_SELECT) {
> > +        *blocklen = 8;
> > +        *iv_offset = CRYPT_CTX_DES_IV_OFFSET;
> > +        if (cmd & CRYPT_CMD_TRIPLE_DES) {
> > +            *alg = QCRYPTO_CIPHER_ALGO_3DES;
> > +            *keylen = 24;
> > +        } else {
> > +            *alg = QCRYPTO_CIPHER_ALGO_DES;
> > +            *keylen = 8;
> > +        }
> > +    } else {
> > +        *blocklen = 16;
> > +        *iv_offset = CRYPT_CTX_IV_OFFSET;
> > +        if (!crypt_aes_alg(cmd, alg, keylen)) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    switch (cmd & CRYPT_CMD_OP_MODE_MASK) {
> > +    case CRYPT_CMD_ECB:
> > +        *mode = QCRYPTO_CIPHER_MODE_ECB;
> > +        break;
> > +    case CRYPT_CMD_CBC:
> > +        *mode = QCRYPTO_CIPHER_MODE_CBC;
> > +        break;
> > +    default:
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +/*
> > + * Direct access mode: the source/destination register
> > +(HACE00/HACE04) points
> > + * at a single contiguous buffer in DRAM. Copy @len bytes between it
> > +and the
> > + * bounce buffer @buf; when @to_dram is true @buf is written out,
> > +otherwise it
> > + * is read in. Returns true on success.
> > + */
> > +static bool crypt_prepare_direct(AspeedHACEState *s, uint64_t addr,
> > +                                 uint8_t *buf, uint32_t len, bool
> > +to_dram) {
> > +    return !address_space_rw(&s->dram_as, addr,
> > MEMTXATTRS_UNSPECIFIED,
> > +                             buf, len, to_dram); }
> > +
> > +/*
> > + * Perform an AES/DES/3DES ECB/CBC operation in direct access mode:
> > +the source
> > + * and destination are single contiguous buffers (HACE00/HACE04) and
> > +the IV/key
> > + * come from the context buffer (HACE08). For CBC the resulting
> > +chaining IV is
> > + * written back to the context buffer so the driver can continue the chain.
> > + */
> > +static void do_crypt_operation(AspeedHACEState *s, uint32_t cmd) {
> > +    uint32_t len = s->regs[R_CRYPT_DATA_LEN];
> > +    bool encrypt = cmd & CRYPT_CMD_ENCRYPT;
> > +    g_autoptr(QCryptoCipher) cipher = NULL;
> > +    g_autofree uint8_t *src_buf = NULL;
> > +    g_autofree uint8_t *dst_buf = NULL;
> > +    uint8_t ctx[CRYPT_CTX_SIZE];
> > +    Error *local_err = NULL;
> > +    QCryptoCipherMode mode;
> > +    QCryptoCipherAlgo alg;
> > +    const uint8_t *next_iv;
> > +    uint64_t ctx_addr;
> > +    uint64_t src_addr;
> > +    uint64_t dst_addr;
> > +    size_t iv_offset;
> > +    size_t blocklen;
> > +    size_t keylen;
> > +
> > +    if (len == 0) {
> > +        return;
> > +    }
> > +
> > +    if (!crypt_decode_cmd(cmd, &alg, &mode, &keylen, &blocklen,
> > &iv_offset)) {
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: Unsupported crypt command 0x%x\n",
> > __func__, cmd);
> > +        return;
> > +    }
> > +
> > +    if (!qcrypto_cipher_supports(alg, mode)) {
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: cipher mode not supported by the crypto
> > backend\n",
> > +                      __func__);
> > +        return;
> > +    }
> > +
> > +    /* Fetch the IV and key from the context buffer in DRAM. */
> > +    ctx_addr = s->regs[R_CRYPT_CONTEXT];
> > +    if (address_space_read(&s->dram_as, ctx_addr,
> > MEMTXATTRS_UNSPECIFIED,
> > +                           ctx, sizeof(ctx))) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Failed to read context, addr=0x%"
> > HWADDR_PRIx "\n",
> > +                      __func__, ctx_addr);
> > +        return;
> > +    }
> > +
> > +    if
> (trace_event_get_state_backends(TRACE_ASPEED_HACE_HEXDUMP))
> > {
> > +        hace_hexdump("context", (char *)ctx, sizeof(ctx));
> > +    }
> > +
> > +    cipher = qcrypto_cipher_new(alg, mode, ctx +
> > + CRYPT_CTX_KEY_OFFSET,
> > keylen,
> > +                                &local_err);
> > +    if (cipher == NULL) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto cipher new
> > failed: %s\n",
> > +                      __func__, error_get_pretty(local_err));
> > +        error_free(local_err);
> > +        return;
> > +    }
> > +
> > +    if (mode != QCRYPTO_CIPHER_MODE_ECB &&
> > +        qcrypto_cipher_setiv(cipher, ctx + iv_offset, blocklen,
> > +                             &local_err) < 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto cipher setiv
> > failed: %s\n",
> > +                      __func__, error_get_pretty(local_err));
> > +        error_free(local_err);
> > +        return;
> > +    }
> > +
> > +    src_buf = g_malloc0(len);
> > +    dst_buf = g_malloc0(len);
> > +
> > +    src_addr = s->regs[R_CRYPT_SRC];
> > +    if (!crypt_prepare_direct(s, src_addr, src_buf, len, false)) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Failed to read src, addr=0x%"
> HWADDR_PRIx
> > "\n",
> > +                      __func__, src_addr);
> > +        return;
> > +    }
> > +
> > +    if
> (trace_event_get_state_backends(TRACE_ASPEED_HACE_HEXDUMP))
> > {
> > +        hace_hexdump("src", (char *)src_buf, len);
> > +    }
> > +
> > +    if (encrypt) {
> > +        if (qcrypto_cipher_encrypt(cipher, src_buf, dst_buf, len,
> > +                                   &local_err) < 0) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: encrypt failed:
> > %s\n",
> > +                          __func__, error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            return;
> > +        }
> > +    } else {
> > +        if (qcrypto_cipher_decrypt(cipher, src_buf, dst_buf, len,
> > +                                   &local_err) < 0) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: decrypt failed:
> > %s\n",
> > +                          __func__, error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            return;
> > +        }
> > +    }
> > +
> > +    dst_addr = s->regs[R_CRYPT_DEST];
> > +    if (!crypt_prepare_direct(s, dst_addr, dst_buf, len, true)) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Failed to write dst, addr=0x%"
> HWADDR_PRIx
> > "\n",
> > +                      __func__, dst_addr);
> > +        return;
> > +    }
> > +
> > +    if
> (trace_event_get_state_backends(TRACE_ASPEED_HACE_HEXDUMP))
> > {
> > +        hace_hexdump("dst", (char *)dst_buf, len);
> > +    }
> > +
> > +    if (mode == QCRYPTO_CIPHER_MODE_CBC) {
> > +        /*
> > +         * CBC chains on the last ciphertext block: the final block of the
> > +         * output when encrypting, or of the input when decrypting.
> > + Write
> > it
> > +         * back as the IV for the next request.
> > +         */
> > +        next_iv = (encrypt ? dst_buf : src_buf) + len - blocklen;
> > +        if (address_space_write(&s->dram_as, ctx_addr + iv_offset,
> > +                                MEMTXATTRS_UNSPECIFIED,
> next_iv,
> > blocklen)) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "%s: Failed to write IV, addr=0x%"
> > HWADDR_PRIx "\n",
> > +                          __func__, ctx_addr + iv_offset);
> > +        }
> > +    }
> > +}
> > +
> >  static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned
> > int
> > size)  {
> >      AspeedHACEState *s = ASPEED_HACE(opaque); @@ -531,16 +774,22
> @@
> > static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
> >                  qemu_irq_lower(s->irq);
> >              }
> >          }
> > -        if (ahc->raise_crypt_interrupt_workaround) {
> > -            if (data & CRYPT_IRQ) {
> > -                data &= ~CRYPT_IRQ;
> > +        if (data & CRYPT_IRQ) {
> > +            data &= ~CRYPT_IRQ;
> >
> > -                if (s->regs[addr] & CRYPT_IRQ) {
> > -                    qemu_irq_lower(s->irq);
> > -                }
> > +            if (s->regs[addr] & CRYPT_IRQ) {
> > +                qemu_irq_lower(s->irq);
> >              }
> >          }
> >          break;
> > +    case R_CRYPT_SRC:
> > +    case R_CRYPT_DEST:
> > +    case R_CRYPT_CONTEXT:
> > +        data &= ahc->src_mask;
> > +        break;
> > +    case R_CRYPT_DATA_LEN:
> > +        data &= CRYPT_DATA_LEN_MASK;
> > +        break;
> >      case R_HASH_SRC:
> >          data &= ahc->src_mask;
> >          break;
> > @@ -589,13 +838,19 @@ static void aspeed_hace_write(void *opaque,
> > hwaddr addr, uint64_t data,
> >          break;
> >      }
> >      case R_CRYPT_CMD:
> > -        qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not
> > implemented\n",
> > -                       __func__);
> > -        if (ahc->raise_crypt_interrupt_workaround) {
> > -            s->regs[R_STATUS] |= CRYPT_IRQ;
> > -            if (data & CRYPT_IRQ_EN) {
> > -                qemu_irq_raise(s->irq);
> > -            }
> > +        /*
> > +         * The AST2700 crypto engine needs 64-bit DMA and AES-GCM,
> > which are
> > +         * added later; until then it keeps the temporary workaround
> > + of
> > only
> > +         * raising the completion interrupt without running the
> command.
> > +         */
> > +        if (!ahc->raise_crypt_interrupt_workaround) {
> > +            do_crypt_operation(s, data);
> > +        }
> > +
> > +        /* Hardware raises the crypt interrupt once the command
> finishes.
> > */
> > +        s->regs[R_STATUS] |= CRYPT_IRQ;
> > +        if (data & CRYPT_CMD_ISR_EN) {
> > +            qemu_irq_raise(s->irq);
> >          }
> >          break;
> >      case R_HASH_SRC_HI:
> > --
> > 2.43.0