[PATCH v2 0/2] vmx-crypto: Add missing dependencies

[PATCH v2 0/2] vmx-crypto: Add missing dependencies

[Petr Vorel]

Hi,

[ Cc powerpc list and VMX people this time ]

changes v1->v2:
* new commit: crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate
* use "select" instead of "depends on" (Nicolai)
* drop !CRYPTO_MANAGER_DISABLE_TESTS as the dependency is always (Nicolai)

Petr Vorel (2):
  crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate
  crypto: vmx: Add missing dependencies

 MAINTAINERS                            | 2 +-
 arch/powerpc/configs/powernv_defconfig | 2 +-
 arch/powerpc/configs/ppc64_defconfig   | 2 +-
 arch/powerpc/configs/pseries_defconfig | 2 +-
 drivers/crypto/Kconfig                 | 6 ------
 drivers/crypto/vmx/Kconfig             | 8 ++++++--
 6 files changed, 10 insertions(+), 12 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate

[Petr Vorel]

and remove CRYPTO_DEV_VMX, which looked redundant when only
CRYPTO_DEV_VMX_ENCRYPT used it. Also it forces CRYPTO_GHASH to be
builtin even CRYPTO_DEV_VMX_ENCRYPT was configured as module.

Update powerpc defconfigs and description in MAINTAINERS.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
new in v2

This might be a bit aggressive, but IMHO CRYPTO_DEV_VMX only complicated
things for nothing. But if you do *not* agree with removing it, I just add
select to drivers/crypto/vmx/Kconfig (which forces dependencies to be
always modules.)

If it's ok for you to remove, please also check whether the description
is ok. get_maintainer.pl script has size limitation:

$ ./scripts/get_maintainer.pl drivers/crypto/vmx/Kconfig
...
linux-crypto@vger.kernel.org (open list:IBM Power VMX Cryptographic Acceleration Instru...)

maybe the name should be shorter.

Kind regards,
Petr

 MAINTAINERS                            | 2 +-
 arch/powerpc/configs/powernv_defconfig | 2 +-
 arch/powerpc/configs/ppc64_defconfig   | 2 +-
 arch/powerpc/configs/pseries_defconfig | 2 +-
 drivers/crypto/Kconfig                 | 6 ------
 drivers/crypto/vmx/Kconfig             | 4 ++--
 6 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index ea3e6c914384..80e562579180 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9207,7 +9207,7 @@ L:	target-devel@vger.kernel.org
 S:	Supported
 F:	drivers/scsi/ibmvscsi_tgt/
 
-IBM Power VMX Cryptographic instructions
+IBM Power VMX Cryptographic Acceleration Instructions Driver
 M:	Breno Leitão <leitao@debian.org>
 M:	Nayna Jain <nayna@linux.ibm.com>
 M:	Paulo Flabiano Smorigo <pfsmorigo@gmail.com>
diff --git a/arch/powerpc/configs/powernv_defconfig b/arch/powerpc/configs/powernv_defconfig
index 49f49c263935..4b250d05dcdf 100644
--- a/arch/powerpc/configs/powernv_defconfig
+++ b/arch/powerpc/configs/powernv_defconfig
@@ -337,7 +337,7 @@ CONFIG_CRYPTO_TEA=m
 CONFIG_CRYPTO_TWOFISH=m
 CONFIG_CRYPTO_LZO=m
 CONFIG_CRYPTO_DEV_NX=y
-CONFIG_CRYPTO_DEV_VMX=y
+CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
 CONFIG_VIRTUALIZATION=y
 CONFIG_KVM_BOOK3S_64=m
 CONFIG_KVM_BOOK3S_64_HV=m
diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig
index c8b0e80d613b..ebd33b94debb 100644
--- a/arch/powerpc/configs/ppc64_defconfig
+++ b/arch/powerpc/configs/ppc64_defconfig
@@ -355,7 +355,7 @@ CONFIG_CRYPTO_TWOFISH=m
 CONFIG_CRYPTO_LZO=m
 CONFIG_CRYPTO_DEV_NX=y
 CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
-CONFIG_CRYPTO_DEV_VMX=y
+CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
 CONFIG_PRINTK_TIME=y
 CONFIG_PRINTK_CALLER=y
 CONFIG_MAGIC_SYSRQ=y
diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
index b571d084c148..304673817ef1 100644
--- a/arch/powerpc/configs/pseries_defconfig
+++ b/arch/powerpc/configs/pseries_defconfig
@@ -315,7 +315,7 @@ CONFIG_CRYPTO_TWOFISH=m
 CONFIG_CRYPTO_LZO=m
 CONFIG_CRYPTO_DEV_NX=y
 CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
-CONFIG_CRYPTO_DEV_VMX=y
+CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
 CONFIG_VIRTUALIZATION=y
 CONFIG_KVM_BOOK3S_64=m
 CONFIG_KVM_BOOK3S_64_HV=m
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4f705674f94f..956f956607a5 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -761,12 +761,6 @@ config CRYPTO_DEV_QCOM_RNG
 	  To compile this driver as a module, choose M here. The
 	  module will be called qcom-rng. If unsure, say N.
 
-config CRYPTO_DEV_VMX
-	bool "Support for VMX cryptographic acceleration instructions"
-	depends on PPC64 && VSX
-	help
-	  Support for VMX cryptographic acceleration instructions.
-
 source "drivers/crypto/vmx/Kconfig"
 
 config CRYPTO_DEV_IMGTEC_HASH
diff --git a/drivers/crypto/vmx/Kconfig b/drivers/crypto/vmx/Kconfig
index c85fab7ef0bd..1a3808b719f3 100644
--- a/drivers/crypto/vmx/Kconfig
+++ b/drivers/crypto/vmx/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config CRYPTO_DEV_VMX_ENCRYPT
-	tristate "Encryption acceleration support on P8 CPU"
-	depends on CRYPTO_DEV_VMX
+	tristate "Power VMX cryptographic acceleration instructions driver"
+	depends on PPC64 && VSX
 	select CRYPTO_GHASH
 	default m
 	help
-- 
2.35.1

[PATCH v2 2/2] crypto: vmx: Add missing dependencies

[Petr Vorel]

vmx-crypto module depends on CRYPTO_AES, CRYPTO_CBC, CRYPTO_CTR or
CRYPTO_XTS, thus add them.

These dependencies are likely to be enabled, but if
CRYPTO_DEV_VMX_ENCRYPT=y && !CRYPTO_MANAGER_DISABLE_TESTS
and either of CRYPTO_AES, CRYPTO_CBC, CRYPTO_CTR or CRYPTO_XTS is built
as module or disabled, alg_test() from crypto/testmgr.c complains during
boot about failing to allocate the generic fallback implementations
(2 == ENOENT):

[    0.540953] Failed to allocate xts(aes) fallback: -2
[    0.541014] alg: skcipher: failed to allocate transform for p8_aes_xts: -2
[    0.541120] alg: self-tests for p8_aes_xts (xts(aes)) failed (rc=-2)
[    0.544440] Failed to allocate ctr(aes) fallback: -2
[    0.544497] alg: skcipher: failed to allocate transform for p8_aes_ctr: -2
[    0.544603] alg: self-tests for p8_aes_ctr (ctr(aes)) failed (rc=-2)
[    0.547992] Failed to allocate cbc(aes) fallback: -2
[    0.548052] alg: skcipher: failed to allocate transform for p8_aes_cbc: -2
[    0.548156] alg: self-tests for p8_aes_cbc (cbc(aes)) failed (rc=-2)
[    0.550745] Failed to allocate transformation for 'aes': -2
[    0.550801] alg: cipher: Failed to load transform for p8_aes: -2
[    0.550892] alg: self-tests for p8_aes (aes) failed (rc=-2)

Fixes: c07f5d3da643 ("crypto: vmx - Adding support for XTS")
Fixes: d2e3ae6f3aba ("crypto: vmx - Enabling VMX module for PPC64")

Suggested-by: Nicolai Stange <nstange@suse.de>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
changes v1->v2:
* use "select" instead of "depends on" (Nicolai)
* drop !CRYPTO_MANAGER_DISABLE_TESTS as the dependency is always (Nicolai)

 drivers/crypto/vmx/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/vmx/Kconfig b/drivers/crypto/vmx/Kconfig
index 1a3808b719f3..ce054e64b92c 100644
--- a/drivers/crypto/vmx/Kconfig
+++ b/drivers/crypto/vmx/Kconfig
@@ -2,7 +2,11 @@
 config CRYPTO_DEV_VMX_ENCRYPT
 	tristate "Power VMX cryptographic acceleration instructions driver"
 	depends on PPC64 && VSX
