Re: [Qemu-devel] [PATCH RFC v3 07/15] vfio: ccw: introduce ccw_io_region

[Alex Williamson]

On Fri, 17 Feb 2017 09:29:31 +0100
Dong Jia Shi <address@hidden> wrote:

> To provide user-space a set of interfaces to:
> 1. pass in a ccw program to perform an I/O operation.
> 2. read back I/O results of the completed I/O operations.
> We introduce an MMIO region for the vfio-ccw device here.
> 
> This region is defined to content:
> 1. areas to store arguments that an ssch required.
> 2. areas to store the I/O results.
> 
> Using pwrite/pread to the device on this region, a user-space program
> could write/read data to/from the vfio-ccw device.
> 
> Signed-off-by: Dong Jia Shi <address@hidden>
> Reviewed-by: Pierre Morel <address@hidden>
> ---
>  arch/s390/include/uapi/asm/Kbuild   |  1 +
>  drivers/s390/cio/vfio_ccw_ops.c     | 47 
> +++++++++++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
>  include/uapi/linux/vfio_ccw.h       | 24 +++++++++++++++++++
>  4 files changed, 76 insertions(+)
>  create mode 100644 include/uapi/linux/vfio_ccw.h
> 
> diff --git a/arch/s390/include/uapi/asm/Kbuild 
> b/arch/s390/include/uapi/asm/Kbuild
> index bf736e7..fdb9529 100644
> --- a/arch/s390/include/uapi/asm/Kbuild
> +++ b/arch/s390/include/uapi/asm/Kbuild
> @@ -52,3 +52,4 @@ header-y += unistd.h
>  header-y += virtio-ccw.h
>  header-y += vtoc.h
>  header-y += zcrypt.h
> +header-y += vfio_ccw.h
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index b8a2fed..6c06805 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -127,6 +127,51 @@ void vfio_ccw_mdev_release(struct mdev_device *mdev)
>                                &private->nb);
>  }
>  
> +static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,
> +                               char __user *buf,
> +                               size_t count,
> +                               loff_t *ppos)
> +{
> +     struct vfio_ccw_private *private;
> +     struct ccw_io_region *region;
> +
> +     if (*ppos + count > sizeof(*region))
> +             return -EINVAL;

Couldn't this wrap-around and still satisfy this test?

> +
> +     private = dev_get_drvdata(mdev_parent_dev(mdev));
> +     if (!private)
> +             return -ENODEV;
> +
> +     region = &private->io_region;
> +     if (copy_to_user(buf, (void *)region + *ppos, count))
> +             return -EFAULT;
> +
> +     return count;
> +}
> +
> +static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> +                                const char __user *buf,
> +                                size_t count,
> +                                loff_t *ppos)
> +{
> +     struct vfio_ccw_private *private;
> +     struct ccw_io_region *region;
> +
> +     if (*ppos + count > sizeof(*region))
> +             return -EINVAL;

Same here

> +
> +     private = dev_get_drvdata(mdev_parent_dev(mdev));
> +     if (!private)
> +             return -ENODEV;
> +
> +     region = &private->io_region;
> +     if (copy_from_user((void *)region + *ppos, buf, count))
> +             return -EFAULT;
> +     region->ret_code = 0;
> +
> +     return count;
> +}
> +
>  static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>       .owner                  = THIS_MODULE,
>       .supported_type_groups  = mdev_type_groups,
> @@ -134,6 +179,8 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>       .remove                 = vfio_ccw_mdev_remove,
>       .open                   = vfio_ccw_mdev_open,
>       .release                = vfio_ccw_mdev_release,
> +     .read                   = vfio_ccw_mdev_read,
> +     .write                  = vfio_ccw_mdev_write,
>  };
>  
>  int vfio_ccw_mdev_reg(struct subchannel *sch)
> diff --git a/drivers/s390/cio/vfio_ccw_private.h 
> b/drivers/s390/cio/vfio_ccw_private.h
> index 5afb3ba..359e96b 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -10,6 +10,8 @@
>  #ifndef _VFIO_CCW_PRIVATE_H_
>  #define _VFIO_CCW_PRIVATE_H_
>  
> +#include <linux/vfio_ccw.h>
> +
>  #include "css.h"
>  
>  /**
> @@ -19,6 +21,7 @@
>   * @avail: available for creating a mediated device
>   * @mdev: pointer to the mediated device
>   * @nb: notifier for vfio events
> + * @io_region: MMIO region to input/output I/O arguments/results
>   */
>  struct vfio_ccw_private {
>       struct subchannel       *sch;
> @@ -26,6 +29,7 @@ struct vfio_ccw_private {
>       atomic_t                avail;
>       struct mdev_device      *mdev;
>       struct notifier_block   nb;
> +     struct ccw_io_region    io_region;
>  } __aligned(8);
>  
>  extern int vfio_ccw_mdev_reg(struct subchannel *sch);
> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> new file mode 100644
> index 0000000..34a7f6f
> --- /dev/null
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -0,0 +1,24 @@
> +/*
> + * Interfaces for vfio-ccw
> + *
> + * Copyright IBM Corp. 2017
> + *
> + * Author(s): Dong Jia Shi <address@hidden>
> + */
> +
> +#ifndef _VFIO_CCW_H_
> +#define _VFIO_CCW_H_
> +
> +#include <linux/types.h>
> +
> +struct ccw_io_region {
> +#define ORB_AREA_SIZE 12
> +     __u8    orb_area[ORB_AREA_SIZE];
> +#define SCSW_AREA_SIZE 12
> +     __u8    scsw_area[SCSW_AREA_SIZE];
> +#define IRB_AREA_SIZE 96
> +     __u8    irb_area[IRB_AREA_SIZE];
> +     __u32   ret_code;
> +} __packed;
> +
> +#endif