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?
- 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;
- mutex_unlock(&master->smmu->streams_mutex);
+}
static int arm_smmu_attach_dev_nested(struct iommu_domain *domain, struct device *dev) { 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 358072b4e293..9e50bcee69d1 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2803,6 +2803,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state, struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(new_domain); unsigned long flags;
- int ret;
/* * arm_smmu_share_asid() must not see two domains pointing to the same @@ -2832,9 +2833,15 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state, } if (smmu_domain) {
ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
IMO, this adds a little confusion for folks not using iommufd.
I guess it'd be cleaner if we invoke this below within the: `if (new_domain->type == IOMMU_DOMAIN_NESTED)` condition instead of simply returning from the function if the new_domain->type isn't NESTED.
if (ret)return ret;- master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
if (!master_domain)
if (!master_domain) {kfree(state->vmaster); return -ENOMEM;}@@ -2861,6 +2868,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state, spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); kfree(master_domain);
kfree(state->vmaster); return -EINVAL;@@ -2893,6 +2901,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state) lockdep_assert_held(&arm_smmu_asid_lock);
- arm_smmu_attach_commit_vmaster(state);
- if (state->ats_enabled && !master->ats_enabled) { arm_smmu_enable_ats(master); } else if (state->ats_enabled && master->ats_enabled) {
@@ -3162,6 +3172,7 @@ 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);
- arm_smmu_master_clear_vmaster(master); arm_smmu_make_bypass_ste(master->smmu, &ste); arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS); return 0;
@@ -3180,7 +3191,9 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain, struct device *dev) { struct arm_smmu_ste ste;
- struct arm_smmu_master *master = dev_iommu_priv_get(dev);
- arm_smmu_master_clear_vmaster(master); arm_smmu_make_abort_ste(&ste); arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_TERMINATE);
Thanks, Praan