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 <[email protected]>
---
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);
+}
+
+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);
}
/*
@@ -330,17 +345,24 @@ static void vfio_ap_mdev_filter_cdoms(struct ap_matrix_mdev *matrix_mdev)
* queue device bound to the vfio_ap device driver.
*
* @matrix_mdev: the mdev whose AP configuration is to be filtered.
+ *
+ * Return: a boolean value indicating whether the KVM guest's APCB was changed
+ * by the filtering or not.
*/
-static void vfio_ap_mdev_filter_matrix(unsigned long *apm, unsigned long *aqm,
+static bool vfio_ap_mdev_filter_matrix(unsigned long *apm, unsigned long *aqm,
struct ap_matrix_mdev *matrix_mdev)
{
int ret;
unsigned long apid, apqi, apqn;
+ DECLARE_BITMAP(shadow_apm, AP_DEVICES);
+ DECLARE_BITMAP(shadow_aqm, AP_DOMAINS);
ret = ap_qci(&matrix_dev->info);
if (ret)
- return;
+ return false;
+ bitmap_copy(shadow_apm, matrix_mdev->shadow_apcb.apm, AP_DEVICES);
+ bitmap_copy(shadow_aqm, matrix_mdev->shadow_apcb.aqm, AP_DOMAINS);
vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb);
/*
@@ -372,6 +394,11 @@ static void vfio_ap_mdev_filter_matrix(unsigned long *apm, unsigned long *aqm,
}
}
}
+
+ return !bitmap_equal(shadow_apm, matrix_mdev->shadow_apcb.apm,
+ AP_DEVICES) ||
+ !bitmap_equal(shadow_aqm, matrix_mdev->shadow_apcb.aqm,
+ AP_DOMAINS);
}
static int vfio_ap_mdev_probe(struct mdev_device *mdev)
@@ -472,11 +499,13 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev)
vfio_unregister_group_dev(&matrix_mdev->vdev);
+ mutex_lock(&matrix_dev->guests_lock);
mutex_lock(&matrix_dev->mdevs_lock);
vfio_ap_mdev_reset_queues(matrix_mdev);
vfio_ap_mdev_unlink_fr_queues(matrix_mdev);
list_del(&matrix_mdev->node);
mutex_unlock(&matrix_dev->mdevs_lock);
+ mutex_unlock(&matrix_dev->guests_lock);
vfio_uninit_group_dev(&matrix_mdev->vdev);
kfree(matrix_mdev);
atomic_inc(&matrix_dev->available_instances);
@@ -612,6 +641,51 @@ static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
AP_MKQID(apid, apqi));
}
+/**
+ * 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);
+}
+
+/**
+ * vfio_ap_mdev_put_locks - release the locks used to assign/unassign AP
+ * adapters, domains and control domains in the proper
+ * unlocking order.
+ *
+ * @matrix_mdev: the matrix mediated device object
+ */
+static void vfio_ap_mdev_put_locks(struct ap_matrix_mdev *matrix_mdev)
+{
+ /* Unlock the mutex taken to access the matrix_mdev's state */
+ mutex_unlock(&matrix_dev->mdevs_lock);
+
+ /*
+ * If a KVM guest is running, unlock the mutex taken to plug/unplug the
+ * AP devices passed through to the guest.
+ */
+ if (matrix_mdev->kvm)
+ mutex_unlock(&matrix_mdev->kvm->lock);
+
+ /* Unlock the mutex taken to allow access to the KVM guest's state */
+ mutex_unlock(&matrix_dev->guests_lock);
+}
+
/**
* 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);
if (ret) {
@@ -680,12 +744,17 @@ static ssize_t assign_adapter_store(struct device *dev,
goto done;
}
+ memset(apm, 0, sizeof(apm));
+ set_bit_inv(apid, apm);
vfio_ap_mdev_link_adapter(matrix_mdev, apid);
- vfio_ap_mdev_filter_matrix(apm, matrix_mdev->matrix.aqm, matrix_mdev);
+
+ if (vfio_ap_mdev_filter_matrix(apm,
+ matrix_mdev->matrix.aqm, matrix_mdev))
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+
ret = count;
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
- mutex_unlock(&matrix_dev->guests_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
return ret;
}
@@ -728,13 +797,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
unsigned long apid;
struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- mutex_lock(&matrix_dev->mdevs_lock);
-
- /* If the KVM guest is running, disallow unassignment of adapter */
- if (matrix_mdev->kvm) {
- ret = -EBUSY;
- goto done;
- }
+ vfio_ap_mdev_get_locks(matrix_mdev);
ret = kstrtoul(buf, 0, &apid);
if (ret)
@@ -748,12 +811,15 @@ static ssize_t unassign_adapter_store(struct device *dev,
clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
vfio_ap_mdev_unlink_adapter(matrix_mdev, apid);
- if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
+ if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+ }
ret = count;
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
+
return ret;
}
static DEVICE_ATTR_WO(unassign_adapter);
@@ -806,28 +872,19 @@ static ssize_t assign_domain_store(struct device *dev,
unsigned long apqi;
DECLARE_BITMAP(aqm, AP_DOMAINS);
struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
- mutex_lock(&matrix_dev->guests_lock);
- mutex_lock(&matrix_dev->mdevs_lock);
-
- /* If the KVM guest is running, disallow assignment of domain */
- if (matrix_mdev->kvm) {
- ret = -EBUSY;
- goto done;
- }
+ vfio_ap_mdev_get_locks(matrix_mdev);
ret = kstrtoul(buf, 0, &apqi);
if (ret)
goto done;
- if (apqi > max_apqi) {
+
+ if (apqi > matrix_mdev->matrix.apm_max) {
ret = -ENODEV;
goto done;
}
set_bit_inv(apqi, matrix_mdev->matrix.aqm);
- memset(aqm, 0, sizeof(aqm));
- set_bit_inv(apqi, aqm);
ret = vfio_ap_mdev_validate_masks(matrix_mdev);
if (ret) {
@@ -835,12 +892,17 @@ static ssize_t assign_domain_store(struct device *dev,
goto done;
}
+ memset(aqm, 0, sizeof(aqm));
+ set_bit_inv(apqi, aqm);
vfio_ap_mdev_link_domain(matrix_mdev, apqi);
- vfio_ap_mdev_filter_matrix(matrix_mdev->matrix.apm, aqm, matrix_mdev);
+
+ if (vfio_ap_mdev_filter_matrix(matrix_mdev->matrix.apm, aqm,
+ matrix_mdev))
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+
ret = count;
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
- mutex_unlock(&matrix_dev->guests_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
return ret;
}
@@ -883,19 +945,13 @@ static ssize_t unassign_domain_store(struct device *dev,
unsigned long apqi;
struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- mutex_lock(&matrix_dev->mdevs_lock);
-
- /* If the KVM guest is running, disallow unassignment of domain */
- if (matrix_mdev->kvm) {
- ret = -EBUSY;
- goto done;
- }
+ vfio_ap_mdev_get_locks(matrix_mdev);
ret = kstrtoul(buf, 0, &apqi);
if (ret)
goto done;
- if (apqi > matrix_mdev->matrix.aqm_max) {
+ if (apqi > matrix_mdev->matrix.apm_max) {
ret = -ENODEV;
goto done;
}
@@ -903,13 +959,15 @@ static ssize_t unassign_domain_store(struct device *dev,
clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
vfio_ap_mdev_unlink_domain(matrix_mdev, apqi);
- if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
+ if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+ }
ret = count;
-
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
+
return ret;
}
static DEVICE_ATTR_WO(unassign_domain);
@@ -936,19 +994,13 @@ static ssize_t assign_control_domain_store(struct device *dev,
unsigned long id;
struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- mutex_lock(&matrix_dev->mdevs_lock);
-
- /* If the KVM guest is running, disallow assignment of control domain */
- if (matrix_mdev->kvm) {
- ret = -EBUSY;
- goto done;
- }
+ vfio_ap_mdev_get_locks(matrix_mdev);
ret = kstrtoul(buf, 0, &id);
if (ret)
goto done;
- if (id > matrix_mdev->matrix.adm_max) {
+ if (id > matrix_mdev->matrix.apm_max) {
ret = -ENODEV;
goto done;
}
@@ -959,10 +1011,13 @@ static ssize_t assign_control_domain_store(struct device *dev,
* number of control domains that can be assigned.
*/
set_bit_inv(id, matrix_mdev->matrix.adm);
- vfio_ap_mdev_filter_cdoms(matrix_mdev);
+ if (vfio_ap_mdev_filter_cdoms(matrix_mdev))
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+
ret = count;
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
+
return ret;
}
static DEVICE_ATTR_WO(assign_control_domain);
@@ -988,32 +1043,29 @@ static ssize_t unassign_control_domain_store(struct device *dev,
int ret;
unsigned long domid;
struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- unsigned long max_domid = matrix_mdev->matrix.adm_max;
- mutex_lock(&matrix_dev->mdevs_lock);
-
- /* If a KVM guest is running, disallow unassignment of control domain */
- if (matrix_mdev->kvm) {
- ret = -EBUSY;
- goto done;
- }
+ vfio_ap_mdev_get_locks(matrix_mdev);
ret = kstrtoul(buf, 0, &domid);
if (ret)
goto done;
- if (domid > max_domid) {
+
+ if (domid > matrix_mdev->matrix.apm_max) {
ret = -ENODEV;
goto done;
}
clear_bit_inv(domid, matrix_mdev->matrix.adm);
- if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm))
+ if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
clear_bit_inv(domid, matrix_mdev->shadow_apcb.adm);
+ vfio_ap_mdev_hotplug_apcb(matrix_mdev);
+ }
ret = count;
done:
- mutex_unlock(&matrix_dev->mdevs_lock);
+ vfio_ap_mdev_put_locks(matrix_mdev);
+
return ret;
}
static DEVICE_ATTR_WO(unassign_control_domain);
--
2.31.1
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 <[email protected]>
>> ---
>> 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.
>
>
>
>
>> +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.
>
> ...
>> +/**
>> + * 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.
>
> ...
>> /**
>> * 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.
>
>
> Both of the above comments also apply to assign_domain_store().
Ditto on the responses.
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 <[email protected]>
> ---
> 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(). You can also call this function in vfio_ap_mdev_set_kvm,
instead of duplicating the code to call kvm_arch_crypto_set_masks().
> +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().
...
> +/**
> + * 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. :)
...
> /**
> * 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.
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.
Both of the above comments also apply to assign_domain_store().
--
-- Jason J. Herne ([email protected])
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 <[email protected]>
>>> ---
>>> 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.
>>
>>
>>
>>
>>> +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.
>>
>> ...
>>> +/**
>>> + * 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.
>> ...
>>> /**
>>> * 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.
--
-- Jason J. Herne ([email protected])
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 <[email protected]>
>>>> ---
>>>> 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.
>
>
>