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