Date:   Thu, 24 Mar 2022 10:09:11 -0400
From:   "Jason J. Herne" <jjherne@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...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 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?


>   /**
> @@ -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.


>    * 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.

>    * 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...?

>   
> @@ -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.


> +	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.


> +	}
> +
> +	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.

> +	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?

> +	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... :)

void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
			    struct ap_config_info *old_config_info)
{
	DECLARE_BITMAP(changed_adapters, AP_DEVICES);
	DECLARE_BITMAP(changed_domains, AP_DOMAINS);
	DECLARE_BITMAP(changed_cdoms, AP_DOMAINS);
	unsigned long *cur_apm, *cur_aqm, *cur_adm;
	unsigned long *prev_apm, *prev_aqm, *prev_adm;

	cur_apm = (unsigned long *)new_config_info->apm;
	cur_aqm = (unsigned long *)new_config_info->aqm;
	cur_adm = (unsigned long *)new_config_info->adm;
	prev_apm = (unsigned long *)old_config_info->apm;
	prev_aqm = (unsigned long *)old_config_info->.aqm;
	prev_adm = (unsigned long *)old_config_info->adm;

	mutex_lock(&matrix_dev->guests_lock);
	
	/* Handle host configuration removals */
	bitmap_andnot(changed_adapters, prev_apm, cur_apm, AP_DEVICES);
	bitmap_andnot(changed_domains, prev_aqm, cur_aqm, AP_DOMAINS);
	bitmap_andnot(changed_cdoms, prev_adm, cur_adm, AP_DOMAINS);

	if (changed_adapters || changed_domains || changed_cdoms)
		vfio_ap_mdev_cfg_remove(aprem, aqrem, cdrem);

	bitmap_clear(changed_adapters, 0, AP_DEVICES);
	bitmap_clear(changed_domains, 0, AP_DOMAINS);
	bitmap_clear(changed_cdoms, 0, AP_DOMAINS);

	/* Handle host configuration additions */
	bitmap_andnot(changed_adapters, cur_apm, prev_apm, AP_DEVICES);
	bitmap_andnot(changed_domains, cur_aqm, prev_aqm, AP_DOMAINS);
	bitmap_andnot(changed_cdoms, cur_adm, prev_adm, AP_DOMAINS);

	if (changed_adapters || changed_domains || changed_cdoms) {
		vfio_ap_mdev_cfg_add(apm_add, aqm_add, adm_add);
	}
	mutex_unlock(&matrix_dev->guests_lock);
}

Not sure if where I put the locking is 100% correct. We could do a
lock/unlock around each call to vfio_ap_mdev_cfg_{remove|add} but
there is probablyt no point to that, right?

Also, a side effect of these changes is that matrix_dev->config_info
is no longer updated. I guess we either would need to update it here
or update it wherever it was originally updated before this patrch.

-- 
-- Jason J. Herne (jjherne@...ux.ibm.com)

Powered by blists - more mailing lists