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

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

[David Hildenbrand]

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@linux.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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();
 
-- 
2.20.1

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

[Cornelia Huck]

On Fri, 31 May 2019 11:36:28 +0200
David Hildenbrand <david@redhat.com> 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@linux.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/s390/crypto/pkey_api.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Looking at what other modules return when needed features are missing,
this looks like the right thing to do.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Harald Freudenberger]

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@linux.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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
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.

regards
Harald Freudenberger

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

[David Hildenbrand]

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@linux.ibm.com>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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.

> 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.

-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Mon, 3 Jun 2019 10:14:53 +0200
David Hildenbrand <david@redhat.com> 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@linux.ibm.com>
> >> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> >> Cc: Cornelia Huck <cohuck@redhat.com>
> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  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...

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@redhat.com> 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@linux.ibm.com>
>>>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  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