Re: [PATCH RFC UNTESTED] vfio-ccw: indirect access to translated cps

From: Halil Pasic <pasic@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: Eric Farman <farman@linux.ibm.com>,
	linux-s390@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH RFC UNTESTED] vfio-ccw: indirect access to translated cps
Date: Tue, 30 Jul 2019 17:49:10 +0200	[thread overview]
Message-ID: <20190730174910.47930494.pasic@linux.ibm.com> (raw)
In-Reply-To: <20190726100617.19718-1-cohuck@redhat.com>

On Fri, 26 Jul 2019 12:06:17 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> We're currently keeping a single area for translated channel
> programs in our private structure, which is filled out when
> we are translating a channel program we have been given by
> user space and marked invalid again when we received an final
> interrupt for that I/O.
> 
> Unfortunately, properly tracking the lifetime of that cp is
> not easy: failures may happen during translation or right when
> it is sent to the hardware, unsolicited interrupts may trigger
> a deferred condition code, a halt/clear request may be issued
> while the I/O is supposed to be running, or a reset request may
> come in from the side. The _PROCESSING state and the ->initialized
> flag help a bit, but not enough.
> 
> We want to have a way to figure out whether we actually have a cp
> currently in progress, so we can update/free only when applicable.
> Points to keep in mind:
> - We will get an interrupt after a cp has been submitted iff ssch
>   finished with cc 0.
> - We will get more interrupts for a cp if the interrupt status is
>   not final.
> - We can have only one cp in flight at a time.
> 
> Let's decouple the actual area in the private structure from the
> means to access it: Only after we have successfully submitted a
> cp (ssch with cc 0), update the pointer in the private structure
> to point to the area used. Therefore, the interrupt handler won't
> access the cp if we don't actually expect an interrupt pertaining
> to it.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> Just hacked this up to get some feedback, did not actually try it
> out. Not even sure if this is a sensible approach; if not, let's
> blame it on the heat and pretend it didn't happen :)
> 

Do not multiple threads access this new cp pointer (and at least one of
them writes)? If that is the case, it smells like a data race to me.

Besides the only point of converting cp to a pointer seems to be
policing access to cp_area (which used to be cp). I.e. if it is
NULL: don't touch it, otherwise: go ahead. We can do that with a single
bit, we don't need a pointer for that.

Could we convert initialized into some sort of cp.status that
tracks/controls access and responsibilities? By working with bits we
could benefit from the atomicity of bit-ops -- if I'm not wrong.

> I also thought about having *two* translation areas and switching
> the pointer between them; this might be too complicated, though?

We only have one channel program at a time or? I can't see the benefit
of having two areas.

Sorry I didn't intend to open a huge discussion, as I'm on vacation
starting Thursday -- means expect delays. If the rest of the bunch
happens to see this differently, please feel free to not seek my consent.

Regards,
Halil

