Em qui., 23 de dez. de 2021 às 09:54, Cédric Le Goater <c...@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_...@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
/*
<kl...@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;
> > };
> >
> >
>
