From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dong Jia Subject: Re: [PATCH v7 2/4] vfio: VFIO driver for mediated devices Date: Thu, 25 Aug 2016 17:22:26 +0800 Message-ID: <20160825172226.2cd06d71@oc7835276234> References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-3-git-send-email-kwankhede@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: , , , , , , , , Dong Jia To: Kirti Wankhede Return-path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:54237 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752142AbcHYJWi (ORCPT ); Thu, 25 Aug 2016 05:22:38 -0400 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u7P9J6S6118549 for ; Thu, 25 Aug 2016 05:22:37 -0400 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0b-001b2d01.pphosted.com with ESMTP id 2512h8qxtp-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 25 Aug 2016 05:22:37 -0400 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Aug 2016 03:22:36 -0600 In-Reply-To: <1472097235-6332-3-git-send-email-kwankhede@nvidia.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, 25 Aug 2016 09:23:53 +0530 Kirti Wankhede 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