Messages in this thread | | | Date | Wed, 30 Mar 2022 15:26:55 -0400 | From | Tony Krowiak <> | Subject | Re: [PATCH v18 15/18] s390/vfio-ap: handle config changed and scan complete notification |
| |
On 3/24/22 10:09, Jason J. Herne wrote: > On 2/14/22 19:50, Tony Krowiak wrote: > ... >> @@ -790,13 +788,17 @@ static void vfio_ap_unlink_apqn_fr_mdev(struct >> ap_matrix_mdev *matrix_mdev, >> q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi)); >> /* If the queue is assigned to the matrix mdev, unlink it. */ >> - if (q) >> + if (q) { >> vfio_ap_unlink_queue_fr_mdev(q); >> - /* If the queue is assigned to the APCB, store it in @qtable. */ >> - if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) && >> - test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) >> - hash_add(qtable->queues, &q->mdev_qnode, q->apqn); >> + /* If the queue is assigned to the APCB, store it in >> @qtable. */ >> + if (qtable) { >> + if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) && >> + test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) >> + hash_add(qtable->queues, &q->mdev_qnode, >> + q->apqn); >> + } >> + } >> } > > This appears to be an unrelated change. Does this belong in this patch?
No, that does not belong in this patch, it belongs in patch [PATCH v18 12/18] s390/vfio-ap: reset queues after adapter/domain unassignment.
> > >> /** >> @@ -1271,7 +1273,7 @@ static const struct attribute_group >> *vfio_ap_mdev_attr_groups[] = { >> * @matrix_mdev: a mediated matrix device >> * @kvm: reference to KVM instance >> * >> - * Note: The matrix_dev->lock must be taken prior to calling >> + * Note: The matrix_dev->mdevs_lock must be taken prior to calling > > This also seems to be unrelated.
The change of the name of the matrix_dev->lock to matrix_dev->mdevs_lock was done in [PATCH v18 09/18] s390/vfio-ap: introduce new mutex to control access to the KVM pointer; however, this comment is not valid. A commit has been pushed for upstream merge which includes removal of this comment.
> > >> * this function; however, the lock will be temporarily released >> while the >> * guest's AP configuration is set to avoid a potential lockdep splat. >> * The kvm->lock is taken to set the guest's AP configuration >> which, under >> @@ -1355,7 +1357,7 @@ static int vfio_ap_mdev_iommu_notifier(struct >> notifier_block *nb, >> * @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 >> + * Note: The matrix_dev->mdevs_lock must be taken prior to calling > > Same here.
Ditto
> >> * this function; however, the lock will be temporarily released >> while the >> * guest's AP configuration is cleared to avoid a potential lockdep >> splat. >> * The kvm->lock is taken to clear the guest's AP configuration >> which, under >> @@ -1708,6 +1710,27 @@ static void vfio_ap_mdev_put_qlocks(struct >> ap_matrix_mdev *matrix_mdev) >> mutex_unlock(&matrix_dev->guests_lock); >> } >> +static bool vfio_ap_mdev_do_filter_matrix(struct ap_matrix_mdev >> *matrix_mdev, >> + struct vfio_ap_queue *q) >> +{ >> + unsigned long apid = AP_QID_CARD(q->apqn); >> + unsigned long apqi = AP_QID_QUEUE(q->apqn); >> + >> + /* >> + * If the queue is being probed because its APID or APQI is in the >> + * process of being added to the host's AP configuration, then >> we don't >> + * want to filter the matrix now as the filtering will be done >> after >> + * the driver is notified that the AP bus scan operation has >> completed >> + * (see the vfio_ap_on_scan_complete callback function). >> + */ >> + if (test_bit_inv(apid, matrix_mdev->apm_add) || >> + test_bit_inv(apqi, matrix_mdev->aqm_add)) >> + return false; >> + >> + >> + return true; >> +} >> + >> int vfio_ap_mdev_probe_queue(struct ap_device *apdev) >> { >> struct vfio_ap_queue *q; >> @@ -1725,10 +1748,15 @@ int vfio_ap_mdev_probe_queue(struct ap_device >> *apdev) >> vfio_ap_mdev_link_queue(matrix_mdev, q); >> memset(apm, 0, sizeof(apm)); >> set_bit_inv(AP_QID_CARD(q->apqn), apm); >> - if (vfio_ap_mdev_filter_matrix(apm, q->matrix_mdev->matrix.aqm, >> - q->matrix_mdev)) >> - vfio_ap_mdev_hotplug_apcb(q->matrix_mdev); >> + >> + if (vfio_ap_mdev_do_filter_matrix(q->matrix_mdev, q)) { >> + if (vfio_ap_mdev_filter_matrix(apm, >> + q->matrix_mdev->matrix.aqm, >> + q->matrix_mdev)) >> + vfio_ap_mdev_hotplug_apcb(q->matrix_mdev); >> + } >> } >> + >> dev_set_drvdata(&apdev->device, q); >> vfio_ap_mdev_put_qlocks(matrix_mdev); >> @@ -1783,10 +1811,15 @@ void vfio_ap_mdev_remove_queue(struct >> ap_device *apdev) >> apid = AP_QID_CARD(q->apqn); >> apqi = AP_QID_QUEUE(q->apqn); >> - if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) && >> - test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) { >> - clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm); >> - vfio_ap_mdev_hotplug_apcb(matrix_mdev); >> + >> + /* >> + * If the queue is assigned to the guest's APCB, then remove >> + * the adapter's APID from the APCB and hot it into the guest. >> + */ >> + if (test_bit_inv(apid, q->matrix_mdev->shadow_apcb.apm) && >> + test_bit_inv(apqi, q->matrix_mdev->shadow_apcb.aqm)) { >> + clear_bit_inv(apid, q->matrix_mdev->shadow_apcb.apm); >> + vfio_ap_mdev_hotplug_apcb(q->matrix_mdev); > > It looks like this a bug fix unrelated to this patch...?
It's not a bug fix, but it is unnecessary. Both what was removed and the code added are fine, but the former makes more sense. I have no idea how this happened other than the code was accidentally changed while merging code during one of the multitude of rebases undertaken while developing this series. > >> @@ -1842,3 +1875,267 @@ int vfio_ap_mdev_resource_in_use(unsigned >> long *apm, unsigned long *aqm) >> return ret; >> } >> + >> +/** >> + * vfio_ap_mdev_hot_unplug_cfg - hot unplug the adapters, domains >> and control >> + * domains that have been removed from the host's >> + * AP configuration from a guest. >> + * >> + * @guest: the guest >> + * @aprem: the adapters that have been removed from the host's AP >> configuration >> + * @aqrem: the domains that have been removed from the host's AP >> configuration >> + * @cdrem: the control domains that have been removed from the >> host's AP >> + * configuration. >> + */ >> +static void vfio_ap_mdev_hot_unplug_cfg(struct ap_matrix_mdev >> *matrix_mdev, >> + unsigned long *aprem, >> + unsigned long *aqrem, >> + unsigned long *cdrem) >> +{ >> + bool do_hotplug = false; > > __bitmap_andnot() returns an int, so I think you should use an int here.
Okay
> > >> + if (!bitmap_empty(aprem, AP_DEVICES)) { >> + do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm, >> + matrix_mdev->shadow_apcb.apm, >> + aprem, AP_DEVICES); > > Also, replace the |= with an = here. This is the first assignment so no > need to do a logical OR as there is no pre-existing data to preserve.
We don't know which, if any, of the bitmaps (aprem, aqrem, cdrem) passed to this function contain set bits, so the |= is necessary because this code block won't be be executed if aprem is empty. That is why the do_hotplug variable is initialized to false when it is declared (i.e., each of the bitmaps might be empty).
> > >> + } >> + >> + if (!bitmap_empty(aqrem, AP_DOMAINS)) { >> + do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.aqm, >> + matrix_mdev->shadow_apcb.aqm, >> + aqrem, AP_DEVICES); >> + } >> + >> + if (!bitmap_empty(cdrem, AP_DOMAINS)) >> + do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.adm, >> + matrix_mdev->shadow_apcb.adm, >> + cdrem, AP_DOMAINS); >> + >> + if (do_hotplug) >> + vfio_ap_mdev_hotplug_apcb(matrix_mdev); >> +} >> + >> +/** >> + * vfio_ap_mdev_cfg_remove - determines which guests are using the >> adapters, >> + * domains and control domains that have been removed >> + * from the host AP configuration and unplugs them >> + * from those guests. >> + * >> + * @ap_remove: bitmap specifying which adapters have been removed >> from the host >> + * config. >> + * @aq_remove: bitmap specifying which domains have been removed >> from the host >> + * config. >> + * @cd_remove: bitmap specifying which control domains have been >> removed from >> + * the host config. >> + */ > ... >> +/** >> + * vfio_ap_mdev_on_cfg_remove - responds to the removal of adapters, >> domains and >> + * control domains from the host AP configuration >> + * by unplugging them from the guests that are >> + * using them. >> + */ >> +static void vfio_ap_mdev_on_cfg_remove(void) >> +{ >> + int ap_remove, aq_remove, cd_remove; > > These can all be replaced with a single variable, just like you did > with do_add.
True, consider it done.
> >> + DECLARE_BITMAP(aprem, AP_DEVICES); >> + DECLARE_BITMAP(aqrem, AP_DOMAINS); >> + DECLARE_BITMAP(cdrem, AP_DOMAINS); >> + unsigned long *cur_apm, *cur_aqm, *cur_adm; >> + unsigned long *prev_apm, *prev_aqm, *prev_adm; >> + >> + cur_apm = (unsigned long *)matrix_dev->config_info.apm; >> + cur_aqm = (unsigned long *)matrix_dev->config_info.aqm; >> + cur_adm = (unsigned long *)matrix_dev->config_info.adm; >> + prev_apm = (unsigned long *)matrix_dev->config_info_prev.apm; >> + prev_aqm = (unsigned long *)matrix_dev->config_info_prev.aqm; >> + prev_adm = (unsigned long *)matrix_dev->config_info_prev.adm; >> + >> + ap_remove = bitmap_andnot(aprem, prev_apm, cur_apm, AP_DEVICES); >> + aq_remove = bitmap_andnot(aqrem, prev_aqm, cur_aqm, AP_DOMAINS); >> + cd_remove = bitmap_andnot(cdrem, prev_adm, cur_adm, AP_DOMAINS); >> + >> + if (ap_remove || aq_remove || cd_remove) >> + vfio_ap_mdev_cfg_remove(aprem, aqrem, cdrem); >> +} > ... >> +/** >> + * vfio_ap_on_cfg_changed - handles notification of changes to the >> host AP >> + * configuration. >> + * >> + * @new_config_info: the new host AP configuration >> + * @old_config_info: the previous host AP configuration >> + */ >> +void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info, >> + struct ap_config_info *old_config_info) >> +{ >> + mutex_lock(&matrix_dev->guests_lock); >> + >> + memcpy(&matrix_dev->config_info_prev, old_config_info, >> + sizeof(struct ap_config_info)); >> + memcpy(&matrix_dev->config_info, new_config_info, >> + sizeof(struct ap_config_info)); > > Why are we storing old_config_info in the matrix_dev? It appears to only > be used within the functions called from right here. Why not just pass it > as an argument?
At the time I thought it might be valuable to keep around. I suppose it can be passed to the functions that need it.
> > >> + vfio_ap_mdev_on_cfg_remove(); >> + vfio_ap_mdev_on_cfg_add(); >> + >> + mutex_unlock(&matrix_dev->guests_lock); >> +} > > Here is an idea to restructure things... consider combining logic from > vfio_ap_mdev_on_cfg_remove and vfio_ap_mdev_on_cfg_add with > vfio_ap_on_cfg_changed. This makes vfio_ap_on_cfg_changed() longer but > it eliminates some duplicated code and gets rid of both > vfio_ap_mdev_on_cfg_remove and vfio_ap_mdev_on_cfg_add. > note: Untested... :)
It seems like a lot of busy work for little gain. I prefer shorter functions that perform a specific task. Since we're solely in the realm of personal preferences, I'm going to leave this function as-is.
| |