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

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

[Tony Krowiak]

Patch 0001-s390-vfio-ap-s390-crypto-fix-all-kernel-doc-warnings.patch does
not need to be reviewed. It is included here because it is not yet merged
into the kernel, but precedes the second patch.

Tony Krowiak (2):
  s390/vfio-ap: s390/crypto: fix all kernel-doc warnings
  s390/vfio-ap: add status attribute to AP queue device's sysfs dir

 drivers/s390/crypto/vfio_ap_drv.c     | 91 ++++++++++++++++++++++++---
 drivers/s390/crypto/vfio_ap_ops.c     |  5 +-
 drivers/s390/crypto/vfio_ap_private.h | 43 +++++++++----
 3 files changed, 118 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/2] s390/vfio-ap: s390/crypto: fix all kernel-doc warnings

[Tony Krowiak]

Fixes the kernel-doc warnings in the following source files:

* drivers/s390/crypto/vfio_ap_private.h
* drivers/s390/crypto/vfio_ap_drv.c
* drivers/s390/crypto/vfio_ap_ops.c

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 16 ++++++----
 drivers/s390/crypto/vfio_ap_ops.c     |  5 ++--
 drivers/s390/crypto/vfio_ap_private.h | 43 +++++++++++++++++++--------
 3 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 4d2556bc7fe5..03311a476366 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -42,10 +42,13 @@ static struct ap_device_id ap_queue_ids[] = {
 MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
 /**
- * vfio_ap_queue_dev_probe:
+ * vfio_ap_queue_dev_probe: Allocate a vfio_ap_queue structure and associate it
+ *			    with the device as driver_data.
  *
- * 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.
  */
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
@@ -61,10 +64,11 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 }
 
 /**
- * vfio_ap_queue_dev_remove:
+ * vfio_ap_queue_dev_remove: Free the associated vfio_ap_queue structure.
+ *
+ * @apdev: the AP device being removed
  *
- * Takes the matrix lock to avoid actions on this device while removing
- * Free the associated vfio_ap_queue structure
+ * Takes the matrix lock to avoid actions on this device while doing the remove.
  */
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 623d5269a52c..94c1c9bd58ad 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -187,6 +187,8 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
  * vfio_ap_irq_enable - Enable Interruption for a APQN
  *
  * @q:	 the vfio_ap_queue holding AQIC parameters
+ * @isc: the guest ISC to register with the GIB interface
+ * @nib: the notification indicator byte to pin.
  *
  * Pin the NIB saved in *q
  * Register the guest ISC to GIB interface and retrieve the
@@ -738,7 +740,6 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
  * assign_domain_store - parses the APQI from @buf and sets the
  * corresponding bit in the mediated matrix device's AQM
  *
- *
  * @dev:	the matrix device
  * @attr:	the mediated matrix device's assign_domain attribute
  * @buf:	a buffer containing the AP queue index (APQI) of the domain to
@@ -866,7 +867,6 @@ static DEVICE_ATTR_WO(unassign_domain);
  * assign_control_domain_store - parses the domain ID from @buf and sets
  * the corresponding bit in the mediated matrix device's ADM
  *
- *
  * @dev:	the matrix device
  * @attr:	the mediated matrix device's assign_control_domain attribute
  * @buf:	a buffer containing the domain ID to be assigned
@@ -1142,6 +1142,7 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
  * by @matrix_mdev.
  *
  * @matrix_mdev: a matrix mediated device
