Re: [PATCH v11 3/6] s390/crypto: New s390 specific protected key hash phmac

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

On 2025-05-23 16:52, Holger Dengler wrote:
> On 22/05/2025 10:57, Harald Freudenberger wrote:
>> Add support for protected key hmac ("phmac") for s390 arch.
>> 
>> With the latest machine generation there is now support for
>> protected key (that is a key wrapped by a master key stored
>> in firmware) hmac for sha2 (sha224, sha256, sha384 and sha512)
>> for the s390 specific CPACF instruction kmac.
>> 
>> This patch adds support via 4 new ahashes registered as
>> phmac(sha224), phmac(sha256), phmac(sha384) and phmac(sha512).
>> 
>> Co-developed-by: Holger Dengler <dengler@linux.ibm.com>
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> See my comments below. The rest looks good to me.
> 
>> ---
>>  arch/s390/configs/debug_defconfig |   1 +
>>  arch/s390/configs/defconfig       |   1 +
>>  arch/s390/crypto/Makefile         |   1 +
>>  arch/s390/crypto/phmac_s390.c     | 899 
>> ++++++++++++++++++++++++++++++
>>  drivers/crypto/Kconfig            |  12 +
>>  5 files changed, 914 insertions(+)
>>  create mode 100644 arch/s390/crypto/phmac_s390.c
>> 
> [...]
>> diff --git a/arch/s390/crypto/phmac_s390.c 
>> b/arch/s390/crypto/phmac_s390.c
>> new file mode 100644
>> index 000000000000..b61454dedf0a
>> --- /dev/null
>> +++ b/arch/s390/crypto/phmac_s390.c
>> @@ -0,0 +1,899 @@
> [...]
>> +struct kmac_sha2_ctx {
>> +	u8 param[MAX_DIGEST_SIZE + MAX_IMBL_SIZE + PHMAC_MAX_PK_SIZE];
>> +	union kmac_gr0 gr0;
>> +	u8 buf[MAX_BLOCK_SIZE];
>> +	unsigned long total;
> 
> The clear-key hmac switched to u64[2] instead of unsinged long for
> `total`. Please synchronize with clear-key hmac.
> 

I've picked these changes from linux-next and reworked the affected
code parts to use a u64 buflen[2] now.

>> +};
>> +
> [...]
>> +/*
>> + * kmac_sha2_set_imbl - sets the input message bit-length based on 
>> the blocksize
>> + */
>> +static inline void kmac_sha2_set_imbl(u8 *param, unsigned long 
>> nbytes,
>> +				      unsigned int blocksize)
> 
> Same here. I would prefer to use this in the phmac as well.
> 
> [...]
>> +static void s390_phmac_exit(void)
>> +{
>> +	struct phmac_alg *phmac;
>> +	int i;
>> +
>> +	if (phmac_crypto_engine) {
>> +		crypto_engine_stop(phmac_crypto_engine);
>> +		crypto_engine_exit(phmac_crypto_engine);
>> +	}
>> +
>> +	for (i = ARRAY_SIZE(phmac_algs) - 1; i >= 0; i--) {
>> +		phmac = &phmac_algs[i];
>> +		if (phmac->registered)
>> +			crypto_engine_unregister_ahash(&phmac->alg);
>> +	}
>> +
>> +	misc_deregister(&phmac_dev);
> 
> The misc_register() call in module init is missing, see my comment to 
> patch 5/6.
> 

Yea. Somehow this code piece moved into patch #5 instead of #3.
Done, see v12.

>> +}
>> +
>> +static int __init s390_phmac_init(void)
>> +{
>> +	struct phmac_alg *phmac;
>> +	int i, rc;
>> +
> 
> The misc_regsiter(&phmac_dev) should go here.
> 
> [...]