Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Krzysztof Kozlowski]

On Wed, Apr 09, 2025 at 11:00:32PM GMT, Nipun Gupta wrote:
> The AMD PKI accelerator driver provides a accel interface to interact
> with the device for offloading and accelerating asymmetric crypto
> operations.
> 

For me this is clearly a crypto driver and you are supposed to:
1. Cc crypto maintaners,
2. Put it actually into crypto and use crypto API.


Limited review follows.

> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> ---
> 
> Changes RFC->v2:
> - moved from misc to accel
> - added architecture and compile test dependency in Kconfig
> - removed sysfs (and added debugfs in new patch 3/3)
> - fixed platform compat
> - removed redundant resource index 1 configuration (which was there in
>   RFC patch)
> 
>  MAINTAINERS                     |   2 +
>  drivers/accel/Kconfig           |   1 +
>  drivers/accel/Makefile          |   1 +
>  drivers/accel/amdpk/Kconfig     |  18 +
>  drivers/accel/amdpk/Makefile    |   8 +
>  drivers/accel/amdpk/amdpk_drv.c | 736 ++++++++++++++++++++++++++++++++
>  drivers/accel/amdpk/amdpk_drv.h | 271 ++++++++++++
>  include/uapi/drm/amdpk.h        |  49 +++
>  8 files changed, 1086 insertions(+)
>  create mode 100644 drivers/accel/amdpk/Kconfig
>  create mode 100644 drivers/accel/amdpk/Makefile
>  create mode 100644 drivers/accel/amdpk/amdpk_drv.c
>  create mode 100644 drivers/accel/amdpk/amdpk_drv.h
>  create mode 100644 include/uapi/drm/amdpk.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 11f8815daa77..cdc305a206aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1161,6 +1161,8 @@ L:	dri-devel@lists.freedesktop.org
>  S:	Maintained
>  T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
>  F:	Documentation/devicetree/bindings/accel/amd,versal-net-pki.yaml
> +F:	drivers/accel/amdpk/
> +F:	include/uapi/drm/amdpk.h
> 
>  AMD PMC DRIVER
>  M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> diff --git a/drivers/accel/Kconfig b/drivers/accel/Kconfig
> index 5b9490367a39..5632c6c62c15 100644
> --- a/drivers/accel/Kconfig
> +++ b/drivers/accel/Kconfig
> @@ -28,5 +28,6 @@ source "drivers/accel/amdxdna/Kconfig"
>  source "drivers/accel/habanalabs/Kconfig"
>  source "drivers/accel/ivpu/Kconfig"
>  source "drivers/accel/qaic/Kconfig"
> +source "drivers/accel/amdpk/Kconfig"

Why placing at the end?

> 
>  endif
> diff --git a/drivers/accel/Makefile b/drivers/accel/Makefile
> index a301fb6089d4..caea6d636ac8 100644
> --- a/drivers/accel/Makefile
> +++ b/drivers/accel/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_ACCEL_AMDXDNA)		+= amdxdna/
>  obj-$(CONFIG_DRM_ACCEL_HABANALABS)	+= habanalabs/
>  obj-$(CONFIG_DRM_ACCEL_IVPU)		+= ivpu/
>  obj-$(CONFIG_DRM_ACCEL_QAIC)		+= qaic/
> +obj-$(CONFIG_DRM_ACCEL_AMDPK)		+= amdpk/

Did you just add it to the end breaking any ordering? Look, there is
already AMD entry in the context.

> diff --git a/drivers/accel/amdpk/Kconfig b/drivers/accel/amdpk/Kconfig
> new file mode 100644
> index 000000000000..c0b459bb66a7
> --- /dev/null
> +++ b/drivers/accel/amdpk/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for AMD PKI accelerator for versal-net
> +#
> +
> +config DRM_ACCEL_AMDPK
> +	tristate "AMD PKI accelerator for versal-net"
> +	depends on DRM_ACCEL
> +	depends on ARM64 || COMPILE_TEST

