Re: [PATCH v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing

["Cédric Le Goater" <clg@redhat.com>]

On 10/23/24 08:52, Joel Stanley wrote:
> On Wed, 23 Oct 2024 at 02:35, Cédric Le Goater <clg@redhat.com> wrote:
>>
>> On 10/22/24 13:54, Joel Stanley wrote:
>>> On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> wrote:
>>>
>>>> 3. Test HACE model with u-boot hash command
>>>> a. load test file to address 83000000 via tftp
>>>> ast# tftp 83000000 jamin_lin/32MB
>>>> b. get sha256
>>>> ast# hash sha256 83000000 2000000
>>>> sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736
>>>> c. get sha384
>>>> ast# hash sha384 83000000 2000000
>>>> sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591
>>>> d. get sha512
>>>> ast# hash sha512 83000000 2000000
>>>> sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478
>>>
>>> I attempted this same test and noticed that the 'hash' command was not
>>> using the hardware. You can see this by putting some printf or
>>> breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's some
>>> missing work on the u-boot side to move the "hash" command over to the
>>> hash uclass, so it can be used to test this code path (or add support
>>> for the old API to the hace driver).
>>>
>>> Separately, I attempted to test with u-boot by enabling hash
>>> verification of the FIT image, and it fails to calculate the correct
>>> SHA.
>>>
>>> I think to have any confidence that this model works, we need to add
>>> some testing to qemu. I did this for the initial version of the model
>>> in tests/qtest/aspeed_hace-test.c.
>>
>> There are "accumulative mode" tests in QEMU, which were added by commit
>> e0c371a0d23b ("tests/qtest: Add test for Aspeed HACE accumulative mode")
>> They pass today with this patch. Are you suggesting we should add more?
> 
> I was trying to find a test case that showed the behaviour was broken
> before (segfault?) and fixed after, but haven't had any luck so far.
> 
> The tests pass before this patch, and they pass after it. By that
> logic there's no problem merging this one:
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks,

> 
> Someone (aspeed?) should take a todo to resolve the HACE situation in u-boot.

I will build a br2 image with upstream u-boot. The ones we use for
tests have an OpenBMC u-boot IIRC.

Cheers,

C.