Re: [PATCH v2 3/5] vfio-ccw: add capabilities chain

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

On 01/25/2019 11:19 AM, Eric Farman wrote:
> 
> 
> On 01/21/2019 06:03 AM, Cornelia Huck wrote:
>> Allow to extend the regions used by vfio-ccw. The first user will be
>> handling of halt and clear subchannel.
>>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_ops.c     | 181 ++++++++++++++++++++++++----
>>   drivers/s390/cio/vfio_ccw_private.h |  38 ++++++
...snip...
>> diff --git a/drivers/s390/cio/vfio_ccw_private.h 
>> b/drivers/s390/cio/vfio_ccw_private.h
>> index e88237697f83..20e75f4f3695 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -3,9 +3,11 @@
>>    * Private stuff for vfio_ccw driver
>>    *
>>    * Copyright IBM Corp. 2017
>> + * Copyright Red Hat, Inc. 2019
>>    *
>>    * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>    *            Xiao Feng Ren <renxiaof@linux.vnet.ibm.com>
>> + *            Cornelia Huck <cohuck@redhat.com>
>>    */
>>   #ifndef _VFIO_CCW_PRIVATE_H_
>> @@ -19,6 +21,38 @@
>>   #include "css.h"
>>   #include "vfio_ccw_cp.h"
>> +#define VFIO_CCW_OFFSET_SHIFT   40
>> +#define VFIO_CCW_OFFSET_TO_INDEX(off)    (off >> VFIO_CCW_OFFSET_SHIFT)
>> +#define VFIO_CCW_INDEX_TO_OFFSET(index)    ((u64)(index) << 
>> VFIO_CCW_OFFSET_SHIFT)
>> +#define VFIO_CCW_OFFSET_MASK    (((u64)(1) << VFIO_CCW_OFFSET_SHIFT) 
>> - 1)
>> +
>> +/* capability chain handling similar to vfio-pci */
>> +struct vfio_ccw_private;
>> +struct vfio_ccw_region;
>> +
>> +struct vfio_ccw_regops {
>> +    size_t    (*read)(struct vfio_ccw_private *private, char __user 
>> *buf,
>> +            size_t count, loff_t *ppos);
>> +    size_t    (*write)(struct vfio_ccw_private *private,
>> +             const char __user *buf, size_t count, loff_t *ppos);

Oops.  Per my recommendation on v1, you change these to "ssize_t" in 
patch 5.  Might as well just do that here.

>> +    void    (*release)(struct vfio_ccw_private *private,
>> +               struct vfio_ccw_region *region);
>> +};
>> +
>> +struct vfio_ccw_region {
>> +    u32                type;
>> +    u32                subtype;
>> +    const struct vfio_ccw_regops    *ops;
>> +    void                *data;
>> +    size_t                size;
>> +    u32                flags;
>> +};
>> +
>> +int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
>> +                 unsigned int subtype,
>> +                 const struct vfio_ccw_regops *ops,
>> +                 size_t size, u32 flags, void *data);
>> +
>>   /**
>>    * struct vfio_ccw_private
>>    * @sch: pointer to the subchannel
...snip...
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 02bb7ad6e986..56e2413d3e00 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -353,6 +353,8 @@ struct vfio_region_gfx_edid {
>>   #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
>>   };
>> +#define VFIO_REGION_TYPE_CCW            (2)
>> +
> 
> Cool.  :)
> 
>>   /*
>>    * 10de vendor sub-type
>>    *
>>
> 
> Looks fine to me.  I'd love to think there was a way to generalize this 
> for other vfio drivers, but man that's a tall task.  So...

With the ssize_t fixup from patch 5...

> 
> Reviewed-by: Eric Farman <farman@linux.ibm.com>