+ * @kvm: the pointer to the kvm structure being unset.
  *
  * Note: The matrix_dev->lock must be taken prior to calling
  * this function; however, the lock will be temporarily released while the
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 77760e2b546f..648fcaf8104a 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -26,16 +26,18 @@
 #define VFIO_AP_DRV_NAME "vfio_ap"
 
 /**
- * ap_matrix_dev - the AP matrix device structure
+ * struct ap_matrix_dev - Contains the data for the matrix device.
+ *
  * @device:	generic device structure associated with the AP matrix device
  * @available_instances: number of mediated matrix devices that can be created
  * @info:	the struct containing the output from the PQAP(QCI) instruction
- * mdev_list:	the list of mediated matrix devices created
- * lock:	mutex for locking the AP matrix device. This lock will be
+ * @mdev_list:	the list of mediated matrix devices created
+ * @lock:	mutex for locking the AP matrix device. This lock will be
  *		taken every time we fiddle with state managed by the vfio_ap
  *		driver, be it using @mdev_list or writing the state of a
  *		single ap_matrix_mdev device. It's quite coarse but we don't
  *		expect much contention.
+ * @vfio_ap_drv: the vfio_ap device driver
  */
 struct ap_matrix_dev {
 	struct device device;
@@ -49,17 +51,19 @@ struct ap_matrix_dev {
 extern struct ap_matrix_dev *matrix_dev;
 
 /**
- * The AP matrix is comprised of three bit masks identifying the adapters,
- * queues (domains) and control domains that belong to an AP matrix. The bits i
- * each mask, from least significant to most significant bit, correspond to IDs
- * 0 to 255. When a bit is set, the corresponding ID belongs to the matrix.
+ * struct ap_matrix - matrix of adapters, domains and control domains
  *
  * @apm_max: max adapter number in @apm
- * @apm identifies the AP adapters in the matrix
+ * @apm: identifies the AP adapters in the matrix
  * @aqm_max: max domain number in @aqm
- * @aqm identifies the AP queues (domains) in the matrix
+ * @aqm: identifies the AP queues (domains) in the matrix
  * @adm_max: max domain number in @adm
- * @adm identifies the AP control domains in the matrix
+ * @adm: identifies the AP control domains in the matrix
+ *
+ * The AP matrix is comprised of three bit masks identifying the adapters,
+ * queues (domains) and control domains that belong to an AP matrix. The bits in
+ * each mask, from left to right, correspond to IDs 0 to 255. When a bit is set
+ * the corresponding ID belongs to the matrix.
  */
 struct ap_matrix {
 	unsigned long apm_max;
@@ -71,13 +75,20 @@ struct ap_matrix {
 };
 
 /**
- * struct ap_matrix_mdev - the mediated matrix device structure
- * @list:	allows the ap_matrix_mdev struct to be added to a list
+ * struct ap_matrix_mdev - Contains the data associated with a matrix mediated
+ *			   device.
+ * @vdev:	the vfio device
+ * @node:	allows the ap_matrix_mdev struct to be added to a list
  * @matrix:	the adapters, usage domains and control domains assigned to the
  *		mediated matrix device.
  * @group_notifier: notifier block used for specifying callback function for
  *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
+ * @iommu_notifier: notifier block used for specifying callback function for
+ *		    handling the VFIO_IOMMU_NOTIFY_DMA_UNMAP even
  * @kvm:	the struct holding guest's state
+ * @pqap_hook:	the function pointer to the interception handler for the
+ *		PQAP(AQIC) instruction.
+ * @mdev:	the mediated device
  */
 struct ap_matrix_mdev {
 	struct vfio_device vdev;
@@ -90,6 +101,14 @@ struct ap_matrix_mdev {
 	struct mdev_device *mdev;
 };
 
+/**
+ * struct vfio_ap_queue - contains the data associated with a queue bound to the
+ *			  vfio_ap device driver
+ * @matrix_mdev: the matrix mediated device
+ * @saved_pfn: the guest PFN pinned for the guest
+ * @apqn: the APQN of the AP queue device
+ * @saved_isc: the guest ISC registered with the GIB interface
+ */
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;
 	unsigned long saved_pfn;
-- 
2.31.1

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

[Tony Krowiak]

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);
-- 
2.31.1

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

[Harald Freudenberger]

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>"

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

[Tony Krowiak]

On 10/28/21 3:40 AM, Harald Freudenberger wrote:
> 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>Looks good to me, add my "Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>"

You already have a Signed-off-by since you created the original version 
of this
patch. I added my Signed-off-by for adding the "in use" status value. 
Should I still
add a Reviewed-by for you?