+	select CRYPTO_AES
+	select CRYPTO_CBC
+	select CRYPTO_CTR
 	select CRYPTO_GHASH
+	select CRYPTO_XTS
 	default m
 	help
 	  Support for VMX cryptographic acceleration instructions on Power8 CPU.
-- 
2.35.1

Re: [PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate

[Petr Vorel]

Hi,

I kept CRYPTO_DEV_VMX_ENCRYPT in drivers/crypto/vmx/Kconfig,
but maybe I should have moved it into drivers/crypto/Kconfig.

It's not clear what is preferred.

Kind regards,
Petr

Re: [PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate

[Nicolai Stange]

Hi Petr,

Petr Vorel <pvorel@suse.cz> writes:

> and remove CRYPTO_DEV_VMX, which looked redundant when only
> CRYPTO_DEV_VMX_ENCRYPT used it. Also it forces CRYPTO_GHASH to be
> builtin even CRYPTO_DEV_VMX_ENCRYPT was configured as module.

I'm confused by the description. CRYPTO_DEV_VMX_ENCRYPT has been a
tristate since ever? And thus, with CRYPTO_DEV_VMX_ENCRYPT=m,
CRYPTO_GHASH=m would be possible as far as vmx is concerned?

What this patch really does is to merge CRYPTO_DEV_VMX into
CRYPTO_DEV_VMX_ENCRYPT AFAICS.

These two seem indeed redundant to me, but for consistency with the
other crypto drivers (e.g. bcm, ccree, ...), I'd rather keep
CRYPTO_DEV_VMX and merge CRYPTO_DEV_VMX_ENCRYPT into it.


> Update powerpc defconfigs and description in MAINTAINERS.

The change to MAINTAINERS is completely unrelated? If anything, it had
to come with a separate patch then.


>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> new in v2
>
> This might be a bit aggressive, but IMHO CRYPTO_DEV_VMX only complicated
> things for nothing.

I agree on the redundancy, but as said, CRYPTO_DEV_VMX_ENCRYPT should
probably the one to get dropped in favor of CRYPTO_DEV_VMX.


> But if you do *not* agree with removing it, I just add
> select to drivers/crypto/vmx/Kconfig (which forces dependencies to be
> always modules.)
>
> If it's ok for you to remove, please also check whether the description
> is ok. get_maintainer.pl script has size limitation:
>
> $ ./scripts/get_maintainer.pl drivers/crypto/vmx/Kconfig
> ...
> linux-crypto@vger.kernel.org (open list:IBM Power VMX Cryptographic Acceleration Instru...)
>
> maybe the name should be shorter.
>
> Kind regards,
> Petr
>
>  MAINTAINERS                            | 2 +-
>  arch/powerpc/configs/powernv_defconfig | 2 +-
>  arch/powerpc/configs/ppc64_defconfig   | 2 +-
>  arch/powerpc/configs/pseries_defconfig | 2 +-
>  drivers/crypto/Kconfig                 | 6 ------
>  drivers/crypto/vmx/Kconfig             | 4 ++--
>  6 files changed, 6 insertions(+), 12 deletions(-)

If you were to drop CONFIG_CRYPTO_DEV_VMX (like it's implemented in this
patch), then something had to be done about

  obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/

in drivers/crypto/Makefile as well.

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea3e6c914384..80e562579180 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9207,7 +9207,7 @@ L:	target-devel@vger.kernel.org
>  S:	Supported
>  F:	drivers/scsi/ibmvscsi_tgt/
>  
> -IBM Power VMX Cryptographic instructions
> +IBM Power VMX Cryptographic Acceleration Instructions Driver
>  M:	Breno Leitão <leitao@debian.org>
>  M:	Nayna Jain <nayna@linux.ibm.com>
>  M:	Paulo Flabiano Smorigo <pfsmorigo@gmail.com>
> diff --git a/arch/powerpc/configs/powernv_defconfig b/arch/powerpc/configs/powernv_defconfig
> index 49f49c263935..4b250d05dcdf 100644
> --- a/arch/powerpc/configs/powernv_defconfig
> +++ b/arch/powerpc/configs/powernv_defconfig
> @@ -337,7 +337,7 @@ CONFIG_CRYPTO_TEA=m
>  CONFIG_CRYPTO_TWOFISH=m
>  CONFIG_CRYPTO_LZO=m
>  CONFIG_CRYPTO_DEV_NX=y
> -CONFIG_CRYPTO_DEV_VMX=y
> +CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
>  CONFIG_VIRTUALIZATION=y
>  CONFIG_KVM_BOOK3S_64=m
>  CONFIG_KVM_BOOK3S_64_HV=m
> diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig
> index c8b0e80d613b..ebd33b94debb 100644
> --- a/arch/powerpc/configs/ppc64_defconfig
> +++ b/arch/powerpc/configs/ppc64_defconfig
> @@ -355,7 +355,7 @@ CONFIG_CRYPTO_TWOFISH=m
>  CONFIG_CRYPTO_LZO=m
>  CONFIG_CRYPTO_DEV_NX=y
>  CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
> -CONFIG_CRYPTO_DEV_VMX=y
> +CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
>  CONFIG_PRINTK_TIME=y
>  CONFIG_PRINTK_CALLER=y
>  CONFIG_MAGIC_SYSRQ=y
> diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
> index b571d084c148..304673817ef1 100644
> --- a/arch/powerpc/configs/pseries_defconfig
> +++ b/arch/powerpc/configs/pseries_defconfig
> @@ -315,7 +315,7 @@ CONFIG_CRYPTO_TWOFISH=m
>  CONFIG_CRYPTO_LZO=m
>  CONFIG_CRYPTO_DEV_NX=y
>  CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
> -CONFIG_CRYPTO_DEV_VMX=y
> +CONFIG_CRYPTO_DEV_VMX_ENCRYPT=m
>  CONFIG_VIRTUALIZATION=y
>  CONFIG_KVM_BOOK3S_64=m
>  CONFIG_KVM_BOOK3S_64_HV=m
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 4f705674f94f..956f956607a5 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -761,12 +761,6 @@ config CRYPTO_DEV_QCOM_RNG
>  	  To compile this driver as a module, choose M here. The
>  	  module will be called qcom-rng. If unsure, say N.
>  
> -config CRYPTO_DEV_VMX
> -	bool "Support for VMX cryptographic acceleration instructions"
> -	depends on PPC64 && VSX
> -	help
> -	  Support for VMX cryptographic acceleration instructions.
> -

As said, I'd keep this one (while moving the GHASH dependency here) ...


>  source "drivers/crypto/vmx/Kconfig"

... and drop this one.

Thanks,

Nicolai


>  
>  config CRYPTO_DEV_IMGTEC_HASH
> diff --git a/drivers/crypto/vmx/Kconfig b/drivers/crypto/vmx/Kconfig
> index c85fab7ef0bd..1a3808b719f3 100644
> --- a/drivers/crypto/vmx/Kconfig
> +++ b/drivers/crypto/vmx/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config CRYPTO_DEV_VMX_ENCRYPT
> -	tristate "Encryption acceleration support on P8 CPU"
> -	depends on CRYPTO_DEV_VMX
> +	tristate "Power VMX cryptographic acceleration instructions driver"
> +	depends on PPC64 && VSX
>  	select CRYPTO_GHASH
>  	default m
>  	help

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Ivo Totev

Re: [PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate

[Petr Vorel]

Hi Nicolai,

thanks for all your comments.

> Hi Petr,

> Petr Vorel <pvorel@suse.cz> writes:

> > and remove CRYPTO_DEV_VMX, which looked redundant when only
> > CRYPTO_DEV_VMX_ENCRYPT used it. Also it forces CRYPTO_GHASH to be
> > builtin even CRYPTO_DEV_VMX_ENCRYPT was configured as module.

> I'm confused by the description. CRYPTO_DEV_VMX_ENCRYPT has been a
> tristate since ever? And thus, with CRYPTO_DEV_VMX_ENCRYPT=m,
> CRYPTO_GHASH=m would be possible as far as vmx is concerned?

I'm sorry, the description is wrong.

I'm not kconfig expert and I verify it again, but I run into it when testing
this with defconfig:
$ make defconfig && scripts/config --enable CONFIG_KVM_GUEST --disable
CRYPTO_MANAGER_DISABLE_TESTS --enable CONFIG_MODULE_SIG

It somehow inherits CRYPTO_DEV_VMX=y. But I'll verify it again.

> What this patch really does is to merge CRYPTO_DEV_VMX into
> CRYPTO_DEV_VMX_ENCRYPT AFAICS.
+1 This is a proper description.

> These two seem indeed redundant to me, but for consistency with the
> other crypto drivers (e.g. bcm, ccree, ...), I'd rather keep
> CRYPTO_DEV_VMX and merge CRYPTO_DEV_VMX_ENCRYPT into it.
I'm not sure myself, because some other modules also have Kconfig.

$ ls -1 drivers/crypto/*/Kconfig
drivers/crypto/allwinner/Kconfig
drivers/crypto/amlogic/Kconfig
drivers/crypto/caam/Kconfig
drivers/crypto/ccp/Kconfig
drivers/crypto/hisilicon/Kconfig
drivers/crypto/chelsio/Kconfig
drivers/crypto/keembay/Kconfig
drivers/crypto/marvell/Kconfig
drivers/crypto/nx/Kconfig
drivers/crypto/qat/Kconfig
drivers/crypto/stm32/Kconfig
drivers/crypto/ux500/Kconfig
drivers/crypto/virtio/Kconfig
drivers/crypto/vmx/Kconfig

Sure, some of them have many config options in Kconfig, but
at least drivers/crypto/chelsio/Kconfig and drivers/crypto/virtio/Kconfig
configure just the module. Given it's just these two, I should probably merge
CRYPTO_DEV_VMX_ENCRYPT into CRYPTO_DEV_VMX as you suggest (also defconfig
changes would not be needed).
@Herbert, Nayna, Paulo, Breno: any preference of these.

> > Update powerpc defconfigs and description in MAINTAINERS.

> The change to MAINTAINERS is completely unrelated? If anything, it had
> to come with a separate patch then.

You're right, I was hesitating myself.


> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > new in v2

> > This might be a bit aggressive, but IMHO CRYPTO_DEV_VMX only complicated
> > things for nothing.

> I agree on the redundancy, but as said, CRYPTO_DEV_VMX_ENCRYPT should
> probably the one to get dropped in favor of CRYPTO_DEV_VMX.


> > But if you do *not* agree with removing it, I just add
> > select to drivers/crypto/vmx/Kconfig (which forces dependencies to be
> > always modules.)

> > If it's ok for you to remove, please also check whether the description
> > is ok. get_maintainer.pl script has size limitation:

> > $ ./scripts/get_maintainer.pl drivers/crypto/vmx/Kconfig
> > ...
> > linux-crypto@vger.kernel.org (open list:IBM Power VMX Cryptographic Acceleration Instru...)

> > maybe the name should be shorter.

> > Kind regards,
> > Petr

> >  MAINTAINERS                            | 2 +-
> >  arch/powerpc/configs/powernv_defconfig | 2 +-
> >  arch/powerpc/configs/ppc64_defconfig   | 2 +-
> >  arch/powerpc/configs/pseries_defconfig | 2 +-
> >  drivers/crypto/Kconfig                 | 6 ------
> >  drivers/crypto/vmx/Kconfig             | 4 ++--
> >  6 files changed, 6 insertions(+), 12 deletions(-)

> If you were to drop CONFIG_CRYPTO_DEV_VMX (like it's implemented in this
> patch), then something had to be done about

>   obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/

> in drivers/crypto/Makefile as well.

+1 (I obviously forget to amend).

Kind regards,
Petr

...
> > +++ b/drivers/crypto/Kconfig
> > @@ -761,12 +761,6 @@ config CRYPTO_DEV_QCOM_RNG
> >  	  To compile this driver as a module, choose M here. The
> >  	  module will be called qcom-rng. If unsure, say N.

> > -config CRYPTO_DEV_VMX
> > -	bool "Support for VMX cryptographic acceleration instructions"
> > -	depends on PPC64 && VSX
> > -	help
> > -	  Support for VMX cryptographic acceleration instructions.
> > -

> As said, I'd keep this one (while moving the GHASH dependency here) ...


> >  source "drivers/crypto/vmx/Kconfig"

> ... and drop this one.

> Thanks,

> Nicolai

Re: [PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into tristate

[Petr Vorel]

Hi,

side notes about the subject (following private notes from Nicolai):

I dared to use "crypto: vmx: " in subject, next version I'll use "crypto: vmx - "
as it's used in crypto.

Also continue the subject line into the rest of the commit message isn't
probably wanted.

Kind regards,
Petr