On Thu, 25 Aug 2016 09:23:53 +0530
Kirti Wankhede <kwankhede@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