> 
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 19 +++++++++++--------
>  drivers/s390/cio/vfio_ccw_fsm.c     | 25 +++++++++++++++++--------
>  drivers/s390/cio/vfio_ccw_ops.c     | 11 +++++++----
>  drivers/s390/cio/vfio_ccw_private.h |  6 ++++--
>  4 files changed, 39 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 9208c0e56c33..059b88c94378 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -86,10 +86,13 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	is_final = !(scsw_actl(&irb->scsw) &
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
> -	if (scsw_is_solicited(&irb->scsw)) {
> -		cp_update_scsw(&private->cp, &irb->scsw);
> -		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
> -			cp_free(&private->cp);
> +	if (scsw_is_solicited(&irb->scsw) && private->cp) {
> +		cp_update_scsw(private->cp, &irb->scsw);
> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
> +			struct channel_program *cp = private->cp;
> +			private->cp = NULL;
> +			cp_free(cp);
> +		}
>  	}
>  	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
> @@ -129,9 +132,9 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>  	if (!private)
>  		return -ENOMEM;
>  
> -	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
> +	private->cp_area.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
>  				       GFP_KERNEL);
> -	if (!private->cp.guest_cp)
> +	if (!private->cp_area.guest_cp)
>  		goto out_free;
>  
>  	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
> @@ -174,7 +177,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>  		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
>  	if (private->io_region)
>  		kmem_cache_free(vfio_ccw_io_region, private->io_region);
> -	kfree(private->cp.guest_cp);
> +	kfree(private->cp_area.guest_cp);
>  	kfree(private);
>  	return ret;
>  }
> @@ -191,7 +194,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
>  
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
>  	kmem_cache_free(vfio_ccw_io_region, private->io_region);
> -	kfree(private->cp.guest_cp);
> +	kfree(private->cp_area.guest_cp);
>  	kfree(private);
>  
>  	return 0;
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 49d9d3da0282..543d007ddc46 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -18,7 +18,8 @@
>  #define CREATE_TRACE_POINTS
>  #include "vfio_ccw_trace.h"
>  
> -static int fsm_io_helper(struct vfio_ccw_private *private)
> +static int fsm_io_helper(struct vfio_ccw_private *private,
> +			 struct channel_program *cp)
>  {
>  	struct subchannel *sch;
>  	union orb *orb;
> @@ -31,7 +32,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>  
>  	spin_lock_irqsave(sch->lock, flags);
>  
> -	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
> +	orb = cp_get_orb(cp, (u32)(addr_t)sch, sch->lpm);
>  	if (!orb) {
>  		ret = -EIO;
>  		goto out;
> @@ -47,6 +48,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>  		 */
>  		sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
>  		ret = 0;
> +		private->cp = cp;
>  		private->state = VFIO_CCW_STATE_CP_PENDING;
>  		break;
>  	case 1:		/* Status pending */
> @@ -236,31 +238,38 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
>  		orb = (union orb *)io_region->orb_area;
>  
> +		/* I/O already in progress? Should not happen (bug in FSM?). */
> +		if (private->cp) {
> +			io_region->ret_code = -EBUSY;
> +			errstr = "cp in progress";
> +			goto err_out;
> +		}
>  		/* Don't try to build a cp if transport mode is specified. */
>  		if (orb->tm.b) {
>  			io_region->ret_code = -EOPNOTSUPP;
>  			errstr = "transport mode";
>  			goto err_out;
>  		}
> -		io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev),
> -					      orb);
> +		io_region->ret_code = cp_init(&private->cp_area,
> +					      mdev_dev(mdev), orb);
>  		if (io_region->ret_code) {
>  			errstr = "cp init";
>  			goto err_out;
>  		}
>  
> -		io_region->ret_code = cp_prefetch(&private->cp);
> +		io_region->ret_code = cp_prefetch(&private->cp_area);
>  		if (io_region->ret_code) {
>  			errstr = "cp prefetch";
> -			cp_free(&private->cp);
> +			cp_free(&private->cp_area);
>  			goto err_out;
>  		}
>  
>  		/* Start channel program and wait for I/O interrupt. */
> -		io_region->ret_code = fsm_io_helper(private);
> +		io_region->ret_code = fsm_io_helper(private,
> +						    &private->cp_area);
>  		if (io_region->ret_code) {
>  			errstr = "cp fsm_io_helper";
> -			cp_free(&private->cp);
> +			cp_free(&private->cp_area);
>  			goto err_out;
>  		}
>  		return;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 5eb61116ca6f..5ad6a7b672bd 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -58,13 +58,14 @@ static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
>  	if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) {
>  		struct vfio_iommu_type1_dma_unmap *unmap = data;
>  
> -		if (!cp_iova_pinned(&private->cp, unmap->iova))
> +		if (!cp_iova_pinned(&private->cp_area, unmap->iova))
>  			return NOTIFY_OK;
>  
>  		if (vfio_ccw_mdev_reset(private->mdev))
>  			return NOTIFY_BAD;
>  
> -		cp_free(&private->cp);
> +		private->cp = NULL;
> +		cp_free(&private->cp_area);
>  		return NOTIFY_OK;
>  	}
>  
> @@ -139,7 +140,8 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
>  		/* The state will be NOT_OPER on error. */
>  	}
>  
> -	cp_free(&private->cp);
> +	private->cp = NULL;
> +	cp_free(&private->cp_area);
>  	private->mdev = NULL;
>  	atomic_inc(&private->avail);
>  
> @@ -180,7 +182,8 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
>  		/* The state will be NOT_OPER on error. */
>  	}
>  
> -	cp_free(&private->cp);
> +	private->cp = NULL;
> +	cp_free(&private->cp_area);
>  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>  				 &private->nb);
>  
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index f1092c3dc1b1..e792a20202c3 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -68,7 +68,8 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
>   * @region: additional regions for other subchannel operations
>   * @cmd_region: MMIO region for asynchronous I/O commands other than START
>   * @num_regions: number of additional regions
> - * @cp: channel program for the current I/O operation
> + * @cp_area: channel program memory area
> + * @cp: pointer to channel program for the current I/O operation
>   * @irb: irb info received from interrupt
>   * @scsw: scsw info
>   * @io_trigger: eventfd ctx for signaling userspace I/O results
> @@ -87,7 +88,8 @@ struct vfio_ccw_private {
>  	struct ccw_cmd_region	*cmd_region;
>  	int num_regions;
>  
> -	struct channel_program	cp;
> +	struct channel_program cp_area;
> +	struct channel_program	*cp;
>  	struct irb		irb;
>  	union scsw		scsw;
>  