From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dong Jia Subject: Re: [RFC PATCH v4 1/3] Mediated device Core driver Date: Mon, 6 Jun 2016 16:29:11 +0800 Message-ID: <20160606162911.7021a02c@oc7835276234> References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> <20160603165746.3df39542@oc7835276234> <799479b0-44d5-061f-185a-43df0dba2fb3@nvidia.com> <20160606140148.31ec05f9@oc7835276234> <20160606062742.GA31747@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Kirti Wankhede , , , , , , , , , , Dong Jia To: Neo Jia Return-path: Received: from e34.co.us.ibm.com ([32.97.110.152]:59665 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751112AbcFFI33 (ORCPT ); Mon, 6 Jun 2016 04:29:29 -0400 Received: from localhost by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 Jun 2016 02:29:27 -0600 In-Reply-To: <20160606062742.GA31747@nvidia.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, 5 Jun 2016 23:27:42 -0700 Neo Jia wrote: > On Mon, Jun 06, 2016 at 02:01:48PM +0800, Dong Jia wrote: > > On Mon, 6 Jun 2016 10:57:49 +0530 > > Kirti Wankhede wrote: > > > > > > > > > > > On 6/3/2016 2:27 PM, Dong Jia wrote: > > > > On Wed, 25 May 2016 01:28:15 +0530 > > > > Kirti Wankhede wrote: > > > > > > > > > > > > ...snip... > > > > > > > >> +struct phy_device_ops { > > > >> + struct module *owner; > > > >> + const struct attribute_group **dev_attr_groups; > > > >> + const struct attribute_group **mdev_attr_groups; > > > >> + > > > >> + int (*supported_config)(struct device *dev, char *config); > > > >> + int (*create)(struct device *dev, uuid_le uuid, > > > >> + uint32_t instance, char *mdev_params); > > > >> + int (*destroy)(struct device *dev, uuid_le uuid, > > > >> + uint32_t instance); > > > >> + int (*start)(uuid_le uuid); > > > >> + int (*shutdown)(uuid_le uuid); > > > >> + ssize_t (*read)(struct mdev_device *vdev, char *buf, size_t count, > > > >> + enum mdev_emul_space address_space, loff_t pos); > > > >> + ssize_t (*write)(struct mdev_device *vdev, char *buf, size_t count, > > > >> + enum mdev_emul_space address_space, loff_t pos); > > > >> + int (*set_irqs)(struct mdev_device *vdev, uint32_t flags, > > > >> + unsigned int index, unsigned int start, > > > >> + unsigned int count, void *data); > > > >> + int (*get_region_info)(struct mdev_device *vdev, int region_index, > > > >> + struct pci_region_info *region_info); > > > >> + int (*validate_map_request)(struct mdev_device *vdev, > > > >> + unsigned long virtaddr, > > > >> + unsigned long *pfn, unsigned long *size, > > > >> + pgprot_t *prot); > > > >> +}; > > > > > > > > Dear Kirti: > > > > > > > > When I rebased my vfio-ccw patches on this series, I found I need an > > > > extra 'ioctl' callback in phy_device_ops. > > > > > > > > > > Thanks for taking closer look. As per my knowledge ccw is not PCI > > > device, right? Correct me if I'm wrong. > > Dear Kirti: > > > > You are right. CCW is different to PCI. The official term is 'Channel > > I/O device'. They use 'Channels' (co-processors) and CCWs (channel > > command words) to handle I/O operations. > > > > > I'm curious to know. Are you planning to write a driver (vfio-mccw) for > > > mediated ccw device? > > I wrote two drivers: > > 1. A vfio-pccw driver for the physical ccw device, which will reigister > > the device and callbacks to mdev framework. With this, I could create > > a mediated ccw device for the physical one then. > > 2. A vfio-mccw driver for the mediated ccw device, which will add > > itself to a vfio_group, mimiced what vfio-mpci did. > > > > The problem is, vfio-mccw need to implement new ioctls besides the > > existing ones (VFIO_DEVICE_GET_INFO, etc). And these ioctls really need > > the physical device help to handle. > > Hi Dong, > > Could you please help us understand a bit more about the new VFIO ioctl? Dear Neo, Sure, with pleasure. Since I tried not to bring too much ccw specific technical details here, I wrote quite briefly. Please feel free to ask me for supplements. :> > Since it is a new ioctl it is send down by QEMU in this case right? Right. > More details? As mentioned in the former emails, I currently added two new ioctl commands. 1. VFIO_DEVICE_CCW_HOT_RESET Both the name and the purpose of this command looks pretty the same with VFIO_DEVICE_PCI_HOT_RESET. Since Kevin proposed an individual callback for hot-reset handling, I believe this will not be a problem (only if you accept the proposal). 2. VFIO_DEVICE_CCW_CMD_REQUEST This intends to handle an intercepted channel I/O instruction. It basically need to do the following thing: a. Copy the raw data of the CCW program (a group of chained CCWs) from user into kernel space buffers. b. Do CCW program translation based on the raw data to get a real-device runnable CCW program. We'd pin pages for those CCWs which have memory space pointers for their offload, and update the CCW program with the pinned results (phys). c. Issue the translated CCW program to a real-device to perform the I/O operation, and wait for the I/O result interrupt. d. Once we got the I/O result, copy the result back to user, and unpin the pages. Step c could only be done by the physical device driver, since it's it that the int_handler belongs to. Step b and d should be done by the physical device driver. Or we'd pin/unpin pages in the mediated device driver? That's why I asked for the new callback. > > Thanks, > Neo > > > > > > > > > Thanks, > > > Kirti > > > > > > > The ccw physical device only supports one ccw mediated device. And I > > > > have two new ioctl commands for the ccw mediated device. One is > > > > to hot-reset the resource in the physical device that allocated for > > > > the mediated device, the other is to do an I/O instruction translation > > > > and perform an I/O operation on the physical device. I found the > > > > existing callbacks could not meet my requirements. > > > > > > > > Something like the following would be fine for my case: > > > > int (*ioctl)(struct mdev_device *vdev, > > > > unsigned int cmd, > > > > unsigned long arg); > > > > > > > > What do you think about this? > > > > > > > > -------- > > > > Dong Jia > > > > > > > > > > > -------- > > Dong Jia > > > -------- Dong Jia From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52394) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9puh-0006XY-Bm for qemu-devel@nongnu.org; Mon, 06 Jun 2016 04:29:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9puc-0007yW-9o for qemu-devel@nongnu.org; Mon, 06 Jun 2016 04:29:34 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:45396) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9pub-0007yG-Vs for qemu-devel@nongnu.org; Mon, 06 Jun 2016 04:29:30 -0400 Received: from localhost by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 Jun 2016 02:29:27 -0600 Date: Mon, 6 Jun 2016 16:29:11 +0800 From: Dong Jia Message-ID: <20160606162911.7021a02c@oc7835276234> In-Reply-To: <20160606062742.GA31747@nvidia.com> References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> <20160603165746.3df39542@oc7835276234> <799479b0-44d5-061f-185a-43df0dba2fb3@nvidia.com> <20160606140148.31ec05f9@oc7835276234> <20160606062742.GA31747@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Neo Jia Cc: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, shuai.ruan@intel.com, jike.song@intel.com, zhiyuan.lv@intel.com, Dong Jia On Sun, 5 Jun 2016 23:27:42 -0700 Neo Jia wrote: > On Mon, Jun 06, 2016 at 02:01:48PM +0800, Dong Jia wrote: > > On Mon, 6 Jun 2016 10:57:49 +0530 > > Kirti Wankhede wrote: > > > > > > > > > > > On 6/3/2016 2:27 PM, Dong Jia wrote: > > > > On Wed, 25 May 2016 01:28:15 +0530 > > > > Kirti Wankhede wrote: > > > > > > > > > > > > ...snip... > > > > > > > >> +struct phy_device_ops { > > > >> + struct module *owner; > > > >> + const struct attribute_group **dev_attr_groups; > > > >> + const struct attribute_group **mdev_attr_groups; > > > >> + > > > >> + int (*supported_config)(struct device *dev, char *config); > > > >> + int (*create)(struct device *dev, uuid_le uuid, > > > >> + uint32_t instance, char *mdev_params); > > > >> + int (*destroy)(struct device *dev, uuid_le uuid, > > > >> + uint32_t instance); > > > >> + int (*start)(uuid_le uuid); > > > >> + int (*shutdown)(uuid_le uuid); > > > >> + ssize_t (*read)(struct mdev_device *vdev, char *buf, size_t count, > > > >> + enum mdev_emul_space address_space, loff_t pos); > > > >> + ssize_t (*write)(struct mdev_device *vdev, char *buf, size_t count, > > > >> + enum mdev_emul_space address_space, loff_t pos); > > > >> + int (*set_irqs)(struct mdev_device *vdev, uint32_t flags, > > > >> + unsigned int index, unsigned int start, > > > >> + unsigned int count, void *data); > > > >> + int (*get_region_info)(struct mdev_device *vdev, int region_index, > > > >> + struct pci_region_info *region_info); > > > >> + int (*validate_map_request)(struct mdev_device *vdev, > > > >> + unsigned long virtaddr, > > > >> + unsigned long *pfn, unsigned long *size, > > > >> + pgprot_t *prot); > > > >> +}; > > > > > > > > Dear Kirti: > > > > > > > > When I rebased my vfio-ccw patches on this series, I found I need an > > > > extra 'ioctl' callback in phy_device_ops. > > > > > > > > > > Thanks for taking closer look. As per my knowledge ccw is not PCI > > > device, right? Correct me if I'm wrong. > > Dear Kirti: > > > > You are right. CCW is different to PCI. The official term is 'Channel > > I/O device'. They use 'Channels' (co-processors) and CCWs (channel > > command words) to handle I/O operations. > > > > > I'm curious to know. Are you planning to write a driver (vfio-mccw) for > > > mediated ccw device? > > I wrote two drivers: > > 1. A vfio-pccw driver for the physical ccw device, which will reigister > > the device and callbacks to mdev framework. With this, I could create > > a mediated ccw device for the physical one then. > > 2. A vfio-mccw driver for the mediated ccw device, which will add > > itself to a vfio_group, mimiced what vfio-mpci did. > > > > The problem is, vfio-mccw need to implement new ioctls besides the > > existing ones (VFIO_DEVICE_GET_INFO, etc). And these ioctls really need > > the physical device help to handle. > > Hi Dong, > > Could you please help us understand a bit more about the new VFIO ioctl? Dear Neo, Sure, with pleasure. Since I tried not to bring too much ccw specific technical details here, I wrote quite briefly. Please feel free to ask me for supplements. :> > Since it is a new ioctl it is send down by QEMU in this case right? Right. > More details? As mentioned in the former emails, I currently added two new ioctl commands. 1. VFIO_DEVICE_CCW_HOT_RESET Both the name and the purpose of this command looks pretty the same with VFIO_DEVICE_PCI_HOT_RESET. Since Kevin proposed an individual callback for hot-reset handling, I believe this will not be a problem (only if you accept the proposal). 2. VFIO_DEVICE_CCW_CMD_REQUEST This intends to handle an intercepted channel I/O instruction. It basically need to do the following thing: a. Copy the raw data of the CCW program (a group of chained CCWs) from user into kernel space buffers. b. Do CCW program translation based on the raw data to get a real-device runnable CCW program. We'd pin pages for those CCWs which have memory space pointers for their offload, and update the CCW program with the pinned results (phys). c. Issue the translated CCW program to a real-device to perform the I/O operation, and wait for the I/O result interrupt. d. Once we got the I/O result, copy the result back to user, and unpin the pages. Step c could only be done by the physical device driver, since it's it that the int_handler belongs to. Step b and d should be done by the physical device driver. Or we'd pin/unpin pages in the mediated device driver? That's why I asked for the new callback. > > Thanks, > Neo > > > > > > > > > Thanks, > > > Kirti > > > > > > > The ccw physical device only supports one ccw mediated device. And I > > > > have two new ioctl commands for the ccw mediated device. One is > > > > to hot-reset the resource in the physical device that allocated for > > > > the mediated device, the other is to do an I/O instruction translation > > > > and perform an I/O operation on the physical device. I found the > > > > existing callbacks could not meet my requirements. > > > > > > > > Something like the following would be fine for my case: > > > > int (*ioctl)(struct mdev_device *vdev, > > > > unsigned int cmd, > > > > unsigned long arg); > > > > > > > > What do you think about this? > > > > > > > > -------- > > > > Dong Jia > > > > > > > > > > > -------- > > Dong Jia > > > -------- Dong Jia