Re: [PATCH v3 8/8] s390/vfio_ccw: implement a crw lock

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 7/23/26 1:47 PM, Eric Farman wrote:
> 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: stable@vger.kernel.org
> Cc: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     | 17 +++++++++++------
>  drivers/s390/cio/vfio_ccw_drv.c     |  4 ++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  3 +++
>  drivers/s390/cio/vfio_ccw_private.h |  2 ++
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index bf678270c457..3d05aaf790fa 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -93,15 +93,19 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
> +	i = array_index_nospec(i, private->num_regions);

Same comment as patch 6 wrt to array_index_nospec

...

> index ac5aaa78a74b..2562679b5ef5 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -121,6 +121,8 @@ struct vfio_ccw_private {
>  
>  	struct irb		irb;
>  	union scsw		scsw;
> +
> +	spinlock_t		crw_lock;

This should have a comment either here or, in keeping with the rest of
the struct, in the comment block above.

This looks good if you're trying to protect private->crw list, I don't
think it's complete if you're also trying to protect
private->crw_trigger - it looks to me like you hold the new lock over
most accesses to crw_trigger but at least not vfio_ccw_mdev_set_irqs
where you set/clear it.


>  	struct list_head	crw;
>  
>  	struct eventfd_ctx	*io_trigger;