[PATCH u-boot v2019.04-aspeed-openbmc 10/11] crypto: Add driver for Aspeed HACE

[Joel Stanley]

On Thu, 15 Apr 2021 at 21:38, Klaus Heinrich Kiwi
<klaus at linux.vnet.ibm.com> wrote:
>
>
>
> On 4/14/2021 11:32 PM, Joel Stanley wrote:
> >> Another interesting thing is that the SPL tries to boot from UART, but neither
> >> my fitImages, Legacy images or even RAW images are working.. Not sure if we need
> >> some special handling of those images before feeding them to the spl ymodem loader?
> > I wasn't able to get the SPL to load any images - raw binaries or FIT
> > - from eMMC either. Something is going wrong, but I am unsure what it
> > is. I will continue to debug.
>
> I was able to make it work on real hardware (rainier100) with the following changes
> (in addition to the other fixups already mentioned in this thread):
>
>  From a2a2819455ec5de689fd0702cce20bfb18ec11b1 Mon Sep 17 00:00:00 2001
> From: Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>
> Date: Thu, 15 Apr 2021 15:16:37 -0300
> Subject: [PATCH] HACE fixups:
>
>   * The AST2600 workbook mentions that the list structure passed to
>     ASPEED_HACE_HASH_SRC needs to be 8-byte aligned. Normally, glibc's
>     malloc() would always align memory to (at least) 8-bytes, but that's
>     the case with u-boot's pre-sdram malloc() implementation. So we need
>     to explicitly align the context to 8-bytes with malign().

We're not using the HACE engine pre-SDRAM, so we should be ok.

common/dlmalloc.c:

    8-byte alignment is guaranteed by normal malloc calls, so don't
    bother calling memalign with an argument of 8 or less.

Were you able to observe any allocations that were not aligned?

>
>   * The __atribute__ ((align 8)) doesn't have an effect in struct
>     elements. Remove it.

Agreed.

>
>   * Since the struct aspeed_hash_ctx->list element is what we need to
>     make sure is aligned to 9-bytes, move that to the first element of
>     the array, and call-out the fact that this needs to be aligned in
>     the declaration.

9 bytes? Did you mean 8?

Regardless, the array in the structure should be aligned as there will
be no padding earlier in the struct. I have added a runtime check for
this and have not hit that in my testing so far.

>
>    * Clear HACE_HASH_ISR before issuing new command

This makes sense.

I will send out a new series for review. I've tested it thoroughly in
qemu, and have tested the u-boot -> Linux path on hardware.

Cheers,

Joel

>
> Signed-off-by: Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>
> ---
>   drivers/crypto/aspeed_hace.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/aspeed_hace.c b/drivers/crypto/aspeed_hace.c
> index 473d4d7391..0551fe6c83 100644
> --- a/drivers/crypto/aspeed_hace.c
> +++ b/drivers/crypto/aspeed_hace.c
> @@ -51,12 +51,19 @@ struct aspeed_sg {
>         u32 addr;
>   };
>
> +
> +/*
> + * Note: element 'list' below needs to be 8-byte aligned,
> + *       keep it as the first element so that we can always
> + *       guarantee that when allocating the struct (that should
> + *       also be 8-byte aligned)
> + */
>   struct aspeed_hash_ctx {
> +       struct aspeed_sg list[ASPEED_MAX_SG];
>         u32 method;
>         u32 digest_size;
>         u32 len;
>         u32 count;
> -       struct aspeed_sg list[ASPEED_MAX_SG] __attribute__((aligned(8)));
>   };
>
>   struct aspeed_hace {
> @@ -85,6 +92,9 @@ static int digest_object(const void *src, unsigned int length, void *digest,
>                 return -EINVAL;
>         }
>
> +       /* clear any pending interrupt */
> +       writel(HACE_HASH_ISR, base + ASPEED_HACE_STS);
> +
>         writel((u32)src, base + ASPEED_HACE_HASH_SRC);
>         writel((u32)digest, base + ASPEED_HACE_HASH_DIGEST_BUFF);
>         writel(length, base + ASPEED_HACE_HASH_DATA_LEN);
> @@ -145,12 +155,13 @@ int hw_sha_init(struct hash_algo *algo, void **ctxp)
>                 return -ENOTSUPP;
>         }
>
> -       ctx = calloc(1, sizeof(*ctx));
> +       ctx = memalign(8, sizeof(struct aspeed_hash_ctx));
>
>         if (ctx == NULL) {
>                 debug("Cannot allocate memory for context\n");
>                 return -ENOMEM;
>         }
> +       ctx->len = ctx->count = 0;
>         ctx->method = method | HACE_SG_EN;
>         ctx->digest_size = algo->digest_size;
>         *ctxp = ctx;
> --
> 2.25.1
>
>
>
>
>
> --
> Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>