kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Halil Pasic <pasic@linux.ibm.com>
To: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, freude@linux.ibm.com,
	borntraeger@de.ibm.com, cohuck@redhat.com,
	mjrosato@linux.ibm.com, alex.williamson@redhat.com,
	kwankhede@nvidia.com, fiuczy@linux.ibm.com,
	frankja@linux.ibm.com, david@redhat.com, hca@linux.ibm.com,
	gor@linux.ibm.com
Subject: Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
Date: Mon, 11 Jan 2021 17:32:06 +0100	[thread overview]
Message-ID: <20210111173206.27808b79.pasic@linux.ibm.com> (raw)
In-Reply-To: <20201223011606.5265-3-akrowiak@linux.ibm.com>

On Tue, 22 Dec 2020 20:15:53 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> The queues assigned to a matrix mediated device are currently reset when:
> 
> * The VFIO_DEVICE_RESET ioctl is invoked
> * The mdev fd is closed by userspace (QEMU)
> * The mdev is removed from sysfs.
> 
> Immediately after the reset of a queue, a call is made to disable
> interrupts for the queue. This is entirely unnecessary because the reset of
> a queue disables interrupts, so this will be removed.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     |  1 -
>  drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
>  drivers/s390/crypto/vfio_ap_private.h |  1 -
>  3 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index be2520cc010b..ca18c91afec9 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>  	apid = AP_QID_CARD(q->apqn);
>  	apqi = AP_QID_QUEUE(q->apqn);
>  	vfio_ap_mdev_reset_queue(apid, apqi, 1);
> -	vfio_ap_irq_disable(q);
>  	kfree(q);
>  	mutex_unlock(&matrix_dev->lock);
>  }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 7339043906cf..052f61391ec7 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -25,6 +25,7 @@
>  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>  
>  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>  
>  static int match_apqn(struct device *dev, const void *data)
>  {
> @@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
>  					int apqn)
>  {
>  	struct vfio_ap_queue *q;
> -	struct device *dev;
>  
>  	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
>  		return NULL;
>  	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
>  		return NULL;
>  
> -	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> -				 &apqn, match_apqn);
> -	if (!dev)
> -		return NULL;
> -	q = dev_get_drvdata(dev);
> -	q->matrix_mdev = matrix_mdev;
> -	put_device(dev);
> +	q = vfio_ap_find_queue(apqn);
> +	if (q)
> +		q->matrix_mdev = matrix_mdev;
>  
>  	return q;
>  }
> @@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>  	return notify_rc;
>  }
>  
> -static void vfio_ap_irq_disable_apqn(int apqn)
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
>  {
>  	struct device *dev;
> -	struct vfio_ap_queue *q;
> +	struct vfio_ap_queue *q = NULL;
>  
>  	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>  				 &apqn, match_apqn);
>  	if (dev) {
>  		q = dev_get_drvdata(dev);
> -		vfio_ap_irq_disable(q);
>  		put_device(dev);
>  	}
> +
> +	return q;
>  }

This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
have next to nothing to do with the patch's objective. If we were at an
earlier stage, I would ask to split it up.

>  
>  int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>  			     unsigned int retry)
>  {
>  	struct ap_queue_status status;
> +	struct vfio_ap_queue *q;
> +	int ret;
>  	int retry2 = 2;
>  	int apqn = AP_MKQID(apid, apqi);
>  
> @@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>  				status = ap_tapq(apqn, NULL);
>  			}
>  			WARN_ON_ONCE(retry2 <= 0);
> -			return 0;
> +			ret = 0;
> +			goto free_aqic_resources;
>  		case AP_RESPONSE_RESET_IN_PROGRESS:
>  		case AP_RESPONSE_BUSY:
>  			msleep(20);
>  			break;
>  		default:
>  			/* things are really broken, give up */
> -			return -EIO;
> +			ret = -EIO;
> +			goto free_aqic_resources;

Do we really want the unpin here? I mean the reset did not work and
we are giving up. So the irqs are potentially still enabled.

Without this patch we try to disable the interrupts using AQIC, and
do the cleanup after that.

I'm aware, the comment says we should not take the default branch,
but if that's really the case we should IMHO log an error and leak the
page.

It's up to you if you want to change this. I don't want to delay the
series any further than absolutely necessary.

Acked-by: Halil Pasic <pasic@linux.ibm.com>

