[v7,04/15] s390/vfio-ap: implement in-use callback for vfio_ap driver
diff mbox series

Message ID 20200407192015.19887-5-akrowiak@linux.ibm.com
State New
Headers show
Series
  • s390/vfio-ap: dynamic configuration support
Related show

Commit Message

Tony Krowiak April 7, 2020, 7:20 p.m. UTC
Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is
invoked whenever a change to the apmask or aqmask would
result in one or more queue devices being removed from the driver. The
vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to
any of the matrix mdevs under the driver's control.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     |  1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
 drivers/s390/crypto/vfio_ap_private.h |  2 ++
 3 files changed, 33 insertions(+), 17 deletions(-)

Comments

Cornelia Huck April 16, 2020, 11:18 a.m. UTC | #1
On Tue,  7 Apr 2020 15:20:04 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> Let's implement the callback to indicate when an APQN
> is in use by the vfio_ap device driver. The callback is
> invoked whenever a change to the apmask or aqmask would
> result in one or more queue devices being removed from the driver. The
> vfio_ap device driver will indicate a resource is in use
> if the APQN of any of the queue devices to be removed are assigned to
> any of the matrix mdevs under the driver's control.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     |  1 +
>  drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
>  drivers/s390/crypto/vfio_ap_private.h |  2 ++
>  3 files changed, 33 insertions(+), 17 deletions(-)

> @@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
>  	kfree(q);
>  	mutex_unlock(&matrix_dev->lock);
>  }
> +
> +bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
> +{
> +	bool in_use;
> +
> +	mutex_lock(&matrix_dev->lock);
> +	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;

Maybe

in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);

?

> +	mutex_unlock(&matrix_dev->lock);
> +
> +	return in_use;
> +}
Tony Krowiak April 16, 2020, 2:45 p.m. UTC | #2
On 4/16/20 7:18 AM, Cornelia Huck wrote:
> On Tue,  7 Apr 2020 15:20:04 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> Let's implement the callback to indicate when an APQN
>> is in use by the vfio_ap device driver. The callback is
>> invoked whenever a change to the apmask or aqmask would
>> result in one or more queue devices being removed from the driver. The
>> vfio_ap device driver will indicate a resource is in use
>> if the APQN of any of the queue devices to be removed are assigned to
>> any of the matrix mdevs under the driver's control.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     |  1 +
>>   drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
>>   drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>   3 files changed, 33 insertions(+), 17 deletions(-)
>> @@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
>>   	kfree(q);
>>   	mutex_unlock(&matrix_dev->lock);
>>   }
>> +
>> +bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
>> +{
>> +	bool in_use;
>> +
>> +	mutex_lock(&matrix_dev->lock);
>> +	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;
> Maybe
>
> in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);
>
> ?

To be honest, I find the !! expression very confusing. Every time I see 
it, I have
to spend time thinking about what the result of !! is going to be. I think
the statement should be left as-is because it more clearly expresses
the intent.

>
>> +	mutex_unlock(&matrix_dev->lock);
>> +
>> +	return in_use;
>> +}
Pierre Morel April 17, 2020, 11:23 a.m. UTC | #3
On 2020-04-16 16:45, Tony Krowiak wrote:
> 
> 
> On 4/16/20 7:18 AM, Cornelia Huck wrote:
>> On Tue,  7 Apr 2020 15:20:04 -0400
>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>
>>> Let's implement the callback to indicate when an APQN
>>> is in use by the vfio_ap device driver. The callback is
>>> invoked whenever a change to the apmask or aqmask would
>>> result in one or more queue devices being removed from the driver. The
>>> vfio_ap device driver will indicate a resource is in use
>>> if the APQN of any of the queue devices to be removed are assigned to
>>> any of the matrix mdevs under the driver's control.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_drv.c     |  1 +
>>>   drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
>>>   drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>>   3 files changed, 33 insertions(+), 17 deletions(-)
>>> @@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue 
>>> *queue)
>>>       kfree(q);
>>>       mutex_unlock(&matrix_dev->lock);
>>>   }
>>> +
>>> +bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long 
>>> *aqm)
>>> +{
>>> +    bool in_use;
>>> +
>>> +    mutex_lock(&matrix_dev->lock);
>>> +    in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : 
>>> false;
>> Maybe
>>
>> in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);
>>
>> ?
> 
> To be honest, I find the !! expression very confusing. Every time I see 
> it, I have
> to spend time thinking about what the result of !! is going to be. I think
> the statement should be left as-is because it more clearly expresses
> the intent.



