Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Klaus Heinrich Kiwi]

On 3/25/2021 4:55 AM, Cédric Le Goater wrote:
> On 3/24/21 11:38 PM, Klaus Heinrich Kiwi wrote:
> > Complement the Aspeed HACE support with Scatter-Gather hash support for
> > sha256 and sha512. Scatter-Gather is only supported on AST2600-series.
> >
> > Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
>
> this looks good. A few extra comments,
>
>
> > @@ -129,6 +133,117 @@ static int do_hash_operation(AspeedHACEState *s, int algo)
> >       return 0;
> >   }
> >   
> > +static int do_hash_sg_operation(AspeedHACEState *s, int algo)
>
> Do we really care about the return value ?

I'm keeping it consistent with do_hash_operation() above it. Maybe the 
underlying
Qemu layers could use it?

 
> > +        sgList = (struct aspeed_sg_list *) address_space_map(&s->dram_as,
> > +                                                                     src,
> > +                                                         (hwaddr *) &len,
> > +                                                                   false,
> > +                                                 MEMTXATTRS_UNSPECIFIED);
>
> This should be doing LE loads.

ack. I'm using address_space_ldl_le() now.

 
> > -        do_hash_operation(s, algo);
> > +        if (data & HASH_SG_EN) {
> > +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;
>
> I think Joel introduced a class mask for this value.

Turns out that the hardware doesn't do any additional masking on the src
register for the HASH_SG_EN operation, so I'll just remove it.

 
> >   
> > +#define ASPEED_HACE_MAX_SG      256
> > +struct aspeed_sg_list {
> > +        uint32_t len;
> > +        uint32_t phy_addr;
> > +} __attribute__ ((__packed__));
> > +> 
> May be keep the definition in the .c file

I actually opted to use #define offsets now that I'm using ldl_le

Thanks,

 -Klaus

--
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>