>  		}
>  	} while (retry--);
>  
>  	return -EBUSY;
> +
> +free_aqic_resources:
> +	/*
> +	 * In order to free the aqic resources, the queue must be linked to
> +	 * the matrix_mdev to which its APQN is assigned and the KVM pointer
> +	 * must be available.
> +	 */
> +	q = vfio_ap_find_queue(apqn);
> +	if (q && q->matrix_mdev && q->matrix_mdev->kvm)

Is this of the type "we know there are no aqic resources to be freed" if
precondition is false?

vfio_ap_free_aqic_resources() checks the matrix_mdev pointer but not the
kvm pointer. Could we just check the kvm pointer in
vfio_ap_free_aqic_resources()?

At the end of the series, is seeing q! indicating a bug, or is it
something we expect to see under certain circumstances?


> +		vfio_ap_free_aqic_resources(q);
> +
> +	return ret;
>  }
>  
>  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
> @@ -1189,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>  			 */
>  			if (ret)
>  				rc = ret;
> -			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
>  		}
>  	}
>  
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index f46dde56b464..0db6fb3d56d5 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -100,5 +100,4 @@ struct vfio_ap_queue {
>  #define VFIO_AP_ISC_INVALID 0xff
>  	unsigned char saved_isc;
>  };
> -struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
>  #endif /* _VFIO_AP_PRIVATE_H_ */


  reply	other threads:[~2021-01-11 16:33 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-23  1:15 [PATCH v13 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 01/15] s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset Tony Krowiak
2021-01-11 16:32   ` Halil Pasic [this message]
2021-01-13 17:06     ` Tony Krowiak
2021-01-13 21:21       ` Halil Pasic
2021-01-14  0:46         ` Tony Krowiak
2021-01-14  3:13           ` Halil Pasic
2020-12-23  1:15 ` [PATCH v13 03/15] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 04/15] s390/vfio-ap: use new AP bus interface to search for queue devices Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 05/15] s390/vfio-ap: manage link between queue struct and matrix mdev Tony Krowiak
2021-01-11 19:17   ` Halil Pasic
2021-01-13 21:41     ` Tony Krowiak
2021-01-14  2:50       ` Halil Pasic
2021-01-14 21:10         ` Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 06/15] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device Tony Krowiak
2021-01-11 20:40   ` Halil Pasic
2021-01-14 17:54     ` Tony Krowiak
2021-01-15  1:08       ` Halil Pasic
2021-01-15  1:44       ` Halil Pasic
2021-03-31 14:36         ` Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 07/15] s390/vfio-ap: introduce shadow APCB Tony Krowiak
2021-01-11 22:50   ` Halil Pasic
2021-01-14 21:35     ` Tony Krowiak
2020-12-23  1:15 ` [PATCH v13 08/15] s390/vfio-ap: sysfs attribute to display the guest's matrix Tony Krowiak
2021-01-11 22:58   ` Halil Pasic
2021-01-28 21:29     ` Tony Krowiak
2020-12-23  1:16 ` [PATCH v13 09/15] s390/vfio-ap: allow hot plug/unplug of AP resources using mdev device Tony Krowiak
2021-01-12  1:12   ` Halil Pasic
2021-01-12 17:55     ` Halil Pasic
2021-02-01 14:41       ` Tony Krowiak
2021-02-03 23:13       ` Tony Krowiak
2021-02-04  0:21         ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 10/15] s390/zcrypt: driver callback to indicate resource in use Tony Krowiak
2021-01-12 16:50   ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 11/15] s390/vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2021-01-12  1:20   ` Halil Pasic
2021-01-12 14:14     ` Matthew Rosato
2021-01-12 16:49       ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 12/15] s390/zcrypt: Notify driver on config changed and scan complete callbacks Tony Krowiak
2021-01-12 16:58   ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 13/15] s390/vfio-ap: handle host AP config change notification Tony Krowiak
2021-01-12 18:39   ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 14/15] s390/vfio-ap: handle AP bus scan completed notification Tony Krowiak
2021-01-12 18:44   ` Halil Pasic
2020-12-23  1:16 ` [PATCH v13 15/15] s390/vfio-ap: update docs to include dynamic config support Tony Krowiak
2021-01-06 15:16 ` [PATCH v13 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2021-01-07 14:41   ` Halil Pasic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210111173206.27808b79.pasic@linux.ibm.com \
    --to=pasic@linux.ibm.com \
    --cc=akrowiak@linux.ibm.com \
    --cc=alex.williamson@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=fiuczy@linux.ibm.com \
    --cc=frankja@linux.ibm.com \
    --cc=freude@linux.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mjrosato@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).