Re: [PATCH v1 03/20] s390/ap: Introduce ap message buffer pool

[Holger Dengler <dengler@linux.ibm.com>]

On 23/02/2025 10:54, Harald Freudenberger wrote:
> There is a need for a do-not-allocate-memory path through the
> ap bus layer. When ap_init_apmsg() with the AP_MSG_FLAG_MEMPOOL
> xflag is called, instead of kmalloc() the ap message buffer is
> allocated from the ap_msg_pool. This pool only holds a limited
> amount of buffers: AP_MSG_POOL_MIN_ITEMS with the item size
> AP_DEFAULT_MAX_MSG_SIZE and exactly one of these items (if available)
> is returned if ap_init_apmsg() with the MEMPOOL flag is called.
> When this pool is exhausted and the MEMPOOL flag is effective,
> ap_init_apmsg() returns -ENOMEM without any attempt to allocate
> memory.
> 
> The zcrypt layer may use this flag to indicate to the ap bus
> that the processing path for this message should not allocate
> memory. This is to prevent deadlocks with crypto and io for
> example with encrypted swap volumes.

See my comments below.
The rest looks good to me.

> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> ---
>  drivers/s390/crypto/ap_bus.c     | 59 +++++++++++++++++++++++++++-----
>  drivers/s390/crypto/ap_bus.h     |  3 +-
>  drivers/s390/crypto/zcrypt_api.c | 10 +++---
>  3 files changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 4940eaf538e9..b585b5d11074 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
[...]
> @@ -546,16 +562,27 @@ static void ap_poll_thread_stop(void)
>  #define is_card_dev(x) ((x)->parent == ap_root_device)
>  #define is_queue_dev(x) ((x)->parent != ap_root_device)
>  
> -/**
> +/*

What is the reason for mixing coding styles?

>   * ap_init_apmsg() - Initialize ap_message.
> - * Initialize a message before using. Otherwise this might result in
> - * unexpected behaviour.
> + * Initialize struct ap_message and allocate buffer to construct
> + * the ap message.
>   */
> -int ap_init_apmsg(struct ap_message *ap_msg)
> +int ap_init_apmsg(struct ap_message *ap_msg, u32 xflags)

The xflags function parameter is very confusing (here and also in all other APIs too), because it allows to set some, but not all flags in ap_msg-flags. Why not using `bool alloc`? If you will keep the more flexible interface, please add another xflags element to struct ap_message. There is nothing in common between the ap_msg->flags and xflags, beside they're both named "flags". 

>  {
> -	unsigned int maxmsgsize = atomic_read(&ap_max_msg_size);
> +	unsigned int maxmsgsize;
>  
>  	memset(ap_msg, 0, sizeof(*ap_msg));
> +
> +	if (xflags & AP_MSG_FLAG_MEMPOOL) {
> +		ap_msg->msg = mempool_alloc_preallocated(ap_msg_pool);
> +		if (!ap_msg->msg)
> +			return -ENOMEM;
> +		ap_msg->bufsize = AP_DEFAULT_MAX_MSG_SIZE;
> +		ap_msg->flags |= AP_MSG_FLAG_MEMPOOL;
> +		return 0;
> +	}
> +
> +	maxmsgsize = atomic_read(&ap_max_msg_size);
>  	ap_msg->msg = kmalloc(maxmsgsize, GFP_KERNEL);
>  	if (!ap_msg->msg)
>  		return -ENOMEM;
> @@ -565,14 +592,18 @@ int ap_init_apmsg(struct ap_message *ap_msg)
>  }
>  EXPORT_SYMBOL(ap_init_apmsg);
>  
> -/**
> +/*

???

>   * ap_release_apmsg() - Release ap_message.
> - * Releases all memory used internal within the ap_message struct
> - * Currently this is the message and private field.
> + * Cleanup struct ap_message and release all memory held.
>   */
>  void ap_release_apmsg(struct ap_message *ap_msg)
>  {
> -	kfree_sensitive(ap_msg->msg);
> +	if (ap_msg->flags & AP_MSG_FLAG_MEMPOOL) {
> +		memzero_explicit(ap_msg->msg, ap_msg->bufsize);
> +		mempool_free(ap_msg->msg, ap_msg_pool);
> +	} else {
> +		kfree_sensitive(ap_msg->msg);
> +	}
>  }
>  EXPORT_SYMBOL(ap_release_apmsg);
>  
[...]

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