In other places you use
"
         ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
         if (ret)
                 goto share_err;
"
then why use a boolean here?

If you want to return a boolean and you do not want to use !! you can do:

  ...
   ret = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);
...
   return (ret) ? false : true;

> 
>>
>>> +    mutex_unlock(&matrix_dev->lock);
>>> +
>>> +    return in_use;
>>> +}
>
Halil Pasic April 24, 2020, 3:13 a.m. UTC | #4
On Thu, 16 Apr 2020 10:45:20 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> 
> 
> On 4/16/20 7:18 AM, Cornelia Huck wrote:
> > On Tue,  7 Apr 2020 15:20:04 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >
> >> Let's implement the callback to indicate when an APQN
> >> is in use by the vfio_ap device driver. The callback is
> >> invoked whenever a change to the apmask or aqmask would
> >> result in one or more queue devices being removed from the driver. The
> >> vfio_ap device driver will indicate a resource is in use
> >> if the APQN of any of the queue devices to be removed are assigned to
> >> any of the matrix mdevs under the driver's control.
> >>
> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> >> ---
> >>   drivers/s390/crypto/vfio_ap_drv.c     |  1 +
> >>   drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
> >>   drivers/s390/crypto/vfio_ap_private.h |  2 ++
> >>   3 files changed, 33 insertions(+), 17 deletions(-)
> >> @@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
> >>   	kfree(q);
> >>   	mutex_unlock(&matrix_dev->lock);
> >>   }
> >> +
> >> +bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
> >> +{
> >> +	bool in_use;
> >> +
> >> +	mutex_lock(&matrix_dev->lock);
> >> +	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;
> > Maybe
> >
> > in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);
> >
> > ?
> 
> To be honest, I find the !! expression very confusing. Every time I see 
> it, I have
> to spend time thinking about what the result of !! is going to be. I think
> the statement should be left as-is because it more clearly expresses
> the intent.
> 

This is discussion is just about cosmetics, I believe. Just a piece of
advice: try to be sensitive about the community. In this community, and
I believe in C general !! is the idiomatic way to convert number to
boolean. Why would one want to do that is a bit longer story. The short
version is in logic condition context the value 0 is false and any
other value is true. !! keeps false value (0) false, and forces a true to
the most true true value. If you keep getting confused every time you
run across a !! that won't help with reading other peoples C.

Regards,
Halil 

> >
> >> +	mutex_unlock(&matrix_dev->lock);
> >> +
> >> +	return in_use;
> >> +}
>
Tony Krowiak April 24, 2020, 4:58 p.m. UTC | #5
On 4/23/20 11:13 PM, Halil Pasic wrote:
> On Thu, 16 Apr 2020 10:45:20 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>>
>> On 4/16/20 7:18 AM, Cornelia Huck wrote:
>>> On Tue,  7 Apr 2020 15:20:04 -0400
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>
>>>> Let's implement the callback to indicate when an APQN
>>>> is in use by the vfio_ap device driver. The callback is
>>>> invoked whenever a change to the apmask or aqmask would
>>>> result in one or more queue devices being removed from the driver. The
>>>> vfio_ap device driver will indicate a resource is in use
>>>> if the APQN of any of the queue devices to be removed are assigned to
>>>> any of the matrix mdevs under the driver's control.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>>>    drivers/s390/crypto/vfio_ap_drv.c     |  1 +
>>>>    drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
>>>>    drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>>>    3 files changed, 33 insertions(+), 17 deletions(-)
>>>> @@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
>>>>    	kfree(q);
>>>>    	mutex_unlock(&matrix_dev->lock);
>>>>    }
>>>> +
>>>> +bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
>>>> +{
>>>> +	bool in_use;
>>>> +
>>>> +	mutex_lock(&matrix_dev->lock);
>>>> +	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;
>>> Maybe
>>>
>>> in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);
>>>
>>> ?
>> To be honest, I find the !! expression very confusing. Every time I see
>> it, I have
>> to spend time thinking about what the result of !! is going to be. I think
>> the statement should be left as-is because it more clearly expresses
>> the intent.
>>
> This is discussion is just about cosmetics, I believe. Just a piece of
> advice: try to be sensitive about the community. In this community, and
> I believe in C general !! is the idiomatic way to convert number to
> boolean. Why would one want to do that is a bit longer story. The short
> version is in logic condition context the value 0 is false and any
> other value is true. !! keeps false value (0) false, and forces a true to
> the most true true value. If you keep getting confused every time you
> run across a !! that won't help with reading other peoples C.
>
> Regards,
> Halil

