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

[Eric Farman]

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 <address@hidden>
> > ---
> >   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 <address@hidden>
> >    *            Xiao Feng Ren <address@hidden>
> > + *            Cornelia Huck <address@hidden>
> >    */
> >   #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 <address@hidden>