Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
(1 comment)
Patch Set 5:
Patch Set 5:
st
Not sure why I got an "st" on this. Apologies. I think Angel is commenting that we are doing some byte swizzling and that the preferred implementation is to treat the buffer in both a: the right size, and b: not swizzle if possible. So we should explore this in a follow up bug with AMD on refining the svc call.
Angel to agree/disagree.
Looks like the byte swizzling is necessary because the PSP reads the data buffer as an array of dwords (32-bit values). Given the documentation I have (none), I guess this is the reason why one needs to swap the byte order. If so, the pointer types of the MOD_EXP_PARAMS_T struct (as well as any other affected structs) should be changed accordingly.
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
This isn't a hardware buffer. It's just a memory buffer. […]
Right, I checked the PSP interface and there's no hardware buffer. However, the svc call provides no flexibility (nor documentation) to change how the PSP reads the memory buffer. I assume PSP always reads the data as 32-bit LE values, which is why swab32 is needed here.