Messages in this thread | | | Date | Fri, 18 Mar 2022 13:30:44 -0400 | Subject | Re: [PATCH v18 10/18] s390/vfio-ap: allow hot plug/unplug of AP devices when assigned/unassigned | From | Tony Krowiak <> |
| |
On 3/14/22 09:17, Jason J. Herne wrote: > On 3/11/22 11:07, Tony Krowiak wrote: >> >> >> On 3/11/22 09:26, Jason J. Herne wrote: >>> On 2/14/22 19:50, Tony Krowiak wrote: >>>> Let's allow adapters, domains and control domains to be hot plugged >>>> into and hot unplugged from a KVM guest using a matrix mdev when an >>>> adapter, domain or control domain is assigned to or unassigned from >>>> the matrix mdev. >>>> >>>> Whenever an assignment or unassignment of an adapter, domain or >>>> control >>>> domain is performed, the AP configuration assigned to the matrix >>>> mediated device will be filtered and assigned to the AP control block >>>> (APCB) that supplies the AP configuration to the guest so that no >>>> adapter, domain or control domain that is not in the host's AP >>>> configuration nor any APQN that does not reference a queue device >>>> bound >>>> to the vfio_ap device driver is assigned. >>>> >>>> After updating the APCB, if the mdev is in use by a KVM guest, it is >>>> hot plugged into the guest to dynamically provide access to the >>>> adapters, >>>> domains and control domains provided via the newly refreshed APCB. >>>> >>>> Keep in mind that the matrix_dev->guests_lock must be taken outside >>>> of the >>>> matrix_mdev->kvm->lock which in turn must be taken outside of the >>>> matrix_dev->mdevs_lock in order to avoid circular lock dependencies >>>> (i.e., >>>> a lockdep splat).Consequently, the locking order for hot plugging the >>>> guest's APCB must be: >>>> >>>> matrix_dev->guests_lock => matrix_mdev->kvm->lock => >>>> matrix_dev->mdevs_lock >>>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>> --- >>>> drivers/s390/crypto/vfio_ap_ops.c | 198 >>>> +++++++++++++++++++----------- >>>> 1 file changed, 125 insertions(+), 73 deletions(-) >>>> >>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >>>> b/drivers/s390/crypto/vfio_ap_ops.c >>>> index 623a4b38676d..4c382cd3afc7 100644 >>>> --- a/drivers/s390/crypto/vfio_ap_ops.c >>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>>> @@ -317,10 +317,25 @@ static void vfio_ap_matrix_init(struct >>>> ap_config_info *info, >>>> matrix->adm_max = info->apxa ? info->Nd : 15; >>>> } >>>> -static void vfio_ap_mdev_filter_cdoms(struct ap_matrix_mdev >>>> *matrix_mdev) >>>> +static void vfio_ap_mdev_hotplug_apcb(struct ap_matrix_mdev >>>> *matrix_mdev) >>>> { >>>> + if (matrix_mdev->kvm) >>>> + kvm_arch_crypto_set_masks(matrix_mdev->kvm, >>>> + matrix_mdev->shadow_apcb.apm, >>>> + matrix_mdev->shadow_apcb.aqm, >>>> + matrix_mdev->shadow_apcb.adm); >>>> +} >>> >>> This function updates a kvm guest's apcb. So let's rename it to >>> vfio_ap_update_apcb(). >> >> The idea was to indicate that the AP adapters, domains and control >> domains configured in the shadow APCB are being hot plugged into >> a running guest. Having said that, I can see your point. I'm not >> married to >> the function name, but I would prefer to go with >> 'vfio_ap_update_guest_apcb()' to distinguish between the shadow and >> the real apcb. >> >>> You can also call this function in vfio_ap_mdev_set_kvm, >>> instead of duplicating the code to call kvm_arch_crypto_set_masks(). >> >> The reason I didn't do that is because we've already verified the >> matrix_mdev->kvm in kvm_arch_crypto_set_masks(). I'm not sure what >> it buys us, but I'm not adverse to making the change. >> > > It avoids code duplication which makes the driver smaller, and slightly > easier to read. It also reduces rework effort if/when mask handling ever > changes.
Good points.
> >>> >>> >>> >>> >>>> +static bool vfio_ap_mdev_filter_cdoms(struct ap_matrix_mdev >>>> *matrix_mdev) >>>> +{ >>>> + DECLARE_BITMAP(shadow_adm, AP_DOMAINS); >>>> + >>>> + bitmap_copy(shadow_adm, matrix_mdev->shadow_apcb.adm, >>>> AP_DOMAINS); >>>> bitmap_and(matrix_mdev->shadow_apcb.adm, >>>> matrix_mdev->matrix.adm, >>>> (unsigned long *)matrix_dev->info.adm, AP_DOMAINS); >>>> + >>>> + return !bitmap_equal(shadow_adm, matrix_mdev->shadow_apcb.adm, >>>> + AP_DOMAINS); >>>> } >>> >>> your variable, shadow_adm, should be named original_adm. Since it >>> represents >>> the original value before filtering. This makes the intent much more >>> clear. >>> Same goes for the vars in vfio_ap_mdev_filter_matrix(). >> >> Makes sense, but I think I'll go with prev_shadow_apm, >> prev_shadow_aqm and >> prev_shadow_adm. That seems more accurate since these are not the >> original >> copies of the bitmaps, but copies of the previous versions prior to >> filtering. > > That works for me :) Thanks! In general, I like to avoid generic > variable names > like "mask" or "thing" whenever possible. Especially if I'm dealing > with multiple > instances of the same type of data within the same scope. Giving each > variable a > specific name can really help de-obfuscate the code.
Agreed. It has been done.
> >>> >>> ... >>>> +/** >>>> + * vfio_ap_mdev_get_locks - acquire the locks required to >>>> assign/unassign AP >>>> + * adapters, domains and control domains for an >>>> mdev in >>>> + * the proper locking order. >>>> + * >>>> + * @matrix_mdev: the matrix mediated device object >>>> + */ >>>> +static void vfio_ap_mdev_get_locks(struct ap_matrix_mdev >>>> *matrix_mdev) >>>> +{ >>>> + /* Lock the mutex required to access the KVM guest's state */ >>>> + mutex_lock(&matrix_dev->guests_lock); >>>> + >>>> + /* If a KVM guest is running, lock the mutex required to >>>> plug/unplug the >>>> + * AP devices passed through to the guest >>>> + */ >>>> + if (matrix_mdev->kvm) >>>> + mutex_lock(&matrix_mdev->kvm->lock); >>>> + >>>> + /* The lock required to access the mdev's state */ >>>> + mutex_lock(&matrix_dev->mdevs_lock); >>>> +} >>> >>> Simplifying the cdoe, and removing duplication by moving the locking >>> code to a >>> function is probably a good thing. But I don't feel like this >>> belongs to this >>> particular patch. In general, a patch should only do one thing, and >>> ideally that >>> one thing should be as small as reasonably possible. This makes the >>> patch easier >>> to read and to review. >>> >>> I feel like, as much as possible, you should refactor the locking in >>> a series >>> of patches that are all kept together. Ideally, they would be a >>> patch series >>> completely separate from dynamic ap. After all, this series is >>> already at 18 >>> patches. :) >> >> I'm going to have to disagree, this locking scheme makes no sense >> outside of >> this series. It is only necessary because we now update a guest's APCB >> whenever an adapter, domain or control domain is assigned or unassigned, >> when a queue device is probed or removed and when the vfio_ap driver is >> notified that the host's AP configuration has changed. >> >> Prior to this series, a guest's APCB was updated only when the vfio_ap >> driver was notified that the KVM pointer was set or cleared, so it was >> only necessary to ensure the kvm->lock is taken before the >> matrix_dev->lock >> in the functions that handle the VFIO_GROUP_NOTIFY_SET_KVM group >> notification event. Prior to this, a patch series to introduce the >> matrix_dev->guests lock >> would make no sense because it is not needed to enforce the locking >> order in those >> functions listed in the previous paragraph because we didn't update >> the guest's >> APCB in those functions. >> > > I don't understand the lock code enough to argue a whole lot here :) > But I do still > think, at the very least, that your refactoring of the locking into > get_locks/put_locks > functions really does belong in a separate patch. Refactoring is not > directly related to > the hotplug/unplug. Also, this is not a minor refactor. This > refactoring touches the code > all over the place and really just adds noise to this patch. That > noise makes it harder > to review.
Okay, this is a little different than what you were asking for in your previous review comment in which you suggested the locking patches should be in a series separate from dynamic ap. This I can get on board with; however, we are now talking about increasing the number of patches in this series.
> > >>> ... >>>> /** >>>> * assign_adapter_store - parses the APID from @buf and sets the >>>> * corresponding bit in the mediated matrix device's APM >>>> @@ -649,17 +723,9 @@ static ssize_t assign_adapter_store(struct >>>> device *dev, >>>> int ret; >>>> unsigned long apid; >>>> DECLARE_BITMAP(apm, AP_DEVICES); >>>> - >>>> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); >>>> - mutex_lock(&matrix_dev->guests_lock); >>>> - mutex_lock(&matrix_dev->mdevs_lock); >>>> - >>>> - /* If the KVM guest is running, disallow assignment of adapter */ >>>> - if (matrix_mdev->kvm) { >>>> - ret = -EBUSY; >>>> - goto done; >>>> - } >>>> + vfio_ap_mdev_get_locks(matrix_mdev); >>>> ret = kstrtoul(buf, 0, &apid); >>>> if (ret) >>>> @@ -671,8 +737,6 @@ static ssize_t assign_adapter_store(struct >>>> device *dev, >>>> } >>>> set_bit_inv(apid, matrix_mdev->matrix.apm); >>>> - memset(apm, 0, sizeof(apm)); >>>> - set_bit_inv(apid, apm); >>>> ret = vfio_ap_mdev_validate_masks(matrix_mdev); >>> >>> It looks like you moved the memset() and set_bit_inv() to be closer >>> to where >>> "apm" is used, namely, the call to vfio_ap_mdev_filter_matrix(). Any >>> reason you >>> cannot move it down under the call to vfio_ap_mdev_link_adapter()? >>> That would >>> get it even closer to where it is used. >> >> I didn't move it to be closer to where it is used, I moved it because >> it was not >> necessary to do the memset/set_bit_inv when not necessary to do so. >> Having >> said that, it can definitely be moved after the >> vfio_ap_mdev_link_adapter(). >> >>> >>> Also, I think renaming apm to apm_delta or apm_diff makes sense >>> here. After all, >>> it is the difference between the original apm, and the new apm. The >>> new apm >>> has an extra bit for the newly added adapter. Do I have that right? >>> If so, I >>> think renaming the variable will make the code clearer. >> >> The purpose of this bitmap is to limit the filtering to the new APID >> being assigned >> because there is no need to do filtering of adapters already >> assigned; so, it is not >> really a new apm per se. It might be more accurate to call it >> new_apid or new_apids, >> although there will only be one bit set in the bitmap. > > My main concern here was generic variables names. The "new_apm" will > have exactly one > new bit set. That bit is the delta (or the difference) between the > previously existing apm, and the new apm, which will be the result of > adding in whatever the "apid" bit is. Therefore, it really is a delta, > right? This was the basis for my suggestion of the > name. Its really not the "new" apm... its the difference between the > old and new.
I overlooked the names you suggested - i.e., apm_delta/apm_diff - and for some reason beamed in on "new apm" as if you were suggesting that as the name of the variable. Of the two names, I prefer apm_delta, so I will go with that.
> > >
| |