Re: [PATCH v1] s390/pkey: Use -ENODEV instead of -EOPNOTSUPP

[David Hildenbrand]

On 03.06.19 11:08, Cornelia Huck wrote:
> On Mon, 3 Jun 2019 10:14:53 +0200
> David Hildenbrand <david@xxxxxxxxxx> wrote:
> 
>> On 03.06.19 09:48, Harald Freudenberger wrote:
>>> On 31.05.19 11:36, David Hildenbrand wrote:  
>>>> systemd-modules-load.service automatically tries to load the pkey module
>>>> on systems that have MSA.
>>>>
>>>> Pkey also requires the MSA3 facility and a bunch of subfunctions.
>>>> Failing with -EOPNOTSUPP makes "systemd-modules-load.service" fail on
>>>> any system that does not have all needed subfunctions. For example,
>>>> when running under QEMU TCG (but also on systems where protected keys
>>>> are disabled via the HMC).
>>>>
>>>> Let's use -ENODEV, so systemd-modules-load.service properly ignores
>>>> failing to load the pkey module because of missing HW functionality.
>>>>
>>>> Cc: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>>>> Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
>>>> Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
>>>> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>>>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>>>> ---
>>>>  drivers/s390/crypto/pkey_api.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
>>>> index 45eb0c14b880..ddfcefb47284 100644
>>>> --- a/drivers/s390/crypto/pkey_api.c
>>>> +++ b/drivers/s390/crypto/pkey_api.c
>>>> @@ -1695,15 +1695,15 @@ static int __init pkey_init(void)
>>>>  	 * are able to work with protected keys.
>>>>  	 */
>>>>  	if (!cpacf_query(CPACF_PCKMO, &pckmo_functions))
>>>> -		return -EOPNOTSUPP;
>>>> +		return -ENODEV;
>>>>  
>>>>  	/* check for kmc instructions available */
>>>>  	if (!cpacf_query(CPACF_KMC, &kmc_functions))
>>>> -		return -EOPNOTSUPP;
>>>> +		return -ENODEV;
>>>>  	if (!cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_128) ||
>>>>  	    !cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_192) ||
>>>>  	    !cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_256))
>>>> -		return -EOPNOTSUPP;
>>>> +		return -ENODEV;
>>>>  
>>>>  	pkey_debug_init();
>>>>    
>>> I can't really agree to this: there are a lot more modules returning
>>> EOPNOTSUPP, for example have a look into the arch/s390/crypto
>>> subdirectory. The ghash_s390 module also registers for MSA feature
>>> and also returns EOPNOTSUPPORTED when the required hardware extension  
>>
>> For s390x KVM, we return ENODEV in case the SIE (the HW feature) is not
>> available. Just because s390x crypto is doing it consistently this way
>> doesn't mean it is the right thing to do.
>>
>> Maybe we should change all s390x crypto modules then.
> 
> I agree.
> 
>>
>>> is not available. Same with the prng kernel module, sha1_s390, sha256_s390
>>> and I assume there is a bunch of other kernel modules with same behavior.
>>> I would prefer having this fixed on the systemd-modules-load.service side.  
>>
>>
>> A very, very bad comparison (because it contains a lot of false positives):
>>
>> t460s: ~/git/linux memory_block_devices2 $ git grep -A 20 "_init("  --
>> 'drivers*.[c]' | grep ENODEV | wc -l
>> 1552
>>
>> t460s: ~/git/linux memory_block_devices2 $ git grep -A 20 "_init("  --
>> 'drivers*.[c]' | grep EOPNOTSUPP | wc -l
>> 56
>>
>> No, I don't think EOPNOTSUPP is the right thing to do.
> 
> If we frame it as
> -EOPNOTSUPP -> operation not supported (i.e. we cannot perform this
> operation)
> -ENODEV -> no such device (i.e. we're lacking hardware support)
> 
> I think -ENODEV makes more sense (even though we could argue for both.)
> And it is an easy change to make...
> 

So do we have an agreement to change all s390x crypto users to ENODEV?

-- 

Thanks,

David / dhildenb