Re: [PATCH 5.10.y] s390/vfio_ccw: Implement a crw lock

On Mon, Aug 24, 2026 at 12:26:15PM -0400, Sasha Levin wrote:
> From: Eric Farman <[email protected]>
> 
> [ Upstream commit 16b0798024c0e9117e395829ddbbe70981c79d9c ]
> 
> Unlike the channel_program struct, which covers synchronous I/O
> submissions and asynchronous interrupts, the CRW region relies
> exclusively on asynchronous events coming from hardware.
> 
> Implement a lock to manage the list of those payloads, to ensure
> they are read cohesively.
> 
> Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
> Cc: [email protected]
> Reviewed-by: Matthew Rosato <[email protected]>
> Reviewed-by: Farhan Ali <[email protected]>
> Signed-off-by: Eric Farman <[email protected]>
> Signed-off-by: Christian Borntraeger <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     | 16 +++++++++++-----
>  drivers/s390/cio/vfio_ccw_drv.c     | 10 ++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 13b26a1c79886..4fc493584a1e2 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -84,19 +84,24 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
>  	struct ccw_crw_region *region;
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
>  	int ret;
>  
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
> +	region = private->region[i].data;
> +
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	crw = list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
>  
>  	if (crw)
>  		list_del(&crw->next);
>  
> -	mutex_lock(&private->io_mutex);
> -	region = private->region[i].data;
> +	/* Drop CRW lock while copying to userspace */
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  
>  	if (crw)
>  		memcpy(&region->crw, &crw->crw, sizeof(region->crw));
> @@ -107,14 +112,15 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		ret = count;
>  
>  	region->crw = 0;
> -
> -	mutex_unlock(&private->io_mutex);
> -
>  	kfree(crw);
>  
>  	/* Notify the guest if more CRWs are on our queue */
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger, 1);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
> +
> +	mutex_unlock(&private->io_mutex);
>  
>  	return ret;
>  }
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index e3c1060b6056c..83993a1423d00 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -119,11 +119,14 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  static void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger, 1);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  }
>  
>  /*
> @@ -207,6 +210,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>  	INIT_LIST_HEAD(&private->crw);
>  	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
>  	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> +	spin_lock_init(&private->crw_lock);
>  	atomic_set(&private->avail, 1);
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  
> @@ -238,13 +242,16 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
>  {
>  	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>  	struct vfio_ccw_crw *crw, *temp;
> +	unsigned long flags;
>  
>  	vfio_ccw_sch_quiesce(sch);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  
>  	vfio_ccw_mdev_unreg(sch);
>  
> @@ -305,6 +312,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  			       unsigned int rsid)
>  {
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
>  
>  	/*
>  	 * If unable to allocate a CRW, just drop the event and
> @@ -322,7 +330,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_add_tail(&crw->next, &private->crw);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }
>  
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 8723156b29eac..38f470c2d26c3 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -83,6 +83,8 @@ struct vfio_ccw_crw {
>   * @cp: channel program for the current I/O operation
>   * @irb: irb info received from interrupt
>   * @scsw: scsw info
> + * @crw_lock: serialization of CRW list information
> + * @crw: list of Channel Report Word elements
>   * @io_trigger: eventfd ctx for signaling userspace I/O results
>   * @io_work: work for deferral process of I/O handling
>   */
> @@ -104,6 +106,8 @@ struct vfio_ccw_private {
>  	struct channel_program	cp;
>  	struct irb		irb;
>  	union scsw		scsw;
> +
> +	spinlock_t		crw_lock;
>  	struct list_head	crw;
>  
>  	struct eventfd_ctx	*io_trigger;
> -- 
> 2.53.0
> 
> 

Does not apply to the queue :(