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

[Eric Farman]

On 7/24/26 4:44 PM, Matthew Rosato wrote:
> 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@xxxxxxxxxxxxxxx
> > Cc: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> > ---
> >   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.

Thanks, missed that. The crw field itself was never added to the comment 
block, so I'll add that as well.

Similarly, I didn't add definitions for the new fields in struct 
channel_program earlier in this series, so I've gone back and done those.

> 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.

I suspect you're right that crw_trigger needs some love and be moved 
under this umbrella, but I'm drawing the line at private->crw for the now.

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