Re: [PATCH v7 12/14] iommu/arm-smmu-v3: Introduce struct arm_smmu_vmaster

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Feb 24, 2025 at 02:24:55PM -0800, Nicolin Chen wrote:
> On Mon, Feb 24, 2025 at 09:53:58PM +0000, Pranjal Shrivastava wrote:
> > On Mon, Feb 24, 2025 at 01:31:11PM -0800, Nicolin Chen wrote:
> > > On Mon, Feb 24, 2025 at 08:35:56PM +0000, Pranjal Shrivastava wrote:
> > > > oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > index 5aa2e7af58b4..364d8469a480 100644
> > > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > > > > +				    struct iommu_domain *domain)
> > > > > +{
> > > > > +	struct arm_smmu_nested_domain *nested_domain;
> > > > > +	struct arm_smmu_vmaster *vmaster;
> > > > > +	unsigned long vsid;
> > > > > +	int ret;
> > > > > +
> > > > > +	iommu_group_mutex_assert(state->master->dev);
> > > > > +
> > > > > +	if (domain->type != IOMMU_DOMAIN_NESTED)
> > > > > +		return 0;
> > > > > +	nested_domain = to_smmu_nested_domain(domain);
> > > > > +
> > > > > +	/* Skip invalid vSTE */
> > > > > +	if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > > > > +		return 0;
> > > > > +
> > > > > +	ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> > > > > +					 state->master->dev, &vsid);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> > > > > +	if (!vmaster)
> > > > > +		return -ENOMEM;
> > > > > +	vmaster->vsmmu = nested_domain->vsmmu;
> > > > > +	vmaster->vsid = vsid;
> > > > > +	state->vmaster = vmaster;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > > > > +{
> > > > > +	struct arm_smmu_master *master = state->master;
> > > > > +
> > > > > +	mutex_lock(&master->smmu->streams_mutex);
> > > > > +	if (state->vmaster != master->vmaster) {
> > > > > +		kfree(master->vmaster);
> > > > > +		master->vmaster = state->vmaster;
> > > > > +	}
> > > > 
> > > > Does this condition suggest that we might end up calling
> > > > `arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
> > > > commiting to a vmaster?
> > > 
> > > No. prepare() and commit() are 1:1. How is it interpreted to have
> > > "multiple times"?
> > 
> > Ohh alright. I was just confused about why do we need to check:
> > `if (state->vmaster != master->vmaster)` ?
> 
> Hmm, it's probably not necessary, since we always allocate a new
> vmaster pointer to the "state" or set a NULL.
> 
> I will clean that up a bit.

I made the following change on top of this patch (will squash):

-------------------------------------------------------------
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 364d8469a480..2c1a51c360fe 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
 
 	iommu_group_mutex_assert(state->master->dev);
 
-	if (domain->type != IOMMU_DOMAIN_NESTED)
-		return 0;
 	nested_domain = to_smmu_nested_domain(domain);
 
 	/* Skip invalid vSTE */
@@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
 {
 	struct arm_smmu_master *master = state->master;
 
-	mutex_lock(&master->smmu->streams_mutex);
-	if (state->vmaster != master->vmaster) {
-		kfree(master->vmaster);
-		master->vmaster = state->vmaster;
-	}
-	mutex_unlock(&master->smmu->streams_mutex);
-}
-
-void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
-{
 	mutex_lock(&master->smmu->streams_mutex);
 	kfree(master->vmaster);
-	master->vmaster = NULL;
+	master->vmaster = state->vmaster;
 	mutex_unlock(&master->smmu->streams_mutex);
 }
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 9e50bcee69d1..b9d0cf571da0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2833,9 +2833,11 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 	}
 
 	if (smmu_domain) {
-		ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
-		if (ret)
-			return ret;
+		if (new_domain->type == IOMMU_DOMAIN_NESTED) {
+			ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
+			if (ret)
+				return ret;
+		}
 
 		master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
 		if (!master_domain) {
@@ -3171,8 +3173,9 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 {
 	struct arm_smmu_ste ste;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_attach_state state = { .master = master };
 
-	arm_smmu_master_clear_vmaster(master);
+	arm_smmu_attach_commit_vmaster(&state);
 	arm_smmu_make_bypass_ste(master->smmu, &ste);
 	arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
 	return 0;
@@ -3192,8 +3195,9 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 {
 	struct arm_smmu_ste ste;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_attach_state state = { .master = master };
 
-	arm_smmu_master_clear_vmaster(master);
+	arm_smmu_attach_commit_vmaster(&state);
 	arm_smmu_make_abort_ste(&ste);
 	arm_smmu_attach_dev_ste(domain, dev, &ste,
 				STRTAB_STE_1_S1DSS_TERMINATE);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 85352504343b..eeec302f1b4b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1066,7 +1066,6 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
 				    struct iommu_domain *domain);
 void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
-void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
 #else
 #define arm_smmu_hw_info NULL
 #define arm_vsmmu_alloc NULL
@@ -1082,9 +1081,6 @@ static inline void
 arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
 {
 }
-static inline void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
-{
-}
 #endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */
 
 #endif /* _ARM_SMMU_V3_H */
-------------------------------------------------------------

Thanks
Nicolin