Re: [RFC PATCH v1 06/10] vfio-ccw: Introduce a new CRW region

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 15 Nov 2019 03:56:16 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> This region can be used by userspace to get channel report
> words from vfio-ccw driver.

I think this needs a bit more explanation; this is for channel report
words concerning vfio-ccw devices that are supposed to be relayed to
the guest, IIUC?

> 
> We provide space for two CRWs, per the limit in the
> base driver (see crw_collect_info()).

That rationale seems a bit sketchy.

As far as I know, current systems provide at most two crws chained
together for an event (one for the ssid + one for the subchannel id in
case of a subchannel event, one crw for other events); and that's the
reason why we provide space for two crws (unless there's something
upcoming which would need three crws chained together?)

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Clean up checkpatch (whitespace) errors
>      - Add ret=-ENOMEM in error path for new region
>      - Add io_mutex for region read (originally in last patch)
>      - Change crw1/crw2 to crw0/crw1
>      - Reorder cleanup of regions
> 
>  drivers/s390/cio/vfio_ccw_chp.c     | 53 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  include/uapi/linux/vfio.h           |  1 +
>  include/uapi/linux/vfio_ccw.h       |  5 +++
>  6 files changed, 86 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 826d08379fe3..d1e8bfef06be 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -73,3 +73,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
>  					    VFIO_REGION_INFO_FLAG_READ,
>  					    private->schib_region);
>  }
> +
> +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
> +					char __user *buf, size_t count,
> +					loff_t *ppos)
> +{
> +	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
> +	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
> +	struct ccw_crw_region *region;
> +	int ret;
> +
> +	if (pos + count > sizeof(*region))
> +		return -EINVAL;
> +
> +	mutex_lock(&private->io_mutex);
> +	region = private->region[i].data;
> +
> +	if (copy_to_user(buf, (void *)region + pos, count))
> +		ret = -EFAULT;
> +	else
> +		ret = count;
> +

Can userspace read the same crw(s) multiple times? How can it find out
if there's something new in there?

> +	mutex_unlock(&private->io_mutex);
> +	return ret;
> +}
> +

(...)

> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> index 7c0a834e5d7a..88b125aad279 100644
> --- a/include/uapi/linux/vfio_ccw.h
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -39,4 +39,9 @@ struct ccw_schib_region {
>  	__u8 schib_area[SCHIB_AREA_SIZE];
>  } __packed;
>

I think this one wants an explaining comment as well.
  
> +struct ccw_crw_region {
> +	__u32 crw0;
> +	__u32 crw1;
> +} __packed;
> +
>  #endif