Hi Yi, On 7/4/20 1:26 PM, Liu Yi L wrote: > This patch refactors the vfio_iommu_type1_ioctl() to use switch instead of > if-else, and each cmd got a helper function. command > > Cc: Kevin Tian <kevin.t...@intel.com> > CC: Jacob Pan <jacob.jun....@linux.intel.com> > Cc: Alex Williamson <alex.william...@redhat.com> > Cc: Eric Auger <eric.au...@redhat.com> > Cc: Jean-Philippe Brucker <jean-phili...@linaro.org> > Cc: Joerg Roedel <j...@8bytes.org> > Cc: Lu Baolu <baolu...@linux.intel.com> > Suggested-by: Christoph Hellwig <h...@infradead.org> > Signed-off-by: Liu Yi L <yi.l....@intel.com> > --- > drivers/vfio/vfio_iommu_type1.c | 392 > ++++++++++++++++++++++------------------ > 1 file changed, 213 insertions(+), 179 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 5e556ac..7accb59 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -2453,6 +2453,23 @@ static int vfio_domains_have_iommu_cache(struct > vfio_iommu *iommu) > return ret; > } > > +static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu, > + unsigned long arg) > +{ > + switch (arg) { > + case VFIO_TYPE1_IOMMU: > + case VFIO_TYPE1v2_IOMMU: > + case VFIO_TYPE1_NESTING_IOMMU: > + return 1; > + case VFIO_DMA_CC_IOMMU: > + if (!iommu) > + return 0; > + return vfio_domains_have_iommu_cache(iommu); > + default: > + return 0; > + } > +} > + > static int vfio_iommu_iova_add_cap(struct vfio_info_cap *caps, > struct vfio_iommu_type1_info_cap_iova_range *cap_iovas, > size_t size) > @@ -2529,238 +2546,255 @@ static int vfio_iommu_migration_build_caps(struct > vfio_iommu *iommu, > return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig)); > } > > -static long vfio_iommu_type1_ioctl(void *iommu_data, > - unsigned int cmd, unsigned long arg) > +static int vfio_iommu_type1_get_info(struct vfio_iommu *iommu, > + unsigned long arg) > { > - struct vfio_iommu *iommu = iommu_data; > + struct vfio_iommu_type1_info info; > unsigned long minsz; > + struct vfio_info_cap caps = { .buf = NULL, .size = 0 }; > + unsigned long capsz; > + int ret; > > - if (cmd == VFIO_CHECK_EXTENSION) { > - switch (arg) { > - case VFIO_TYPE1_IOMMU: > - case VFIO_TYPE1v2_IOMMU: > - case VFIO_TYPE1_NESTING_IOMMU: > - return 1; > - case VFIO_DMA_CC_IOMMU: > - if (!iommu) > - return 0; > - return vfio_domains_have_iommu_cache(iommu); > - default: > - return 0; > - } > - } else if (cmd == VFIO_IOMMU_GET_INFO) { > - struct vfio_iommu_type1_info info; > - struct vfio_info_cap caps = { .buf = NULL, .size = 0 }; > - unsigned long capsz; > - int ret; > - > - minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes); > + minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes); > > - /* For backward compatibility, cannot require this */ > - capsz = offsetofend(struct vfio_iommu_type1_info, cap_offset); > + /* For backward compatibility, cannot require this */ > + capsz = offsetofend(struct vfio_iommu_type1_info, cap_offset); > > - if (copy_from_user(&info, (void __user *)arg, minsz)) > - return -EFAULT; > + if (copy_from_user(&info, (void __user *)arg, minsz)) > + return -EFAULT; > > - if (info.argsz < minsz) > - return -EINVAL; > + if (info.argsz < minsz) > + return -EINVAL; > > - if (info.argsz >= capsz) { > - minsz = capsz; > - info.cap_offset = 0; /* output, no-recopy necessary */ > - } > + if (info.argsz >= capsz) { > + minsz = capsz; > + info.cap_offset = 0; /* output, no-recopy necessary */ > + } > > - mutex_lock(&iommu->lock); > - info.flags = VFIO_IOMMU_INFO_PGSIZES; > + mutex_lock(&iommu->lock); > + info.flags = VFIO_IOMMU_INFO_PGSIZES; > > - info.iova_pgsizes = iommu->pgsize_bitmap; > + info.iova_pgsizes = iommu->pgsize_bitmap; > > - ret = vfio_iommu_migration_build_caps(iommu, &caps); > + ret = vfio_iommu_migration_build_caps(iommu, &caps); > > - if (!ret) > - ret = vfio_iommu_iova_build_caps(iommu, &caps); > + if (!ret) > + ret = vfio_iommu_iova_build_caps(iommu, &caps); > > - mutex_unlock(&iommu->lock); > + mutex_unlock(&iommu->lock); > > - if (ret) > - return ret; > + if (ret) > + return ret; > > - if (caps.size) { > - info.flags |= VFIO_IOMMU_INFO_CAPS; > + if (caps.size) { > + info.flags |= VFIO_IOMMU_INFO_CAPS; > > - if (info.argsz < sizeof(info) + caps.size) { > - info.argsz = sizeof(info) + caps.size; > - } else { > - vfio_info_cap_shift(&caps, sizeof(info)); > - if (copy_to_user((void __user *)arg + > - sizeof(info), caps.buf, > - caps.size)) { > - kfree(caps.buf); > - return -EFAULT; > - } > - info.cap_offset = sizeof(info); > + if (info.argsz < sizeof(info) + caps.size) { > + info.argsz = sizeof(info) + caps.size; > + } else { > + vfio_info_cap_shift(&caps, sizeof(info)); > + if (copy_to_user((void __user *)arg + > + sizeof(info), caps.buf, > + caps.size)) { > + kfree(caps.buf); > + return -EFAULT; > } > - > - kfree(caps.buf); > + info.cap_offset = sizeof(info); > } > > - return copy_to_user((void __user *)arg, &info, minsz) ? > - -EFAULT : 0; > + kfree(caps.buf); > + } > > - } else if (cmd == VFIO_IOMMU_MAP_DMA) { > - struct vfio_iommu_type1_dma_map map; > - uint32_t mask = VFIO_DMA_MAP_FLAG_READ | > - VFIO_DMA_MAP_FLAG_WRITE; > + return copy_to_user((void __user *)arg, &info, minsz) ? > + -EFAULT : 0; > +} > > - minsz = offsetofend(struct vfio_iommu_type1_dma_map, size); > +static int vfio_iommu_type1_map_dma(struct vfio_iommu *iommu, > + unsigned long arg) > +{ > + struct vfio_iommu_type1_dma_map map; > + unsigned long minsz; > + uint32_t mask = VFIO_DMA_MAP_FLAG_READ | > + VFIO_DMA_MAP_FLAG_WRITE; nit: may fit into a single line? other examples below. > > - if (copy_from_user(&map, (void __user *)arg, minsz)) > - return -EFAULT; > + minsz = offsetofend(struct vfio_iommu_type1_dma_map, size); > > - if (map.argsz < minsz || map.flags & ~mask) > - return -EINVAL; > + if (copy_from_user(&map, (void __user *)arg, minsz)) > + return -EFAULT; > > - return vfio_dma_do_map(iommu, &map); > + if (map.argsz < minsz || map.flags & ~mask) > + return -EINVAL; > > - } else if (cmd == VFIO_IOMMU_UNMAP_DMA) { > - struct vfio_iommu_type1_dma_unmap unmap; > - struct vfio_bitmap bitmap = { 0 }; > - int ret; > + return vfio_dma_do_map(iommu, &map); > +} > > - minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size); > +static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, > + unsigned long arg) > +{ > + struct vfio_iommu_type1_dma_unmap unmap; > + struct vfio_bitmap bitmap = { 0 }; > + unsigned long minsz; > + long ret; int? > > - if (copy_from_user(&unmap, (void __user *)arg, minsz)) > - return -EFAULT; > + minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size); > > - if (unmap.argsz < minsz || > - unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) > - return -EINVAL; > + if (copy_from_user(&unmap, (void __user *)arg, minsz)) > + return -EFAULT; > > - if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { > - unsigned long pgshift; > + if (unmap.argsz < minsz || > + unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) > + return -EINVAL; > > - if (unmap.argsz < (minsz + sizeof(bitmap))) > - return -EINVAL; > + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { > + unsigned long pgshift; > > - if (copy_from_user(&bitmap, > - (void __user *)(arg + minsz), > - sizeof(bitmap))) > - return -EFAULT; > + if (unmap.argsz < (minsz + sizeof(bitmap))) > + return -EINVAL; > > - if (!access_ok((void __user *)bitmap.data, bitmap.size)) > - return -EINVAL; > + if (copy_from_user(&bitmap, > + (void __user *)(arg + minsz), > + sizeof(bitmap))) > + return -EFAULT; > > - pgshift = __ffs(bitmap.pgsize); > - ret = verify_bitmap_size(unmap.size >> pgshift, > - bitmap.size); > - if (ret) > - return ret; > - } > + if (!access_ok((void __user *)bitmap.data, bitmap.size)) > + return -EINVAL; > > - ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap); > + pgshift = __ffs(bitmap.pgsize); > + ret = verify_bitmap_size(unmap.size >> pgshift, > + bitmap.size); > if (ret) > return ret; > + } > + > + ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap); > + if (ret) > + return ret; > > - return copy_to_user((void __user *)arg, &unmap, minsz) ? > + return copy_to_user((void __user *)arg, &unmap, minsz) ? > -EFAULT : 0; > - } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) { > - struct vfio_iommu_type1_dirty_bitmap dirty; > - uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START | > - VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP | > - VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP; > - int ret = 0; > +} > > - if (!iommu->v2) > - return -EACCES; > +static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu, > + unsigned long arg) > +{ > + struct vfio_iommu_type1_dirty_bitmap dirty; > + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START | > + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP | > + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP; > + unsigned long minsz; > + int ret = 0; > > - minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap, > - flags); > + if (!iommu->v2) > + return -EACCES; > > - if (copy_from_user(&dirty, (void __user *)arg, minsz)) > - return -EFAULT; > + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap, > + flags); single line? > > - if (dirty.argsz < minsz || dirty.flags & ~mask) > - return -EINVAL; > + if (copy_from_user(&dirty, (void __user *)arg, minsz)) > + return -EFAULT; > + > + if (dirty.argsz < minsz || dirty.flags & ~mask) > + return -EINVAL; > + > + /* only one flag should be set at a time */ > + if (__ffs(dirty.flags) != __fls(dirty.flags)) > + return -EINVAL; > + > + if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) { > + size_t pgsize; > > - /* only one flag should be set at a time */ > - if (__ffs(dirty.flags) != __fls(dirty.flags)) > + mutex_lock(&iommu->lock); > + pgsize = 1 << __ffs(iommu->pgsize_bitmap); > + if (!iommu->dirty_page_tracking) { > + ret = vfio_dma_bitmap_alloc_all(iommu, pgsize); > + if (!ret) > + iommu->dirty_page_tracking = true; > + } > + mutex_unlock(&iommu->lock); > + return ret; > + } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) { > + mutex_lock(&iommu->lock); > + if (iommu->dirty_page_tracking) { > + iommu->dirty_page_tracking = false; > + vfio_dma_bitmap_free_all(iommu); > + } > + mutex_unlock(&iommu->lock); > + return 0; > + } else if (dirty.flags & > + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) { single line? > + struct vfio_iommu_type1_dirty_bitmap_get range; > + unsigned long pgshift; > + size_t data_size = dirty.argsz - minsz; > + size_t iommu_pgsize; > + > + if (!data_size || data_size < sizeof(range)) > return -EINVAL; > > - if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) { > - size_t pgsize; > + if (copy_from_user(&range, (void __user *)(arg + minsz), > + sizeof(range))) > + return -EFAULT; > > - mutex_lock(&iommu->lock); > - pgsize = 1 << __ffs(iommu->pgsize_bitmap); > - if (!iommu->dirty_page_tracking) { > - ret = vfio_dma_bitmap_alloc_all(iommu, pgsize); > - if (!ret) > - iommu->dirty_page_tracking = true; > - } > - mutex_unlock(&iommu->lock); > + if (range.iova + range.size < range.iova) > + return -EINVAL; > + if (!access_ok((void __user *)range.bitmap.data, > + range.bitmap.size)) > + return -EINVAL; > + > + pgshift = __ffs(range.bitmap.pgsize); > + ret = verify_bitmap_size(range.size >> pgshift, > + range.bitmap.size); > + if (ret) > return ret; > - } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) { > - mutex_lock(&iommu->lock); > - if (iommu->dirty_page_tracking) { > - iommu->dirty_page_tracking = false; > - vfio_dma_bitmap_free_all(iommu); > - } > - mutex_unlock(&iommu->lock); > - return 0; > - } else if (dirty.flags & > - VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) { idem > - struct vfio_iommu_type1_dirty_bitmap_get range; > - unsigned long pgshift; > - size_t data_size = dirty.argsz - minsz; > - size_t iommu_pgsize; > - > - if (!data_size || data_size < sizeof(range)) > - return -EINVAL; > - > - if (copy_from_user(&range, (void __user *)(arg + minsz), > - sizeof(range))) > - return -EFAULT; > > - if (range.iova + range.size < range.iova) > - return -EINVAL; > - if (!access_ok((void __user *)range.bitmap.data, > - range.bitmap.size)) > - return -EINVAL; > + mutex_lock(&iommu->lock); > > - pgshift = __ffs(range.bitmap.pgsize); > - ret = verify_bitmap_size(range.size >> pgshift, > - range.bitmap.size); > - if (ret) > - return ret; > + iommu_pgsize = (size_t)1 << __ffs(iommu->pgsize_bitmap); > > - mutex_lock(&iommu->lock); > + /* allow only smallest supported pgsize */ > + if (range.bitmap.pgsize != iommu_pgsize) { > + ret = -EINVAL; > + goto out_unlock; > + } > + if (range.iova & (iommu_pgsize - 1)) { > + ret = -EINVAL; > + goto out_unlock; > + } > + if (!range.size || range.size & (iommu_pgsize - 1)) { > + ret = -EINVAL; > + goto out_unlock; > + } > > - iommu_pgsize = (size_t)1 << __ffs(iommu->pgsize_bitmap); > + if (iommu->dirty_page_tracking) > + ret = vfio_iova_dirty_bitmap(range.bitmap.data, > + iommu, range.iova, range.size, > + range.bitmap.pgsize); > + else > + ret = -EINVAL; > +out_unlock: > + mutex_unlock(&iommu->lock); > > - /* allow only smallest supported pgsize */ > - if (range.bitmap.pgsize != iommu_pgsize) { > - ret = -EINVAL; > - goto out_unlock; > - } > - if (range.iova & (iommu_pgsize - 1)) { > - ret = -EINVAL; > - goto out_unlock; > - } > - if (!range.size || range.size & (iommu_pgsize - 1)) { > - ret = -EINVAL; > - goto out_unlock; > - } > + return ret; > + } > > - if (iommu->dirty_page_tracking) > - ret = vfio_iova_dirty_bitmap(range.bitmap.data, > - iommu, range.iova, range.size, > - range.bitmap.pgsize); > - else > - ret = -EINVAL; > -out_unlock: > - mutex_unlock(&iommu->lock); > + return -EINVAL; > +} > > - return ret; > - } > +static long vfio_iommu_type1_ioctl(void *iommu_data, > + unsigned int cmd, unsigned long arg) > +{ > + struct vfio_iommu *iommu = iommu_data; > + > + switch (cmd) { > + case VFIO_CHECK_EXTENSION: > + return vfio_iommu_type1_check_extension(iommu, arg); > + case VFIO_IOMMU_GET_INFO: > + return vfio_iommu_type1_get_info(iommu, arg); > + case VFIO_IOMMU_MAP_DMA: > + return vfio_iommu_type1_map_dma(iommu, arg); > + case VFIO_IOMMU_UNMAP_DMA: > + return vfio_iommu_type1_unmap_dma(iommu, arg); > + case VFIO_IOMMU_DIRTY_PAGES: > + return vfio_iommu_type1_dirty_pages(iommu, arg); default: return -ENOTTY; ? > } > > return -ENOTTY; >
Besides Reviewed-by: Eric Auger <eric.au...@redhat.com> Thanks Eric