The point is moot. After seeing that Conny's comment generated a
discussion, I decided to avoid wasting additional time discussing
personal preferences and am now using the !! syntax. Unfortunately,
I've been having some odd problems with my email client and my
response to Pierre's comment never made it to the list, so I apologize
that you had to waste valuable time on your tutorial.

>
>>>> +	mutex_unlock(&matrix_dev->lock);
>>>> +
>>>> +	return in_use;
>>>> +}

Patch
diff mbox series

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index e9c226c0730e..5197a1fe14d4 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -174,6 +174,7 @@  static int __init vfio_ap_init(void)
 	memset(&vfio_ap_drv, 0, sizeof(vfio_ap_drv));
 	vfio_ap_drv.probe = vfio_ap_queue_dev_probe;
 	vfio_ap_drv.remove = vfio_ap_queue_dev_remove;
+	vfio_ap_drv.in_use = vfio_ap_mdev_resource_in_use;
 	vfio_ap_drv.ids = ap_queue_ids;
 
 	ret = ap_driver_register(&vfio_ap_drv, THIS_MODULE, VFIO_AP_DRV_NAME);
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 00699bd72d2b..8ece0d52ff4c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -500,7 +500,9 @@  vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
  *
  * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
  */
-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
 {
 	struct ap_matrix_mdev *lstdev;
 	DECLARE_BITMAP(apm, AP_DEVICES);
@@ -517,12 +519,10 @@  static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 		 * We work on full longs, as we can only exclude the leftover
 		 * bits in non-inverse order. The leftover is all zeros.
 		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
 			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
 			continue;
 
 		return -EADDRINUSE;
@@ -594,6 +594,7 @@  static ssize_t assign_adapter_store(struct device *dev,
 {
 	int ret;
 	unsigned long apid;
+	DECLARE_BITMAP(apm, AP_DEVICES);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
@@ -619,18 +620,18 @@  static ssize_t assign_adapter_store(struct device *dev,
 	if (ret)
 		goto done;
 
-	set_bit_inv(apid, matrix_mdev->matrix.apm);
+	memset(apm, 0, sizeof(apm));
+	set_bit_inv(apid, apm);
 
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
+	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm,
+					     matrix_mdev->matrix.aqm);
 	if (ret)
-		goto share_err;
+		goto done;
 
+	set_bit_inv(apid, matrix_mdev->matrix.apm);
 	vfio_ap_mdev_qlinks_for_apid(matrix_mdev, apid);
 	ret = count;
-	goto done;
 
-share_err:
-	clear_bit_inv(apid, matrix_mdev->matrix.apm);
 done:
 	mutex_unlock(&matrix_dev->lock);
 
@@ -767,6 +768,7 @@  static ssize_t assign_domain_store(struct device *dev,
 {
 	int ret;
 	unsigned long apqi;
+	DECLARE_BITMAP(aqm, AP_DOMAINS);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
@@ -787,18 +789,18 @@  static ssize_t assign_domain_store(struct device *dev,
 	if (ret)
 		goto done;
 
-	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
+	memset(aqm, 0, sizeof(aqm));
+	set_bit_inv(apqi, aqm);
 
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
+	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev,
+					     matrix_mdev->matrix.apm, aqm);
 	if (ret)
-		goto share_err;
+		goto done;
 
+	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
 	vfio_ap_mdev_qlinks_for_apqi(matrix_mdev, apqi);
 	ret = count;
-	goto done;
 
-share_err:
-	clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
 done:
 	mutex_unlock(&matrix_dev->lock);
 
@@ -1369,3 +1371,14 @@  void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
 	kfree(q);
 	mutex_unlock(&matrix_dev->lock);
 }
+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use;
+
+	mutex_lock(&matrix_dev->lock);
+	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;
+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index e1f8c82cc55d..4b6e144bab17 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -105,4 +105,6 @@  struct vfio_ap_queue {
 int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
 void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
 
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */