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

[Jamin Lin]

Hi Joel and Cedric

> Subject: Re: [PATCH v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing
> 
> 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 ==>
> >>>>
> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77
> 91
> >>>> 6d47084c954254f101fc0f10c0591
> >>>> d. get sha512
> >>>> ast# hash sha512 83000000 2000000
> >>>> sha512 for 83000000 ... 84ffffff ==>
> >>>>
> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498
> >>>> e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478
> >>>
> >>> 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.
> 

I used ASPEED FORKED OpenBMC pre-built image and hardware hash engine was 
enabled by default with u-boot hash command.
https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.03/ast2600-default-obmc.tar.gz
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/spl.c#L46
 
I added "printf" in "do_hash_operation" function and ensure this function was 
called in HACE model.

ast# hash sha256 83000000 8000000
jamin do_hash_operation
jamin do_hash_operation: qcrypto_hash_new
jamin do_hash_operation: acc_mode
sha256 for 83000000 ... 8affffff ==> 
652dcd794aabcbde2fe4480398ad5da3917cfb90d26baa64910fc4bea4471646

Thanks-Jamin

> Cheers,
> 
> C.