Re: [PATCH v11 5/6] s390/crypto: Add selftest support for phmac

[Harald Freudenberger <freude@linux.ibm.com>]

On 2025-05-23 16:55, Holger Dengler wrote:
> On 22/05/2025 10:57, Harald Freudenberger wrote:
>> Add key preparation code in case of selftest running to the phmac
>> setkey function:
>> 
>> As long as the CRYPTO_ALG_TESTED flag is not set, all setkey()
>> invocations are assumed to carry sheer hmac clear key values and thus
>> need some preparation to work with the phmac implementation. Thus it
>> is possible to use the already available hmac test vectors implemented
>> in the testmanager to test the phmac code.
>> 
>> When the CRYPTO_ALG_TESTED flag is set (after larval state) the phmac
>> code assumes the key material is a blob digestible by the pkey kernel
>> module which converts the blob into a working key for the phmac code.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> For this series, the explicit calls of CPACF for hashing long keys is
> ok. Long term, I would suggest to switch to introduce library
> functions for sha224(), sha384() and sha512(), all similar to
> sha256(), and use them for hashing the clear key. This will remove the
> hard dependency to CPACF SHA function codes for this module. But we
> should do this in a separate series and do the same for clear-key
> hmac.
> 

D'accord - but let's first focus on the real implementation and when all
this rework on the next kernel is done, we can start to use the library
functions instead.

> See another comment below. The rest looks good.
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 
> 
> [...]
>> @@ -854,6 +986,17 @@ static int __init s390_phmac_init(void)
> [...]
>> +	/* register a simple phmac pseudo misc device */
>> +	rc = misc_register(&phmac_dev);
>> +	if (rc)
>> +		return rc;
>> +
> 
> These 5 lines should go into patch 3/6, as it uses `phmac_dev` already.
> 
>>  	/* with this pseudo device alloc and start a crypto engine */
>>  	phmac_crypto_engine =
>>  		crypto_engine_alloc_init_and_set(phmac_dev.this_device,

Done - this code piece really should have been in patch #3.