Re: [RFC 2/7] s390/vfio-ap: circumvent filtering for adapters/domains not in host config

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 17 Oct 2023 18:22:49 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> While filtering the mdev matrix, it doesn't make sense - and will have
> unexpected results - to filter an APID from the matrix if the APID or one
> of the associated APQIs is not in the host's AP configuration. There are
> two reasons for this:
> 
> 1. An adapter or domain that is not in the host's AP configuration can be
>    assigned to the matrix; this is known as over-provisioning. Queue
>    devices, however, are only created for adapters and domains in the
>    host's AP configuration, so there will be no queues associated with an
>    over-provisioned adapter or domain to filter.
> 
> 2. The adapter or domain may have been externally removed from the host's
>    configuration via an SE or HMC attached to a DPM enabled LPAR. In this
>    case, the vfio_ap device driver would have been notified by the AP bus
>    via the on_config_changed callback and the adapter or domain would
>    have already been filtered.
> 
> Let's bypass the filtering of an APID if an adapter or domain assigned to
> the mdev matrix is not in the host's AP configuration.

I strongly agree.

> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Fixes: 48cae940c31d ("s390/vfio-ap: refresh guest's APCB by filtering AP resources assigned to mdev")
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 32 +++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e5490640e19c..4e40e226ce62 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -692,17 +692,37 @@ static bool vfio_ap_mdev_filter_matrix(struct ap_matrix_mdev *matrix_mdev)
>  		   (unsigned long *)matrix_dev->info.aqm, AP_DOMAINS);
> 
>  	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES) {

What speaks against doing the loop on matrix_mdev->shadow_apcb.a[pq]m?

Those are the and of matrix_mdev->matrix.a{p,q}m and
matrix_dev->info.a{p,q}m so excactly those bits are 0 for which you are adding
the ifs...

> +		/*
> +		 * If the adapter is not in the host's AP configuration, it will
> +		 * be due to one of two reasons:
> +		 * 1. The adapter is over-provisioned.
> +		 * 2. The adapter was removed from the host's
> +		 *    configuration in which case it will already have
> +		 *    been processed by the on_config_changed callback.
> +		 * In either case, we should skip the filtering and
> +		 * continue examining APIDs.
> +		 */
> +		if (!test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm))
> +			continue;
> +
>  		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS) {
>  			/*
> -			 * If the APQN is not bound to the vfio_ap device
> -			 * driver, then we can't assign it to the guest's
> -			 * AP configuration. The AP architecture won't
> -			 * allow filtering of a single APQN, so let's filter
> -			 * the APID since an adapter represents a physical
> -			 * hardware device.
> +			 * If the domain is not in the host's AP configuration,
> +			 * it will for one of two reasons:
> +			 * 1. The domain is over-provisioned.
> +			 * 2. The domain was removed from the host's
> +			 *    configuration in which case it will already have
> +			 *    been processed by the on_config_changed callback.
> +			 * In either case, we should skip the filtering and
> +			 * continue examining APQIs.
>  			 */
> +			if (!test_bit_inv(apqi,
> +					  (unsigned long *)matrix_dev->info.aqm))
> +				continue;
> +
>  			apqn = AP_MKQID(apid, apqi);
>  			q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
> +
>  			if (!q || q->reset_status.response_code) {
>  				clear_bit_inv(apid,
>  					      matrix_mdev->shadow_apcb.apm);