From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH 2/3] VFIO driver for mediated PCI device Date: Thu, 30 Jun 2016 14:34:28 +0800 Message-ID: <5774BD74.3030002@linux.intel.com> References: <1466440308-4961-1-git-send-email-kwankhede@nvidia.com> <1466440308-4961-3-git-send-email-kwankhede@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: shuai.ruan@intel.com, jike.song@intel.com, kvm@vger.kernel.org, kevin.tian@intel.com, qemu-devel@nongnu.org, zhiyuan.lv@intel.com, bjsdjshi@linux.vnet.ibm.com To: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com Return-path: In-Reply-To: <1466440308-4961-3-git-send-email-kwankhede@nvidia.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org On 06/21/2016 12:31 AM, Kirti Wankhede wrote: > +static int mdev_dev_mmio_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > +{ > + int ret; > + struct vfio_mdev *vmdev = vma->vm_private_data; > + struct mdev_device *mdev; > + struct parent_device *parent; > + u64 virtaddr = (u64)vmf->virtual_address; > + u64 offset, phyaddr; > + unsigned long req_size, pgoff; > + pgprot_t pg_prot; > + > + if (!vmdev && !vmdev->mdev) > + return -EINVAL; > + > + mdev = vmdev->mdev; > + parent = mdev->parent; > + > + offset = virtaddr - vma->vm_start; > + phyaddr = (vma->vm_pgoff << PAGE_SHIFT) + offset; > + pgoff = phyaddr >> PAGE_SHIFT; > + req_size = vma->vm_end - virtaddr; > + pg_prot = vma->vm_page_prot; > + > + if (parent && parent->ops->validate_map_request) { > + mutex_lock(&mdev->ops_lock); > + ret = parent->ops->validate_map_request(mdev, virtaddr, > + &pgoff, &req_size, > + &pg_prot); It is not only 'validate' but also adjust the parameters. > + mutex_unlock(&mdev->ops_lock); > + if (ret) > + return ret; > + > + if (!req_size) > + return -EINVAL; > + } > + > + ret = remap_pfn_range(vma, virtaddr, pgoff, req_size, pg_prot); > + Do you know why "Issues with mmap region fault handler, EPT is not correctly populated with the information provided by remap_pfn_range() inside fault handler." as you mentioned in the patch 0. > + return ret | VM_FAULT_NOPAGE; > +} > + > +static const struct vm_operations_struct mdev_dev_mmio_ops = { > + .fault = mdev_dev_mmio_fault, > +}; > + > + > +static int vfio_mpci_mmap(void *device_data, struct vm_area_struct *vma) > +{ > + unsigned int index; > + struct vfio_mdev *vmdev = device_data; > + struct mdev_device *mdev = vmdev->mdev; > + struct pci_dev *pdev; > + unsigned long pgoff; > + loff_t offset; > + > + if (!mdev->parent || !dev_is_pci(mdev->parent->dev)) > + return -EINVAL; > + > + pdev = to_pci_dev(mdev->parent->dev); > + > + offset = vma->vm_pgoff << PAGE_SHIFT; > + > + index = VFIO_PCI_OFFSET_TO_INDEX(offset); > + > + if (index >= VFIO_PCI_ROM_REGION_INDEX) > + return -EINVAL; > + > + pgoff = vma->vm_pgoff & > + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1); > + > + vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff; > + > + vma->vm_private_data = vmdev; > + vma->vm_ops = &mdev_dev_mmio_ops; > + > + return 0; > +} > + > +static const struct vfio_device_ops vfio_mpci_dev_ops = { > + .name = "vfio-mpci", > + .open = vfio_mpci_open, > + .release = vfio_mpci_close, > + .ioctl = vfio_mpci_unlocked_ioctl, > + .read = vfio_mpci_read, > + .write = vfio_mpci_write, > + .mmap = vfio_mpci_mmap, > +}; > + > +int vfio_mpci_probe(struct device *dev) > +{ > + struct vfio_mdev *vmdev; > + struct mdev_device *mdev = to_mdev_device(dev); > + int ret; > + > + if (!mdev) > + return -EINVAL; > + > + vmdev = kzalloc(sizeof(*vmdev), GFP_KERNEL); > + if (IS_ERR(vmdev)) > + return PTR_ERR(vmdev); > + > + vmdev->mdev = mdev_get_device(mdev); > + vmdev->group = mdev->group; > + mutex_init(&vmdev->vfio_mdev_lock); > + > + ret = vfio_add_group_dev(dev, &vfio_mpci_dev_ops, vmdev); > + if (ret) > + kfree(vmdev); > + > + mdev_put_device(mdev); If you can make sure that mdev is always valid during vmdev's lifecyle, it is not necessary to get and put the refcount of mdev.