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

[Klaus Heinrich Kiwi]

Hi Joel,

On 4/19/2021 9:49 AM, Joel Stanley wrote:
> On Thu, 15 Apr 2021 at 21:38, Klaus Heinrich Kiwi

>>
>>    * 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?

yes, I thought I had. And by pre-sdram I guess I was trying to say pre-relocation.
But that might have been an artifact of trying to probe/initialize hace in the
SPL, so I might have mixed things.

I was sure that Qemu and Hardware were behaving differently, since the same
image would work on qemu but fail on hardware. Perhaps we should put a warning
on Qemu for unaligned access instead of just masking.

  
>>
>>    * 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?
> I did. A typo.

> 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.
> 

My change involved moving the array to the first element exactly to not
risk changing the u32 elements before it to something else and breaking
alignment.

>>
>>     * 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

Sounds good. I can give it a spin on rain100 and check if it's all good.

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