RE: [PATCH v1 2/8] vfio/type1: Add vfio_iommu_type1 parameter for quota tuning

Tian, Kevin
2020-03-30T08:41:22Z

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Sunday, March 22, 2020 8:32 PM
>
> From: Liu Yi L <yi.l.liu@intel.com>
>
> This patch adds a module option to make the PASID quota tunable by
> administrator.
>
> TODO: needs to think more on how to make the tuning to be per-process.
>
> Previous discussions:
> https://patchwork.kernel.org/patch/11209429/
>
> Cc: Kevin Tian <kevin.tian@intel.com>
> CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> ---
> drivers/vfio/vfio.c | 8 +++++++-
> drivers/vfio/vfio_iommu_type1.c | 7 ++++++-
> include/linux/vfio.h | 3 ++-
> 3 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index d13b483..020a792 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -2217,13 +2217,19 @@ struct vfio_mm *vfio_mm_get_from_task(struct
> task_struct *task)
> }
> EXPORT_SYMBOL_GPL(vfio_mm_get_from_task);
>
> -int vfio_mm_pasid_alloc(struct vfio_mm *vmm, int min, int max)
> +int vfio_mm_pasid_alloc(struct vfio_mm *vmm, int quota, int min, int max)
> {
> ioasid_t pasid;
> int ret = -ENOSPC;
>
> mutex_lock(&vmm->pasid_lock);
>
> + /* update quota as it is tunable by admin */
> + if (vmm->pasid_quota != quota) {
> + vmm->pasid_quota = quota;
> + ioasid_adjust_set(vmm->ioasid_sid, quota);
> + }
> +

It's a bit weird to have quota adjusted in the alloc path, since the latter might
be initiated by non-privileged users. Why not doing the simple math in vfio_
create_mm to set the quota when the ioasid set is created? even in the future
you may allow per-process quota setting, that should come from separate
privileged path instead of thru alloc...

> pasid = ioasid_alloc(vmm->ioasid_sid, min, max, NULL);
> if (pasid == INVALID_IOASID) {
> ret = -ENOSPC;
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> index 331ceee..e40afc0 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -60,6 +60,11 @@ module_param_named(dma_entry_limit,
> dma_entry_limit, uint, 0644);
> MODULE_PARM_DESC(dma_entry_limit,
> "Maximum number of user DMA mappings per container
> (65535).");
>
> +static int pasid_quota = VFIO_DEFAULT_PASID_QUOTA;
> +module_param_named(pasid_quota, pasid_quota, uint, 0644);
> +MODULE_PARM_DESC(pasid_quota,
> + "Quota of user owned PASIDs per vfio-based application
> (1000).");
> +
> struct vfio_iommu {
> struct list_head domain_list;
> struct list_head iova_list;
> @@ -2200,7 +2205,7 @@ static int vfio_iommu_type1_pasid_alloc(struct
> vfio_iommu *iommu,
> goto out_unlock;
> }
> if (vmm)
> - ret = vfio_mm_pasid_alloc(vmm, min, max);
> + ret = vfio_mm_pasid_alloc(vmm, pasid_quota, min, max);
> else
> ret = -EINVAL;
> out_unlock:
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 75f9f7f1..af2ef78 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -106,7 +106,8 @@ struct vfio_mm {
>
> extern struct vfio_mm *vfio_mm_get_from_task(struct task_struct *task);
> extern void vfio_mm_put(struct vfio_mm *vmm);
> -extern int vfio_mm_pasid_alloc(struct vfio_mm *vmm, int min, int max);
> +extern int vfio_mm_pasid_alloc(struct vfio_mm *vmm,
> + int quota, int min, int max);
> extern int vfio_mm_pasid_free(struct vfio_mm *vmm, ioasid_t pasid);
>
> /*
> --
> 2.7.4