Date:   Wed, 15 Feb 2023 13:36:51 -0800
From:   Jacob Pan <jacob.jun.pan@linux.intel.com>
To:     Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, iommu@lists.linux.dev,
        Jason Gunthorpe <jgg@nvidia.com>,
        Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>,
        Jean-Philippe Brucker <jean-philippe@linaro.com>,
        Robin Murphy <robin.murphy@arm.com>,
        Will Deacon <will@kernel.org>,
        David Woodhouse <dwmw2@infradead.org>,
        Raj Ashok <ashok.raj@intel.com>,
        "Tian, Kevin" <kevin.tian@intel.com>, Yi Liu <yi.l.liu@intel.com>,
        jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Message-ID: <20230215133651.776a1553@jacob-builder>
In-Reply-To: <Y+tW3kxFeOMcELww@myrica>
References: <20230214064414.1038058-1-jacob.jun.pan@linux.intel.com>
        <20230214064414.1038058-3-jacob.jun.pan@linux.intel.com>
        <Y+tW3kxFeOMcELww@myrica>
Organization: OTC
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jean-Philippe,

On Tue, 14 Feb 2023 09:39:42 +0000, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:

> On Mon, Feb 13, 2023 at 10:44:13PM -0800, Jacob Pan wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> > each mm_struct.
> > 
> > Future work would be to allow drivers using the SVA APIs to reserve
> > global PASIDs from this IDA for their internal use, eg with the DMA API
> > PASID support.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> > v2: minor fixes
> > 	- let idxd driver use iommu.h for ioasid_t
> > 	- return 0 after mm_pasid_set()
> > ---
> >  drivers/dma/idxd/idxd.h   |  1 +
> >  drivers/iommu/iommu-sva.c | 53 +++++++++++----------------------------
> >  drivers/iommu/iommu-sva.h |  3 ---
> >  include/linux/sched/mm.h  |  8 +-----
> >  4 files changed, 17 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index 7ced8d283d98..417e602a46b6 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -13,6 +13,7 @@
> >  #include <linux/ioasid.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/perf_event.h>
> > +#include <linux/iommu.h>  
> 
> Unrelated 
this is for ioasid_t, won't compile without it 

> >  #include <uapi/linux/idxd.h>
> >  #include "registers.h"
> >  
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 24bf9b2b58aa..8fb6f1867af3 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -9,24 +9,11 @@
> >  #include "iommu-sva.h"
> >  
> >  static DEFINE_MUTEX(iommu_sva_lock);
> > -static DECLARE_IOASID_SET(iommu_sva_pasid);
> > +static DEFINE_IDA(iommu_global_pasid_ida);
> >  
> > -/**
> > - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> > - * @mm: the mm
> > - * @min: minimum PASID value (inclusive)
> > - * @max: maximum PASID value (inclusive)
> > - *
> > - * Try to allocate a PASID for this mm, or take a reference to the
> > existing one
> > - * provided it fits within the [@min, @max] range. On success the
> > PASID is
> > - * available in mm->pasid and will be available for the lifetime of
> > the mm.
> > - *
> > - * Returns 0 on success and < 0 on error.
> > - */
> > -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
> > max) +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t
> > min, ioasid_t max) {
> > -	int ret = 0;
> > -	ioasid_t pasid;
> > +	int ret;
> >  
> >  	if (min == INVALID_IOASID || max == INVALID_IOASID ||
> >  	    min == 0 || max < min)
> > @@ -37,39 +24,29 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm,
> > ioasid_t min, ioasid_t max) if (pasid_valid(mm->pasid)) {
> >  		if (mm->pasid < min || mm->pasid >= max)
> >  			ret = -EOVERFLOW;
> > +		else
> > +			ret = 0;
> >  		goto out;
> >  	}
> >  
> > -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> > -	if (!pasid_valid(pasid))
> > -		ret = -ENOMEM;
> > -	else
> > -		mm_pasid_set(mm, pasid);
> > +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
> > GFP_KERNEL);
> > +	if (ret < min)  
> 
> Just check ret < 0
yes, but i thought < min is safe too. 

> > +		goto out;
> > +	mm_pasid_set(mm, ret);
> > +	ret = 0;
> >  out:
> >  	mutex_unlock(&iommu_sva_lock);
> >  	return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
> >  
> > -/* ioasid_find getter() requires a void * argument */
> > -static bool __mmget_not_zero(void *mm)
> > +void mm_pasid_drop(struct mm_struct *mm)
> >  {
> > -	return mmget_not_zero(mm);
> > -}
> > +	pr_alert("%s %d", __func__, mm->pasid);  
> 
> Debug print
will remove,
Thanks a lot!


Jacob