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

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

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>