Re: [PATCH 2/2] s390/vfio-ap: add status attribute to AP queue device's sysfs dir

[Harald Freudenberger <freude@linux.ibm.com>]

On 27.10.21 18:48, Tony Krowiak wrote:
> This patch adds a sysfs 'status' attribute to a queue device when it is
> bound to the vfio_ap device driver. The field displays a string indicating
> the status of the queue device:
>
> Status String:  Indicates:
> -------------   ---------
> "assigned"      the queue is assigned to an mdev, but is not in use by a
>                 KVM guest.
> "in use"        the queue is assigned to an mdev and is in use by a KVM
>                 guest.
> "unassigned"    the queue is not assigned to an mdev.
>
> The status string will be displayed by the 'lszcrypt' command if the queue
> device is bound to the vfio_ap device driver.
>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> [akrowiak@linux.ibm.com: added check for queue in use by guest]
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c | 79 +++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 03311a476366..e043ae236630 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -17,6 +17,9 @@
>  
>  #define VFIO_AP_ROOT_NAME "vfio_ap"
>  #define VFIO_AP_DEV_NAME "matrix"
> +#define AP_QUEUE_ASSIGNED "assigned"
> +#define AP_QUEUE_UNASSIGNED "unassigned"
> +#define AP_QUEUE_IN_USE "in use"
>  
>  MODULE_AUTHOR("IBM Corporation");
>  MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
> @@ -41,26 +44,95 @@ static struct ap_device_id ap_queue_ids[] = {
>  
>  MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>  
> +static struct ap_matrix_mdev *vfio_ap_mdev_for_queue(struct vfio_ap_queue *q)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +	unsigned long apid = AP_QID_CARD(q->apqn);
> +	unsigned long apqi = AP_QID_QUEUE(q->apqn);
> +
> +	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> +		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
> +		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
> +			return matrix_mdev;
> +	}
> +
> +	return NULL;
> +}
> +
> +static ssize_t status_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	ssize_t nchars = 0;
> +	struct vfio_ap_queue *q;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	struct ap_device *apdev = to_ap_dev(dev);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	q = dev_get_drvdata(&apdev->device);
> +	matrix_mdev = vfio_ap_mdev_for_queue(q);
> +
> +	if (matrix_mdev) {
> +		if (matrix_mdev->kvm)
> +			nchars = scnprintf(buf, PAGE_SIZE, "%s\n",
> +					   AP_QUEUE_IN_USE);
> +		else
> +			nchars = scnprintf(buf, PAGE_SIZE, "%s\n",
> +					   AP_QUEUE_ASSIGNED);
> +	} else {
> +		nchars = scnprintf(buf, PAGE_SIZE, "%s\n",
> +				   AP_QUEUE_UNASSIGNED);
> +	}
> +
> +	mutex_unlock(&matrix_dev->lock);
> +
> +	return nchars;
> +}
> +
> +static DEVICE_ATTR_RO(status);
> +
> +static struct attribute *vfio_queue_attrs[] = {
> +	&dev_attr_status.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group vfio_queue_attr_group = {
> +	.attrs = vfio_queue_attrs,
> +};
> +
>  /**
>   * vfio_ap_queue_dev_probe: Allocate a vfio_ap_queue structure and associate it
>   *			    with the device as driver_data.
>   *
>   * @apdev: the AP device being probed
>   *
> - * Return: returns 0 if the probe succeeded; otherwise, returns -ENOMEM if
> - *	   storage could not be allocated for a vfio_ap_queue object.
> + * Return: returns 0 if the probe succeeded; otherwise, returns an error if
> + *	   storage could not be allocated for a vfio_ap_queue object or the
> + *	   sysfs 'status' attribute could not be created for the queue device.
>   */
>  static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>  {
> +	int ret;
>  	struct vfio_ap_queue *q;
>  
>  	q = kzalloc(sizeof(*q), GFP_KERNEL);
>  	if (!q)
>  		return -ENOMEM;
> +
> +	mutex_lock(&matrix_dev->lock);
>  	dev_set_drvdata(&apdev->device, q);
>  	q->apqn = to_ap_queue(&apdev->device)->qid;
>  	q->saved_isc = VFIO_AP_ISC_INVALID;
> -	return 0;
> +
> +	ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
> +	if (ret) {
> +		dev_set_drvdata(&apdev->device, NULL);
> +		kfree(q);
> +	}
> +
> +	mutex_unlock(&matrix_dev->lock);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -75,6 +147,7 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>  	struct vfio_ap_queue *q;
>  
>  	mutex_lock(&matrix_dev->lock);
> +	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
>  	q = dev_get_drvdata(&apdev->device);
>  	vfio_ap_mdev_reset_queue(q, 1);
>  	dev_set_drvdata(&apdev->device, NULL);
Looks good to me, add my "Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>"