| Message ID | 20200407192015.19887-5-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
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; > +}
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; >> +}
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; >>> +} >
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; > >> +} >
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; >>>> +}
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_ */
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(-)