Re: [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 17 Apr 2020 04:29:58 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> This region provides a mechanism to pass Channel Report Word(s)
> that affect vfio-ccw devices, and need to be passed to the guest
> for its awareness and/or processing.
> 
> The base driver (see crw_collect_info()) provides space for two
> CRWs, as a subchannel event may have two CRWs chained together
> (one for the ssid, one for the subcahnnel).  All other CRWs will
> only occupy the first one.  Even though this support will also
> only utilize the first one, we'll provide space for two also.

This is no longer true?

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v2->v3:
>      - Remove "if list-empty" check, since there's no list yet [EF]
>      - Reduce the CRW region to one fullword, instead of two [CH]
>     
>     v1->v2:
>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>      - Add a block comment to struct ccw_crw_region [CH]
>     
>     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
> 
>  Documentation/s390/vfio-ccw.rst     | 16 +++++++++
>  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       |  8 +++++
>  7 files changed, 105 insertions(+)
> 
> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> index 98832d95f395..3338551ef642 100644
> --- a/Documentation/s390/vfio-ccw.rst
> +++ b/Documentation/s390/vfio-ccw.rst
> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
>  Reading this region triggers a STORE SUBCHANNEL to be issued to the
>  associated hardware.
>  
> +vfio-ccw crw region
> +---------------------
> +
> +The vfio-ccw crw region is used to return Channel Report Word (CRW)
> +data to userspace::
> +
> +  struct ccw_crw_region {
> +         __u32 crw;
> +  } __packed;
> +
> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
> +
> +Currently, space is provided for a single CRW. Handling of chained
> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
> +the region for additional CRW data.

What about the following instead:

"Reading this region returns a CRW if one that is relevant for this
subchannel (e.g. one reporting changes in channel path state) is
pending, or all zeroes if not. If multiple CRWs are pending (including
possibly chained CRWs), reading this region again will return the next
one, until no more CRWs are pending and zeroes are returned. This is
similar to how STORE CHANNEL REPORT WORD works."

> +
>  vfio-ccw operation details
>  --------------------------
>  
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 18f3b3e873a9..c1362aae61f5 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -74,3 +74,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;

I see that you implemented it a bit differently in patch 7, but I think
resetting crw to 0 immediately after the copy_to_user() is cleaner. It
also can be done in this patch already :)

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

(...)

> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> index 758bf214898d..faf57e691d4a 100644
> --- a/include/uapi/linux/vfio_ccw.h
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -44,4 +44,12 @@ struct ccw_schib_region {
>  	__u8 schib_area[SCHIB_AREA_SIZE];
>  } __packed;
>  
> +/*
> + * Used for returning Channel Report Word(s) to userspace.

s/Channel Report Word(s)/a Channel Report Word/ ?

> + * Note: this is controlled by a capability
> + */
> +struct ccw_crw_region {
> +	__u32 crw;
> +} __packed;
> +
>  #endif