2022-02-16 04:17:45

by Petr Vorel

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

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 <[email protected]>
---
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
...
[email protected] (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: [email protected]
S: Supported
F: drivers/scsi/ibmvscsi_tgt/

-IBM Power VMX Cryptographic instructions
+IBM Power VMX Cryptographic Acceleration Instructions Driver
M: Breno Leitão <[email protected]>
M: Nayna Jain <[email protected]>
M: Paulo Flabiano Smorigo <[email protected]>
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


2022-02-16 07:55:32

by Petr Vorel

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

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

2022-02-16 09:39:45

by Nicolai Stange

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

Hi Petr,

Petr Vorel <[email protected]> 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 <[email protected]>
> ---
> 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
> ...
> [email protected] (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: [email protected]
> S: Supported
> F: drivers/scsi/ibmvscsi_tgt/
>
> -IBM Power VMX Cryptographic instructions
> +IBM Power VMX Cryptographic Acceleration Instructions Driver
> M: Breno Leitão <[email protected]>
> M: Nayna Jain <[email protected]>
> M: Paulo Flabiano Smorigo <[email protected]>
> 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

2022-02-16 09:57:42

by Petr Vorel

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

Hi Nicolai,

thanks for all your comments.

> Hi Petr,

> Petr Vorel <[email protected]> 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 <[email protected]>
> > ---
> > 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
> > ...
> > [email protected] (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

2022-02-16 10:03:32

by Petr Vorel

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

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