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

["Gupta, Nipun" <nipun.gupta@amd.com>]

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