What do you need from arm64? I don't see it from the code at all.

> +	help
> +	  Enables platform driver for AMD PKI accelerator that are designed
> +	  for high performance Public Key asymmetric crypto operations on AMD
> +	  versal-net.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called amdpk.

...

> +static void amdpk_remove_device(struct amdpk_dev *pkdev)
> +{
> +	drm_dev_unplug(&pkdev->ddev);
> +	pk_wrreg(pkdev->regs, REG_IRQ_ENABLE, 0);
> +	ida_destroy(&pkdev->avail_queues);
> +}
> +
> +static int amdpk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct amdpk_dev *pkdev;
> +	struct resource *memres;
> +	int irq, ret;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (ret < 0)
> +		return ret;
> +
> +	pkdev = devm_drm_dev_alloc(dev, &amdpk_accel_driver, typeof(*pkdev), ddev);
> +	if (IS_ERR(pkdev))
> +		return PTR_ERR(pkdev);
> +	pkdev->dev = dev;
> +
> +	memres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pkdev->regs = devm_ioremap_resource(dev, memres);

Use wrapper for these two.

> +	if (IS_ERR(pkdev->regs))
> +		return PTR_ERR(pkdev->regs);
> +	pkdev->regsphys = memres->start;
> +	platform_set_drvdata(pdev, pkdev);
> +
> +	if (platform_irq_count(pdev) != 1)

Drop, what's the benefit? DT schema ensures you have only one entry.

> +		return -ENODEV;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return -ENODEV;
> +
> +	ret = drm_dev_register(&pkdev->ddev, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "DRM register failed, ret %d", ret);
> +		return ret;
> +	}
> +
> +	return amdpk_create_device(pkdev, dev, irq);
> +}
> +
> +static void amdpk_remove(struct platform_device *pdev)
> +{
> +	struct amdpk_dev *pkdev = platform_get_drvdata(pdev);
> +
> +	amdpk_remove_device(pkdev);
> +}
> +
> +static void amdpk_shutdown(struct platform_device *pdev)
> +{
> +	amdpk_remove(pdev);

I am not sure why this is necessary. Please provide comment WHY you
need it. IOW, why do you need to disable IRQ manually here if the entire
system is shutting down?

> +}
> +
> +static const struct of_device_id amdpk_match_table[] = {
> +	{ .compatible = "amd,versal-net-pki" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, amdpk_match_table);
> +
> +static struct platform_driver amdpk_pdrv = {
> +	.probe = amdpk_probe,
> +	.remove = amdpk_remove,
> +	.shutdown = amdpk_shutdown,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = amdpk_match_table,
> +	},
> +};
> +
> +static int __init amdpk_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&amdpk_pdrv);
> +	if (ret) {
> +		pr_err("can't register platform driver\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit amdpk_exit(void)
> +{
> +	platform_driver_unregister(&amdpk_pdrv);
> +}
> +
> +module_init(amdpk_init);
> +module_exit(amdpk_exit);

Why do you need to open code module_platform_driver?

Best regards,
Krzysztof

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Gupta, Nipun]

On 10-04-2025 13:06, Krzysztof Kozlowski wrote:
> On Wed, Apr 09, 2025 at 11:00:32PM GMT, Nipun Gupta wrote:
>> The AMD PKI accelerator driver provides a accel interface to interact
>> with the device for offloading and accelerating asymmetric crypto
>> operations.
>>
> 
> For me this is clearly a crypto driver and you are supposed to:
> 1. Cc crypto maintaners,
> 2. Put it actually into crypto and use crypto API.

added crypto maintainers for comments.
IMO, as accel framework is designed to support any type of compute
accelerators, the PKI crypto accelerator in accel framework is not 
completely out of place here, as also suggested at:
https://lore.kernel.org/all/2025031352-gyration-deceit-5563@gregkh/

Having the crypto accelerator as part of accel also enables to extract
the most performance from the HW PKI engines, given that the queue
assignment is handled per drm device open call.

Regards,
Nipun

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Herbert Xu]

