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

[Jike Song <jike.song@intel.com>]

On 09/08/2016 10:45 AM, Jike Song wrote:
> On 08/25/2016 05:22 PM, Dong Jia wrote:
>> On Thu, 25 Aug 2016 09:23:53 +0530
>> Kirti Wankhede <kwankhede@nvidia.com> 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.
>>
> 
> Hi Jia,
> 
> Sorry I didn't follow a lot in previous discussion, but since
> vfio-mdev in v7 patchset is at least PCI-agnostic, would you share
> with us why you still need a vfio-ccw?

Kind ping :)


Hi Dong Jia,

Since Kirti has confirmed that in v7 it is designed to have only one
vfio-mdev driver for all mdev devices, would you please tell us the
reason of your vfio-ccw? It could possibly be an architectural gap and
the earlier we discuss it the better :)

--
Thanks,
Jike

>>> +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
>>