| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e0630d1f-bff1-dd94-b8e5-52421f101217@linux.ibm.com>
Date: Wed, 30 Mar 2022 15:26:55 -0400
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: jjherne@...ux.ibm.com, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com,
mjrosato@...ux.ibm.com, pasic@...ux.ibm.com,
alex.williamson@...hat.com, kwankhede@...dia.com,
fiuczy@...ux.ibm.com
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.
Powered by blists - more mailing lists