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

[Eric Farman <farman@linux.ibm.com>]

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

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