On 4/21/20 5:41 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:29:58 +0200
> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
>
>> From: Farhan Ali <alifm@xxxxxxxxxxxxx>
>>
>> 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
s/subcahnnel/subchannel/
>> 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?
Oops, yes.
>
>>
>> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
>> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
>> ---
>>
>> 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."
Sounds good to me.
Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or
just rely on the notification from the host to trigger the read?
>
>> +
>> 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 :)
Ha, yes. I actually had it set that way, but didn't like the result in
patch 7. I'll revisit that.
>
>> +
>> + 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/ ?
Nod.
>
>> + * Note: this is controlled by a capability
>> + */
>> +struct ccw_crw_region {
>> + __u32 crw;
>> +} __packed;
>> +
>> #endif
>
