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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
> 



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux