Re: [PATCH v2 07/20] s390/zcrypt: Rework zcrypt function zcrypt_device_status_mask_ext

Holger Dengler
2025-03-19T11:03:20Z

On 04/03/2025 18:21, Harald Freudenberger wrote:
> Rework the existing function zcrypt_device_status_mask_ext():
> * Add two new parameters to provide upper limits for
> cards and queues. The existing implementation needed an
> array of 256 * 256 * 4 = 256 KB which is really huge. The
> reworked function is more flexible in the sense that the
> caller can decide the upper limit for cards and domains to
> be stored into the status array. So for example a caller may
> decide to only query for cards 0...127 and queues 0...127
> and thus only an array of size 128 * 128 * 4 = 64 KB is needed.
> * Instead of void the reworked function now returns an int.
> The currently only way to have the function return != 0
> is by providing card or domains limits beyond 256.

I would prefer to stay with a void function and limit the card and domain values to the current maximum. Details below.

>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> ---
> drivers/s390/crypto/zcrypt_api.c | 20 +++++++++++++++-----
> drivers/s390/crypto/zcrypt_api.h | 3 ++-
> drivers/s390/crypto/zcrypt_ccamisc.c | 22 +++++++++++++++++-----
> drivers/s390/crypto/zcrypt_ep11misc.c | 14 ++++++++++----
> 4 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index 62cc05881b13..bd2738e3792a 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -1317,19 +1317,25 @@ static void zcrypt_device_status_mask(struct zcrypt_device_status *devstatus)
> spin_unlock(&zcrypt_list_lock);
> }
>
> -void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus)
> +int zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus,
> + int maxcard, int maxqueue)

Keep void and ...

> {
> struct zcrypt_card *zc;
> struct zcrypt_queue *zq;
> struct zcrypt_device_status_ext *stat;
> int card, queue;
>
> + if (maxcard > MAX_ZDEV_CARDIDS_EXT || maxqueue > MAX_ZDEV_DOMAINS_EXT)
> + return -EINVAL;
> +

... limit maxcard/maxqueue to the maximum supported values. In my opinion, it does not make any sense to call this function with higher values than the maximum.

maxcard = MIN(maxcard, MAX_ZDEV_CARDIDS_EXT);
maxqueue = MIN(maxqueue, MAX_ZDEV_DOMAINS_EXT);

As a side effect, it keeps the caller code much simpler.

> spin_lock(&zcrypt_list_lock);
> for_each_zcrypt_card(zc) {
> for_each_zcrypt_queue(zq, zc) {
> card = AP_QID_CARD(zq->queue->qid);
> queue = AP_QID_QUEUE(zq->queue->qid);
> - stat = &devstatus[card * AP_DOMAINS + queue];
> + if (card >= maxcard || queue >= maxqueue)
> + continue;
> + stat = &devstatus[card * maxqueue + queue];
> stat->hwtype = zc->card->ap_dev.device_type;
> stat->functions = zc->card->hwinfo.fac >> 26;
> stat->qid = zq->queue->qid;
[...]
> @@ -1635,9 +1643,11 @@ static long zcrypt_unlocked_ioctl(struct file *filp, unsigned int cmd,
> GFP_KERNEL);
> if (!device_status)
> return -ENOMEM;
> - zcrypt_device_status_mask_ext(device_status);
> - if (copy_to_user((char __user *)arg, device_status,
> - total_size))
> + rc = zcrypt_device_status_mask_ext(device_status,
> + MAX_ZDEV_CARDIDS_EXT,
> + MAX_ZDEV_DOMAINS_EXT);
> + if (!rc && copy_to_user((char __user *)arg, device_status,
> + total_size))

With the change above, you can stay with the current error handling. Only the addition parameters for zcrypt_device_status_mask_ext() need to be added.

> rc = -EFAULT;
> kvfree(device_status);
> return rc;
[...]

--
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com