Re: [PATCH v4 1/3] hw: Model ASPEED's Hash and Crypto Engine

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 3/24/21 8:09 AM, Joel Stanley wrote:
> The HACE (Hash and Crypto 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>
> ---
> v3:
>  - rebase on upstream to fix meson.build conflict
> v2:
>  - reorder register defines
>  - mask src/dest/len registers according to hardware
> v4:
>  - Fix typos in comments
>  - Remove sdram base address; new memory region fixes mean this is not
>    required
>  - Use PRIx64
>  - Add Object Classes for soc familiy specific features
>  - Convert big switch statement to a lookup in a struct
> ---
>  include/hw/misc/aspeed_hace.h |  43 ++++
>  hw/misc/aspeed_hace.c         | 358 ++++++++++++++++++++++++++++++++++
>  hw/misc/meson.build           |   1 +
>  3 files changed, 402 insertions(+)
>  create mode 100644 include/hw/misc/aspeed_hace.h
>  create mode 100644 hw/misc/aspeed_hace.c

> +static int hash_algo_lookup(uint32_t mask)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(hash_algo_map); i++) {
> +        if (mask == hash_algo_map[i].mask)

{

> +            return hash_algo_map[i].algo;

}

> +    }
> +
> +    return -1;
> +}
> +
> +static int do_hash_operation(AspeedHACEState *s, int algo)
> +{
> +    hwaddr src, len, dest;
> +    uint8_t *digest_buf = NULL;

Eventually g_autofree,

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

removing g_free().

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

Generic model LGTM.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>