Re: [PATCH] Supporting AST2600 HACE engine accumulative mode

[Klaus Heinrich Kiwi]

Em qui., 23 de dez. de 2021 às 09:54, Cédric Le Goater <clg@kaod.org> escreveu:
>
> [ Adding Klaus ]

Thanks Cedric. It's been a while since I've looked at this but I'll do my best..

>
> On 12/22/21 03:22, Troy Lee wrote:
> > Accumulative mode will supply a initial state and append padding bit at
> > the end of hash stream.  However, the crypto library will padding those
> > bit automatically, so ripped it off from iov array.
> >
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_hace.c         | 30 ++++++++++++++++++++++++++++--
> >   include/hw/misc/aspeed_hace.h |  1 +
> >   2 files changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> > index 10f00e65f4..7c1794d6d0 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -27,6 +27,7 @@
> >
> >   #define R_HASH_SRC      (0x20 / 4)
> >   #define R_HASH_DEST     (0x24 / 4)
> > +#define R_HASH_KEY_BUFF (0x28 / 4)
> >   #define R_HASH_SRC_LEN  (0x2c / 4)
> >
> >   #define R_HASH_CMD      (0x30 / 4)
> > @@ -94,7 +95,10 @@ static int hash_algo_lookup(uint32_t reg)
> >       return -1;
> >   }
> >
> > -static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode)
> > +static void do_hash_operation(AspeedHACEState *s,
> > +                              int algo,
> > +                              bool sg_mode,
> > +                              bool acc_mode)
> >   {
> >       struct iovec iov[ASPEED_HACE_MAX_SG];
> >       g_autofree uint8_t *digest_buf;
> > @@ -103,6 +107,7 @@ static void do_hash_operation(AspeedHACEState *s, int 
> > algo, bool sg_mode)
> >
> >       if (sg_mode) {
> >           uint32_t len = 0;
> > +        uint32_t total_len = 0;
> >
> >           for (i = 0; !(len & SG_LIST_LEN_LAST); i++) {
> >               uint32_t addr, src;
> > @@ -127,6 +132,21 @@ static void do_hash_operation(AspeedHACEState *s, int 
> > algo, bool sg_mode)
> >               plen = iov[i].iov_len;
> >               iov[i].iov_base = address_space_map(&s->dram_as, addr, &plen, 
> > false,
> >                                                   MEMTXATTRS_UNSPECIFIED);
> > +
> > +            total_len += plen;
> > +            if (acc_mode && len & SG_LIST_LEN_LAST) {
I think we should make the precedence here explicit (with the use of
()) instead of implicit. Also, isn't (len * SGL_LIST_LEN_LAST) always
true inside this for loop?

> > +                /*
> > +                 * Read the message length in bit from last 64/128 bits
> > +                 * and tear the padding bits from iov
> > +                 */
> > +                uint64_t stream_len;
> > +
> > +                memcpy(&stream_len, iov[i].iov_base + iov[i].iov_len - 8, 
> > 8);
> > +                stream_len = __bswap_64(stream_len) / 8;
> > +

I no longer have access to the aspeed workbook I guess, but what is
the actual specification here? is the message length 64 or 128 bits?
and bswap seems arch-dependent - you probably want to be explicit if
this is big or little-endian and use the appropriate conversion
function.

> > +                if (total_len > stream_len)
> > +                    iov[i].iov_len -= total_len - stream_len;
> > +            }

I guess you're not using total_len except for this comparison? Feels
like there's a better way to first compare and then assign the value,
other than assign, compare, re-assign etc.

The rest of it looks fine apparently. Are you planning on adding to
the testcases are well?

Thanks,

 -Klaus

 /*
 <klaus@klauskiwi.com> - http://blog.klauskiwi.com
 */

> >           }
> >       } else {
> >           hwaddr len = s->regs[R_HASH_SRC_LEN];
> > @@ -210,6 +230,9 @@ static void aspeed_hace_write(void *opaque, hwaddr 
> > addr, uint64_t data,
> >       case R_HASH_DEST:
> >           data &= ahc->dest_mask;
> >           break;
> > +    case R_HASH_KEY_BUFF:
> > +        data &= ahc->key_mask;
> > +        break;
> >       case R_HASH_SRC_LEN:
> >           data &= 0x0FFFFFFF;
> >           break;
> > @@ -234,7 +257,7 @@ static void aspeed_hace_write(void *opaque, hwaddr 
> > addr, uint64_t data,
> >                           __func__, data & ahc->hash_mask);
> >                   break;
> >           }
> > -        do_hash_operation(s, algo, data & HASH_SG_EN);
> > +        do_hash_operation(s, algo, data & HASH_SG_EN, data & 
> > HASH_DIGEST_ACCUM);
> >
> >           if (data & HASH_IRQ_EN) {
> >               qemu_irq_raise(s->irq);
> > @@ -333,6 +356,7 @@ static void aspeed_ast2400_hace_class_init(ObjectClass 
> > *klass, void *data)
> >
> >       ahc->src_mask = 0x0FFFFFFF;
> >       ahc->dest_mask = 0x0FFFFFF8;
> > +    ahc->key_mask = 0x0FFFFFC0;
> >       ahc->hash_mask = 0x000003ff; /* No SG or SHA512 modes */
> >   }
> >
> > @@ -351,6 +375,7 @@ static void aspeed_ast2500_hace_class_init(ObjectClass 
> > *klass, void *data)
> >
> >       ahc->src_mask = 0x3fffffff;
> >       ahc->dest_mask = 0x3ffffff8;
> > +    ahc->key_mask = 0x3FFFFFC0;
> >       ahc->hash_mask = 0x000003ff; /* No SG or SHA512 modes */
> >   }
> >
> > @@ -369,6 +394,7 @@ static void aspeed_ast2600_hace_class_init(ObjectClass 
> > *klass, void *data)
> >
> >       ahc->src_mask = 0x7FFFFFFF;
> >       ahc->dest_mask = 0x7FFFFFF8;
> > +    ahc->key_mask = 0x7FFFFFF8;
> >       ahc->hash_mask = 0x00147FFF;
> >   }
> >
> > diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
> > index 94d5ada95f..2242945eb4 100644
> > --- a/include/hw/misc/aspeed_hace.h
> > +++ b/include/hw/misc/aspeed_hace.h
> > @@ -37,6 +37,7 @@ struct AspeedHACEClass {
> >
> >       uint32_t src_mask;
> >       uint32_t dest_mask;
> > +    uint32_t key_mask;
> >       uint32_t hash_mask;
> >   };
> >
> >
>