Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 16 Feb 2022 10:57:33 +0100
From:   Petr Vorel <pvorel@suse.cz>
To:     Nicolai Stange <nstange@suse.de>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>, leitao@debian.org,
        Nayna Jain <nayna@linux.ibm.com>,
        Paulo Flabiano Smorigo <pfsmorigo@gmail.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kbuild@vger.kernel.org
Subject: Re: [PATCH v2 1/2] crypto: vmx: Turn CRYPTO_DEV_VMX_ENCRYPT into
 tristate
Message-ID: <YgzKjVKsNXtqqXOy@pevik>
Reply-To: Petr Vorel <pvorel@suse.cz>
References: <20220215185936.15576-1-pvorel@suse.cz>
 <20220215185936.15576-2-pvorel@suse.cz>
 <87tuczf96a.fsf@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87tuczf96a.fsf@suse.de>
Precedence: bulk

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