On Fri, Apr 11, 2025 at 10:21:03AM +0530, Gupta, Nipun wrote:
>
> added crypto maintainers for comments.
> IMO, as accel framework is designed to support any type of compute
> accelerators, the PKI crypto accelerator in accel framework is not
> completely out of place here, as also suggested at:
> https://lore.kernel.org/all/2025031352-gyration-deceit-5563@gregkh/
> 
> Having the crypto accelerator as part of accel also enables to extract
> the most performance from the HW PKI engines, given that the queue
> assignment is handled per drm device open call.

There is actually a user-space interface for asymmetric crypto
through the keyring subsystem.  Adding the maintainers of those
in case they wish to comment on your driver.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Gupta, Nipun]

On 11-04-2025 10:47, Herbert Xu wrote:
> On Fri, Apr 11, 2025 at 10:21:03AM +0530, Gupta, Nipun wrote:
>>
>> added crypto maintainers for comments.
>> IMO, as accel framework is designed to support any type of compute
>> accelerators, the PKI crypto accelerator in accel framework is not
>> completely out of place here, as also suggested at:
>> https://lore.kernel.org/all/2025031352-gyration-deceit-5563@gregkh/
>>
>> Having the crypto accelerator as part of accel also enables to extract
>> the most performance from the HW PKI engines, given that the queue
>> assignment is handled per drm device open call.
> 
> There is actually a user-space interface for asymmetric crypto
> through the keyring subsystem.  Adding the maintainers of those
> in case they wish to comment on your driver.

AFAIU after looking into it, the keyring subsystem is not to perform the 
data operations, but for managing keys for these operations. Kindly 
correct me if I am wrong here.

Thanks,
Nipun

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Herbert Xu]

On Fri, Apr 11, 2025 at 11:50:54PM +0530, Gupta, Nipun wrote:
>
> AFAIU after looking into it, the keyring subsystem is not to perform the
> data operations, but for managing keys for these operations. Kindly correct
> me if I am wrong here.

Have a look at

security/keys/keyctl_pkey.c

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Gupta, Nipun]

On 12-04-2025 06:53, Herbert Xu wrote:
> On Fri, Apr 11, 2025 at 11:50:54PM +0530, Gupta, Nipun wrote:
>>
>> AFAIU after looking into it, the keyring subsystem is not to perform the
>> data operations, but for managing keys for these operations. Kindly correct
>> me if I am wrong here.
> 
> Have a look at
> 
> security/keys/keyctl_pkey.c

Thanks for pointing out to the C file, but as these these system calls 
can support only synchronous operations, precludes their use for 
asynchronous operations. In the TLS handshakes, where multiple 
connections occur simultaneously, asynchronous operations are 
beneficial. OpenSSL ASYNC support can very well utilizes the 
asynchronous operations while establishing multiple TLS connections.

Thanks,
Nipun

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Herbert Xu]

On Thu, Apr 17, 2025 at 09:02:15PM +0530, Gupta, Nipun wrote:
>
> Thanks for pointing out to the C file, but as these these system calls can
> support only synchronous operations, precludes their use for asynchronous
> operations. In the TLS handshakes, where multiple connections occur
> simultaneously, asynchronous operations are beneficial. OpenSSL ASYNC
> support can very well utilizes the asynchronous operations while
> establishing multiple TLS connections.

In that case we should extend af_alg to support akcipher algorithms.

Having every crypto driver make up its own user-space PKI interface
is not scalable.

I held back on adding akcipher to af_alg because it would lead to
the freezing of our akcipher API.  But it's time to do this.

Being the first user of such an interface, could you please post
your OpenSSL patches as well so that we can look at what's actually
needed?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Lukas Wunner]

On Fri, Apr 11, 2025 at 10:21:03AM +0530, Gupta, Nipun wrote:
> On 10-04-2025 13:06, Krzysztof Kozlowski wrote:
> > On Wed, Apr 09, 2025 at 11:00:32PM GMT, Nipun Gupta wrote:
> > > The AMD PKI accelerator driver provides a accel interface to interact
> > > with the device for offloading and accelerating asymmetric crypto
> > > operations.
> > > 
> > 
> > For me this is clearly a crypto driver and you are supposed to:
> > 1. Cc crypto maintaners,
> > 2. Put it actually into crypto and use crypto API.
> 
> added crypto maintainers for comments.
> IMO, as accel framework is designed to support any type of compute
> accelerators, the PKI crypto accelerator in accel framework is not
> completely out of place here, as also suggested at:
> https://lore.kernel.org/all/2025031352-gyration-deceit-5563@gregkh/

To be fair, Greg did suggest drivers/crypto/ as an alternative... :)

  "Great, then why isn't this in drivers/accel/ or drivers/crypto/ ?"
  https://lore.kernel.org/r/2025031236-siamese-graffiti-5b98@gregkh/

There are already six drivers for crypto accelerators in drivers/crypto/,
so that would seem to be a natural fit for your driver:

  aspeed/aspeed-acry.c
  caam/caampkc.c
  ccp/ccp-crypto-rsa.c                 <-- from AMD no less!
  hisilicon/hpre/hpre_crypto.c
  intel/qat/qat_common/qat_asym_algs.c
  starfive/jh7110-rsa.c

You can find these in the tree with:

  git grep 'cra_name = "rsa"'

So far there are only drivers to accelerate RSA encryption/decryption.
The kernel supports a single padding scheme, PKCS1, which is implemented
by crypto/rsa-pkcs1pad.c.  There is no support yet for OAEP.

So the padding of the hash (which is cheap) happens in software and then
crypto/rsa-pkcs1pad.c performs an RSA encrypt/decrypt operation which is
either performed in software by crypto/rsa.c, or in hardware if a crypto
accelerator is present.  Drivers for crypto accelerators register the
"rsa" algorithm with a higher cra_priority than the software
implementation, hence are generally preferred.

One benefit that you get from implementing a proper akcipher_alg in your
driver is that virtual machines may take advantage of the hardware
accelerator through the virtio support implemented by:
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c

Note that the crypto subsystem currently does not support hardware
acceleration of signature generation/verification (crypto_sig),
but only encryption/decryption (crypto_akcipher).  One reason is
that signature generation/verification is generally a synchronous
operation and doesn't benefit as much from hardware acceleration
due to the overhead of interacting with the hardware.

So there's no support e.g. for generating or verifying ECDSA signatures
in hardware.  I think that would only really make sense if private keys
are kept in hardware and cannot be retrieved.  So the use case wouldn't
be acceleration, but security of private keys.

That said, for RSA specifically, signature generation/verification does
involve an encrypt/decrypt operation internally.  The padding is once
again done in software (by crypto/rsassa-pkcs1.c -- no PSS support yet).
But the actual encrypt/decrypt operation will be performed in
hardware if a crypto accelerator is present.

The user space interface Herbert referred to is a set of system calls
which are usable e.g. via the keyutils library and command line utility:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/

HTH,

Lukas

Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

[Gupta, Nipun]

