Re: [PATCH v4 01/27] iommu/arm-smmu-v3: Check that the RID domain is S1 in SVA

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 30, 2024 at 08:46:10AM +0000, Shameerali Kolothum Thodi wrote:

> > 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 05722121f00e70..b4549d698f3569 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
> > @@ -387,7 +387,13 @@ static int __arm_smmu_sva_bind(struct device *dev,
> > struct mm_struct *mm)
> >  	struct arm_smmu_bond *bond;
> >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> >  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > -	struct arm_smmu_domain *smmu_domain =
> > to_smmu_domain(domain);
> > +	struct arm_smmu_domain *smmu_domain;
> > +
> > +	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
> > +		return -ENODEV;
> > +	smmu_domain = to_smmu_domain(iommu_get_domain_for_dev(dev));
> 
> We already have the iommu_domain from above.

Yep
 
> > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > +		return -ENODEV;
> 
> I think we need to do the above checks in arm_smmu_sva_remove_dev_pasid()
> as well.

The core won't call that unless a PASID is already attached, meaning
we already passed the above check in bind. So it shouldn't need to be
duplicated.

Further, at this instant all the RID attach paths are protected by:

	if (arm_smmu_master_sva_enabled(master))
		return -EBUSY;

Dropping the S1 check was a mistake introduced in commit 386fa64fd52b
("arm-smmu-v3/sva: Add SVA domain support")

The current logic for SMMUv3 requires that a calling driver assert the
SVA feature, which locks the STE, and then if the STE was a S1 you can
install a SVA PASID.

Thanks,
Jason