| Message ID | 1-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 2/3) | expand |
Hi Jason, On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > The SVA code is wired to assume that the SVA is programmed onto the > mm->pasid. The current core code always does this, so it is fine. > > Add a check for clarity. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > int ret = 0; > struct mm_struct *mm = domain->mm; > > + if (mm_get_enqcmd_pasid(mm) != id) > + return -EINVAL; > + This seems to get deleted entirely by a followup change. Is it added here to ensure a 1:1 mapping for git-bisect? Thanks Nicolin
On Thu, Mar 14, 2024 at 08:38:30PM -0700, Nicolin Chen wrote: > Hi Jason, > > On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > > The SVA code is wired to assume that the SVA is programmed onto the > > mm->pasid. The current core code always does this, so it is fine. > > > > Add a check for clarity. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > int ret = 0; > > struct mm_struct *mm = domain->mm; > > > > + if (mm_get_enqcmd_pasid(mm) != id) > > + return -EINVAL; > > + > > This seems to get deleted entirely by a followup change. Is it > added here to ensure a 1:1 mapping for git-bisect? Right, it is here in the series primarily for clarity, but arguably it should be backported too. I'll add a fixes line Jason
Hi Jason, On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > The SVA code is wired to assume that the SVA is programmed onto the > mm->pasid. The current core code always does this, so it is fine. > > Add a check for clarity. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > int ret = 0; > struct mm_struct *mm = domain->mm; > > + if (mm_get_enqcmd_pasid(mm) != id) > + return -EINVAL; > + I am not sure if that is needed, the only caller in the tree is the IOMMU code and it does the right thing, as that check is removed later anyway, I don’t think this patch adds much. > mutex_lock(&sva_lock); > ret = __arm_smmu_sva_bind(dev, id, mm); > mutex_unlock(&sva_lock); > -- > 2.43.2 Thanks, Mostafa
On Fri, Mar 22, 2024 at 05:48:52PM +0000, Mostafa Saleh wrote: > Hi Jason, > > On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > > The SVA code is wired to assume that the SVA is programmed onto the > > mm->pasid. The current core code always does this, so it is fine. > > > > Add a check for clarity. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > int ret = 0; > > struct mm_struct *mm = domain->mm; > > > > + if (mm_get_enqcmd_pasid(mm) != id) > > + return -EINVAL; > > + > I am not sure if that is needed, the only caller in the tree is the IOMMU code > and it does the right thing, as that check is removed later anyway, I don’t > think this patch adds much. It really should be backported, when we get drivers that do other things it creates a hazard. I've added a fixes line. Jason
On Tue, Mar 26, 2024 at 03:30:16PM -0300, Jason Gunthorpe wrote: > On Fri, Mar 22, 2024 at 05:48:52PM +0000, Mostafa Saleh wrote: > > Hi Jason, > > > > On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > > > The SVA code is wired to assume that the SVA is programmed onto the > > > mm->pasid. The current core code always does this, so it is fine. > > > > > > Add a check for clarity. > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > > int ret = 0; > > > struct mm_struct *mm = domain->mm; > > > > > > + if (mm_get_enqcmd_pasid(mm) != id) > > > + return -EINVAL; > > > + > > I am not sure if that is needed, the only caller in the tree is the IOMMU code > > and it does the right thing, as that check is removed later anyway, I don’t > > think this patch adds much. > > It really should be backported, when we get drivers that do other > things it creates a hazard. I've added a fixes line. Maybe I am misunderstanding the case, but AFAIU, the only caller for this is iommu_sva_bind_device() which is the function populating the pasid, so this condition should never hit with the current code. And Linux won’t backport new drivers, so there is no need to do that? Thanks, Mostafa
On Tue, Mar 26, 2024 at 07:06:18PM +0000, Mostafa Saleh wrote: > On Tue, Mar 26, 2024 at 03:30:16PM -0300, Jason Gunthorpe wrote: > > On Fri, Mar 22, 2024 at 05:48:52PM +0000, Mostafa Saleh wrote: > > > Hi Jason, > > > > > > On Mon, Mar 04, 2024 at 07:43:49PM -0400, Jason Gunthorpe wrote: > > > > The SVA code is wired to assume that the SVA is programmed onto the > > > > mm->pasid. The current core code always does this, so it is fine. > > > > > > > > Add a check for clarity. > > > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > > --- > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > > index 2610e82c0ecd0d..347c2fdd865c1a 100644 > > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > > > @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > > > int ret = 0; > > > > struct mm_struct *mm = domain->mm; > > > > > > > > + if (mm_get_enqcmd_pasid(mm) != id) > > > > + return -EINVAL; > > > > + > > > I am not sure if that is needed, the only caller in the tree is the IOMMU code > > > and it does the right thing, as that check is removed later anyway, I don’t > > > think this patch adds much. > > > > It really should be backported, when we get drivers that do other > > things it creates a hazard. I've added a fixes line. > > Maybe I am misunderstanding the case, but AFAIU, the only caller for this is > iommu_sva_bind_device() which is the function populating the pasid, so this > condition should never hit with the current code. Yes, but iommufd is going to break this soon. > And Linux won’t backport new drivers, so there is no need to do that? Heh, you never know what -stable will backport and the distros are quite a bit more lax. Seems nicer to fail quickly than get all corrupted. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index 2610e82c0ecd0d..347c2fdd865c1a 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -581,6 +581,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, int ret = 0; struct mm_struct *mm = domain->mm; + if (mm_get_enqcmd_pasid(mm) != id) + return -EINVAL; + mutex_lock(&sva_lock); ret = __arm_smmu_sva_bind(dev, id, mm); mutex_unlock(&sva_lock);