Re: [Qemu-devel] [PATCH v7 2/4] vfio: VFIO driver for mediated devices

[Dong Jia]

On Thu, 25 Aug 2016 09:23:53 +0530
Kirti Wankhede <address@hidden> wrote:

[...]

Dear Kirti,

I just rebased my vfio-ccw patches to this series.
With a little fix, which was pointed it out in my reply to the #3
patch, it works fine.

> +static long vfio_mdev_unlocked_ioctl(void *device_data,
> +                                  unsigned int cmd, unsigned long arg)
> +{
> +     int ret = 0;
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +     unsigned long minsz;
> +
> +     switch (cmd) {
> +     case VFIO_DEVICE_GET_INFO:
> +     {
> +             struct vfio_device_info info;
> +
> +             minsz = offsetofend(struct vfio_device_info, num_irqs);
> +
> +             if (copy_from_user(&info, (void __user *)arg, minsz))
> +                     return -EFAULT;
> +
> +             if (info.argsz < minsz)
> +                     return -EINVAL;
> +
> +             if (parent->ops->get_device_info)
> +                     ret = parent->ops->get_device_info(vmdev->mdev, &info);
> +             else
> +                     return -EINVAL;
> +
> +             if (ret)
> +                     return ret;
> +
> +             if (parent->ops->reset)
> +                     info.flags |= VFIO_DEVICE_FLAGS_RESET;
Shouldn't this be done inside the get_device_info callback?

> +
> +             memcpy(&vmdev->dev_info, &info, sizeof(info));
> +
> +             return copy_to_user((void __user *)arg, &info, minsz);
> +     }
[...]

> +
> +static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> +                           size_t count, loff_t *ppos)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct mdev_device *mdev = vmdev->mdev;
> +     struct parent_device *parent = mdev->parent;
> +     unsigned int done = 0;
> +     int ret;
> +
> +     if (!parent->ops->read)
> +             return -EINVAL;
> +
> +     while (count) {
Here, I have to say sorry to you guys for that I didn't notice the
bad impact of this change to my patches during the v6 discussion.

For vfio-ccw, I introduced an I/O region to input/output I/O
instruction parameters and results for Qemu. The @count of these data
currently is 140. So supporting arbitrary lengths in one shot here, and
also in vfio_mdev_write, seems the better option for this case.

I believe that if the pci drivers want to iterate in a 4 bytes step, you
can do that in the parent read/write callbacks instead.

What do you think?

> +             size_t filled;
> +
> +             if (count >= 4 && !(*ppos % 4)) {
> +                     u32 val;
> +
> +                     ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
> +                                             *ppos);
> +                     if (ret <= 0)
> +                             goto read_err;
> +
> +                     if (copy_to_user(buf, &val, sizeof(val)))
> +                             goto read_err;
> +
> +                     filled = 4;
> +             } else if (count >= 2 && !(*ppos % 2)) {
> +                     u16 val;
> +
> +                     ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
> +                                             *ppos);
> +                     if (ret <= 0)
> +                             goto read_err;
> +
> +                     if (copy_to_user(buf, &val, sizeof(val)))
> +                             goto read_err;
> +
> +                     filled = 2;
> +             } else {
> +                     u8 val;
> +
> +                     ret = parent->ops->read(mdev, &val, sizeof(val), *ppos);
> +                     if (ret <= 0)
> +                             goto read_err;
> +
> +                     if (copy_to_user(buf, &val, sizeof(val)))
> +                             goto read_err;
> +
> +                     filled = 1;
> +             }
> +
> +             count -= filled;
> +             done += filled;
> +             *ppos += filled;
> +             buf += filled;
> +     }
> +
> +     return done;
> +
> +read_err:
> +     return -EFAULT;
> +}
[...]

--------
Dong Jia