From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [PATCH v7 2/4] vfio: VFIO driver for mediated devices Date: Tue, 13 Sep 2016 10:35:11 +0800 Message-ID: <57D765DF.7050801@intel.com> References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-3-git-send-email-kwankhede@nvidia.com> <20160825172226.2cd06d71@oc7835276234> <57D0D0C2.6040201@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com To: Dong Jia Return-path: Received: from mga11.intel.com ([192.55.52.93]:63578 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752105AbcIMChH (ORCPT ); Mon, 12 Sep 2016 22:37:07 -0400 In-Reply-To: <57D0D0C2.6040201@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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 >> From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjday-0001tS-Sy for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:37:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjdat-0005iO-C1 for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:37:11 -0400 Received: from mga11.intel.com ([192.55.52.93]:3390) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjdat-0005i3-1j for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:37:07 -0400 Message-ID: <57D765DF.7050801@intel.com> Date: Tue, 13 Sep 2016 10:35:11 +0800 From: Jike Song MIME-Version: 1.0 References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-3-git-send-email-kwankhede@nvidia.com> <20160825172226.2cd06d71@oc7835276234> <57D0D0C2.6040201@intel.com> In-Reply-To: <57D0D0C2.6040201@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 2/4] vfio: VFIO driver for mediated devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Jia Cc: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@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 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 >>