On 14-04-2025 00:22, Lukas Wunner wrote:
> On Fri, Apr 11, 2025 at 10:21:03AM +0530, Gupta, Nipun wrote:
>> On 10-04-2025 13:06, Krzysztof Kozlowski wrote:
>>> On Wed, Apr 09, 2025 at 11:00:32PM GMT, Nipun Gupta wrote:
>>>> The AMD PKI accelerator driver provides a accel interface to interact
>>>> with the device for offloading and accelerating asymmetric crypto
>>>> operations.
>>>>
>>>
>>> For me this is clearly a crypto driver and you are supposed to:
>>> 1. Cc crypto maintaners,
>>> 2. Put it actually into crypto and use crypto API.
>>
>> added crypto maintainers for comments.
>> IMO, as accel framework is designed to support any type of compute
>> accelerators, the PKI crypto accelerator in accel framework is not
>> completely out of place here, as also suggested at:
>> https://lore.kernel.org/all/2025031352-gyration-deceit-5563@gregkh/
> 
> To be fair, Greg did suggest drivers/crypto/ as an alternative... :)
> 
>    "Great, then why isn't this in drivers/accel/ or drivers/crypto/ ?"
>    https://lore.kernel.org/r/2025031236-siamese-graffiti-5b98@gregkh/
> 
> There are already six drivers for crypto accelerators in drivers/crypto/,
> so that would seem to be a natural fit for your driver:
> 
>    aspeed/aspeed-acry.c
>    caam/caampkc.c
>    ccp/ccp-crypto-rsa.c                 <-- from AMD no less!
>    hisilicon/hpre/hpre_crypto.c
>    intel/qat/qat_common/qat_asym_algs.c
>    starfive/jh7110-rsa.c
> 
> You can find these in the tree with:
> 
>    git grep 'cra_name = "rsa"'
> 
> So far there are only drivers to accelerate RSA encryption/decryption.
> The kernel supports a single padding scheme, PKCS1, which is implemented
> by crypto/rsa-pkcs1pad.c.  There is no support yet for OAEP.
> 
> So the padding of the hash (which is cheap) happens in software and then
> crypto/rsa-pkcs1pad.c performs an RSA encrypt/decrypt operation which is
> either performed in software by crypto/rsa.c, or in hardware if a crypto
> accelerator is present.  Drivers for crypto accelerators register the
> "rsa" algorithm with a higher cra_priority than the software
> implementation, hence are generally preferred.
> 
> One benefit that you get from implementing a proper akcipher_alg in your
> driver is that virtual machines may take advantage of the hardware
> accelerator through the virtio support implemented by:
> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> 
> Note that the crypto subsystem currently does not support hardware
> acceleration of signature generation/verification (crypto_sig),
> but only encryption/decryption (crypto_akcipher).  One reason is
> that signature generation/verification is generally a synchronous
> operation and doesn't benefit as much from hardware acceleration
> due to the overhead of interacting with the hardware.

Thank you for the feedback.

When establishing TLS connections, OpenSSL requires signature 
generation/verification. In scenarios where multiple connections occur 
simultaneously, asynchronous operations are beneficial as they lead to 
much improved CPU utilization. OpenSSL ASYNC support can very well 
utilizes the asynchronous operations while establishing multiple TLS 
connections.

> 
> So there's no support e.g. for generating or verifying ECDSA signatures
> in hardware.  I think that would only really make sense if private keys
> are kept in hardware and cannot be retrieved.  So the use case wouldn't
> be acceleration, but security of private keys.

While ECDSA signature generation and verification enhance the security 
of private keys when stored in hardware, they also play important role 
in the establishment of TLS connections. Offloading these operations to 
hardware frees up CPU, enhancing performance during TLS handshakes.

> 
> That said, for RSA specifically, signature generation/verification does
> involve an encrypt/decrypt operation internally.  The padding is once
> again done in software (by crypto/rsassa-pkcs1.c -- no PSS support yet).
> But the actual encrypt/decrypt operation will be performed in
> hardware if a crypto accelerator is present.
> 
> The user space interface Herbert referred to is a set of system calls
> which are usable e.g. via the keyutils library and command line utility:
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/

These system calls can support only synchronous operations, which 
precludes their use for asynchronous operations. This limitation 
prevents the use of keyutils here.

Thanks,
Nipun