Re: [PATCH v18 15/18] s390/vfio-ap: handle config changed and scan complete notification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux