lkml.org 
[lkml]   [2016]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v13 09/22] vfio iommu type1: Add task structure to vfio_dma
    From
    Date


    On 11/16/2016 11:36 AM, Dong Jia Shi wrote:
    > * Kirti Wankhede <kwankhede@nvidia.com> [2016-11-15 20:59:52 +0530]:
    >
    > Hi Kirti,
    >
    > [...]
    > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
    >
    >> @@ -331,13 +338,16 @@ static long vfio_pin_pages_remote(unsigned long vaddr, long npage,
    >> }
    >>
    >> if (!rsvd)
    >> - vfio_lock_acct(current, i);
    >> + vfio_lock_acct(dma->task, i);
    >> + ret = i;
    >>
    >> - return i;
    >> +pin_pg_remote_exit:
    > out_mmput sounds a better name to me.
    >
    >> + mmput(mm);
    >> + return ret;
    >> }
    >>
    > [...]
    >
    >> @@ -510,6 +521,12 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
    >> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
    >> if (!iommu->v2 && unmap->iova > dma->iova)
    >> break;
    >> + /*
    >> + * Task with same address space who mapped this iova range is
    >> + * allowed to unmap the iova range.
    >> + */
    >> + if (dma->task->mm != current->mm)
    > How about:
    > if (dma->task != current)
    >

    As I mentioned in comment above this and commit description, if a
    process calls DMA_MAP, forks a thread and then child thread calls
    DMA_UNMAP, this should be allowed since address space is same for parent
    process and child. QEMU also works that way.

    >> + break;
    >> unmapped += dma->size;
    >> vfio_remove_dma(iommu, dma);
    >> }
    >> @@ -576,17 +593,55 @@ unwind:
    >> return ret;
    >> }
    >>
    >> +static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
    >> + size_t map_size)
    > Do you factor out this function for future usage?
    > I didn't find the other callers.
    >

    This is pulled out to make caller simple and short. Otherwise
    vfio_dma_do_map() would have become a long function.


    >> +{
    >> + dma_addr_t iova = dma->iova;
    >> + unsigned long vaddr = dma->vaddr;
    >> + size_t size = map_size;
    >> + long npage;
    >> + unsigned long pfn;
    >> + int ret = 0;
    >> +
    >> + while (size) {
    >> + /* Pin a contiguous chunk of memory */
    >> + npage = vfio_pin_pages_remote(dma, vaddr + dma->size,
    >> + size >> PAGE_SHIFT, dma->prot,
    >> + &pfn);
    >> + if (npage <= 0) {
    >> + WARN_ON(!npage);
    >> + ret = (int)npage;
    >> + break;
    >> + }
    >> +
    >> + /* Map it! */
    >> + ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage,
    >> + dma->prot);
    >> + if (ret) {
    >> + vfio_unpin_pages_remote(dma, pfn, npage,
    >> + dma->prot, true);
    >> + break;
    >> + }
    >> +
    >> + size -= npage << PAGE_SHIFT;
    >> + dma->size += npage << PAGE_SHIFT;
    >> + }
    >> +
    >> + if (ret)
    >> + vfio_remove_dma(iommu, dma);
    >> +
    >> + return ret;
    >> +}
    >> +
    >> static int vfio_dma_do_map(struct vfio_iommu *iommu,
    >> struct vfio_iommu_type1_dma_map *map)
    >> {
    >> dma_addr_t iova = map->iova;
    >> unsigned long vaddr = map->vaddr;
    >> size_t size = map->size;
    >> - long npage;
    >> int ret = 0, prot = 0;
    >> uint64_t mask;
    >> struct vfio_dma *dma;
    >> - unsigned long pfn;
    >>
    >> /* Verify that none of our __u64 fields overflow */
    >> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
    >> @@ -612,47 +667,27 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
    >> mutex_lock(&iommu->lock);
    >>
    >> if (vfio_find_dma(iommu, iova, size)) {
    >> - mutex_unlock(&iommu->lock);
    >> - return -EEXIST;
    >> + ret = -EEXIST;
    >> + goto do_map_err;
    >> }
    >>
    >> dma = kzalloc(sizeof(*dma), GFP_KERNEL);
    >> if (!dma) {
    >> - mutex_unlock(&iommu->lock);
    >> - return -ENOMEM;
    >> + ret = -ENOMEM;
    >> + goto do_map_err;
    >> }
    >>
    >> dma->iova = iova;
    >> dma->vaddr = vaddr;
    >> dma->prot = prot;
    >> + get_task_struct(current);
    >> + dma->task = current;
    >>
    >> /* Insert zero-sized and grow as we map chunks of it */
    >> vfio_link_dma(iommu, dma);
    >>
    >> - while (size) {
    >> - /* Pin a contiguous chunk of memory */
    >> - npage = vfio_pin_pages_remote(vaddr + dma->size,
    >> - size >> PAGE_SHIFT, prot, &pfn);
    >> - if (npage <= 0) {
    >> - WARN_ON(!npage);
    >> - ret = (int)npage;
    >> - break;
    >> - }
    >> -
    >> - /* Map it! */
    >> - ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage, prot);
    >> - if (ret) {
    >> - vfio_unpin_pages_remote(pfn, npage, prot, true);
    >> - break;
    >> - }
    >> -
    >> - size -= npage << PAGE_SHIFT;
    >> - dma->size += npage << PAGE_SHIFT;
    >> - }
    >> -
    >> - if (ret)
    >> - vfio_remove_dma(iommu, dma);
    >> -
    >> + ret = vfio_pin_map_dma(iommu, dma, size);
    >> +do_map_err:
    > Rename to out_unlock?
    >
    >> mutex_unlock(&iommu->lock);
    >> return ret;
    >> }
    >> --
    >> 2.7.0
    >>
    >
    > Otherwise, LGTM!
    >

    Thanks.

    Kirti.

    \
     
     \ /
      Last update: 2016-11-16 16:13    [W:4.187 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site