[PATCH v2 21/27] iommu/arm-smmu-v3: Put the SVA mmu notifier in the smmu_domain
Michael Shavit
mshavit at google.com
Tue Nov 7 05:28:08 PST 2023
On Thu, Nov 2, 2023 at 7:37 AM Jason Gunthorpe <jgg at nvidia.com> wrote:
> [...]
> @@ -309,24 +169,26 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> struct arm_smmu_cd target;
> struct arm_smmu_cd *cdptr;
>
> - cdptr = arm_smmu_get_cd_ptr(master, mm->pasid);
> + cdptr = arm_smmu_get_cd_ptr(master, master_domain->ssid);
> if (WARN_ON(!cdptr))
> continue;
> - arm_smmu_make_sva_cd(&target, master, NULL, smmu_mn->cd->asid);
> - arm_smmu_write_cd_entry(master, mm->pasid, cdptr, &target);
> + arm_smmu_make_sva_cd(&target, master, NULL,
> + smmu_domain->cd.asid,
> + smmu_domain->btm_invalidation);
> + arm_smmu_write_cd_entry(master, master_domain->ssid, cdptr,
> + &target);
> }
> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> - arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
> - arm_smmu_atc_inv_domain_sva(smmu_domain, mm->pasid, 0, 0);
> -
> - smmu_mn->cleared = true;
> - mutex_unlock(&sva_lock);
> + arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_domain->cd.asid);
Similar questions to patch 11 from the v1, but why is it ok to remove
the ATC invalidation here? Sure it eventually get's flushed in
arm_smmu_remove_dev_pasid, but can't the ATCs get hit in the meantime?
I'm not as familiar with ATC so likely wrong, but I was under the
impression that they can still give a translation hit after the CD is
cleared+synced.
Did you perhaps mean to remove the TLB invalidation instead (for which
it's IIUC ok to delay the invalidation to when the domain/asid is
freed, since those cache entries won't give a hit while the CD is
cleared)?
> }
>
> static void arm_smmu_mmu_notifier_free(struct mmu_notifier *mn)
> {
> - kfree(mn_to_smmu(mn));
> + struct arm_smmu_domain *smmu_domain =
> + container_of(mn, struct arm_smmu_domain, mmu_notifier);
> +
> + kfree(smmu_domain);
> }
>
> static const struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = {
> @@ -335,109 +197,6 @@ static const struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = {
> .free_notifier = arm_smmu_mmu_notifier_free,
> };
>
> -/* Allocate or get existing MMU notifier for this {domain, mm} pair */
> -static struct arm_smmu_mmu_notifier *
> -arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
> - struct mm_struct *mm)
> -{
> - int ret;
> - struct arm_smmu_ctx_desc *cd;
> - struct arm_smmu_mmu_notifier *smmu_mn;
> -
> - list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) {
> - if (smmu_mn->mn.mm == mm) {
> - refcount_inc(&smmu_mn->refs);
> - return smmu_mn;
> - }
> - }
> -
> - cd = arm_smmu_alloc_shared_cd(mm);
> - if (IS_ERR(cd))
> - return ERR_CAST(cd);
> -
> - smmu_mn = kzalloc(sizeof(*smmu_mn), GFP_KERNEL);
> - if (!smmu_mn) {
> - ret = -ENOMEM;
> - goto err_free_cd;
> - }
> -
> - refcount_set(&smmu_mn->refs, 1);
> - smmu_mn->cd = cd;
> - smmu_mn->domain = smmu_domain;
> - smmu_mn->mn.ops = &arm_smmu_mmu_notifier_ops;
> -
> - ret = mmu_notifier_register(&smmu_mn->mn, mm);
> - if (ret) {
> - kfree(smmu_mn);
> - goto err_free_cd;
> - }
> -
> - list_add(&smmu_mn->list, &smmu_domain->mmu_notifiers);
> - return smmu_mn;
> -
> -err_free_cd:
> - arm_smmu_free_shared_cd(cd);
> - return ERR_PTR(ret);
> -}
> -
> -static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
> -{
> - struct mm_struct *mm = smmu_mn->mn.mm;
> - struct arm_smmu_ctx_desc *cd = smmu_mn->cd;
> - struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
> -
> - if (!refcount_dec_and_test(&smmu_mn->refs))
> - return;
> -
> - list_del(&smmu_mn->list);
> -
> - /*
> - * If we went through clear(), we've already invalidated, and no
> - * new TLB entry can have been formed.
> - */
> - if (!smmu_mn->cleared) {
> - arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid);
> - arm_smmu_atc_inv_domain_sva(smmu_domain, mm->pasid, 0, 0);
> - }
> -
> - /* Frees smmu_mn */
> - mmu_notifier_put(&smmu_mn->mn);
> - arm_smmu_free_shared_cd(cd);
> -}
> -
> -static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
> - struct arm_smmu_cd *target)
> -{
> - int ret;
> - struct arm_smmu_bond *bond;
> - struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> - struct arm_smmu_domain *smmu_domain =
> - to_smmu_domain_safe(iommu_get_domain_for_dev(dev));
> -
> - if (!smmu_domain || smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> - return -ENODEV;
> -
> - bond = kzalloc(sizeof(*bond), GFP_KERNEL);
> - if (!bond)
> - return -ENOMEM;
> -
> - bond->mm = mm;
> -
> - bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
> - if (IS_ERR(bond->smmu_mn)) {
> - ret = PTR_ERR(bond->smmu_mn);
> - goto err_free_bond;
> - }
> -
> - list_add(&bond->list, &master->bonds);
> - arm_smmu_make_sva_cd(target, master, mm, bond->smmu_mn->cd->asid);
> - return 0;
> -
> -err_free_bond:
> - kfree(bond);
> - return ret;
> -}
> -
> bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> {
> unsigned long reg, fld;
> @@ -565,11 +324,6 @@ int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
> int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
> {
> mutex_lock(&sva_lock);
> - if (!list_empty(&master->bonds)) {
> - dev_err(master->dev, "cannot disable SVA, device is bound\n");
> - mutex_unlock(&sva_lock);
> - return -EBUSY;
> - }
> arm_smmu_master_sva_disable_iopf(master);
> master->sva_enabled = false;
> mutex_unlock(&sva_lock);
> @@ -586,59 +340,54 @@ void arm_smmu_sva_notifier_synchronize(void)
> mmu_notifier_synchronize();
> }
>
> -void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
> - struct device *dev, ioasid_t id)
> -{
> - struct mm_struct *mm = domain->mm;
> - struct arm_smmu_bond *bond = NULL, *t;
> - struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> -
> - arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
> -
> - mutex_lock(&sva_lock);
> - list_for_each_entry(t, &master->bonds, list) {
> - if (t->mm == mm) {
> - bond = t;
> - break;
> - }
> - }
> -
> - if (!WARN_ON(!bond)) {
> - list_del(&bond->list);
> - arm_smmu_mmu_notifier_put(bond->smmu_mn);
> - kfree(bond);
> - }
> - mutex_unlock(&sva_lock);
> -}
> -
> static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
> struct device *dev, ioasid_t id)
> {
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> - int ret = 0;
> - struct mm_struct *mm = domain->mm;
> struct arm_smmu_cd target;
> + int ret;
>
> - if (mm->pasid != id || !master->cd_table.used_sid)
> + /* Prevent arm_smmu_mm_release from being called while we are attaching */
> + if (!mmget_not_zero(domain->mm))
> return -EINVAL;
>
> - if (!arm_smmu_get_cd_ptr(master, id))
> - return -ENOMEM;
> + /*
> + * This does not need the arm_smmu_asid_lock because SVA domains never
> + * get reassigned
> + */
> + arm_smmu_make_sva_cd(&target, master, smmu_domain->domain.mm,
> + smmu_domain->cd.asid,
> + smmu_domain->btm_invalidation);
>
> - mutex_lock(&sva_lock);
> - ret = __arm_smmu_sva_bind(dev, mm, &target);
> - mutex_unlock(&sva_lock);
> - if (ret)
> - return ret;
> + ret = arm_smmu_set_pasid(master, to_smmu_domain(domain), id, &target);
>
> - /* This cannot fail since we preallocated the cdptr */
> - arm_smmu_set_pasid(master, to_smmu_domain(domain), id, &target);
> - return 0;
> + mmput(domain->mm);
> + return ret;
> }
>
> static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
> {
> - kfree(to_smmu_domain(domain));
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +
> + /*
> + * Ensure the ASID is empty in the iommu cache before allowing reuse.
> + */
> + arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_domain->cd.asid);
> +
> + /*
> + * Notice that the arm_smmu_mm_arch_invalidate_secondary_tlbs op can
> + * still be called/running at this point. We allow the ASID to be
> + * reused, and if there is a race then it just suffers harmless
> + * unnecessary invalidation.
> + */
> + xa_erase(&arm_smmu_asid_xa, smmu_domain->cd.asid);
> +
> + /*
> + * Actual free is defered to the SRCU callback
> + * arm_smmu_mmu_notifier_free()
> + */
> + mmu_notifier_put(&smmu_domain->mmu_notifier);
> }
>
> static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> @@ -652,6 +401,8 @@ struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> struct arm_smmu_device *smmu = master->smmu;
> struct arm_smmu_domain *smmu_domain;
> + u32 asid;
> + int ret;
>
> smmu_domain = arm_smmu_domain_alloc();
> if (!smmu_domain)
> @@ -661,5 +412,22 @@ struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,
> smmu_domain->domain.ops = &arm_smmu_sva_domain_ops;
> smmu_domain->smmu = smmu;
>
> + ret = xa_alloc(&arm_smmu_asid_xa, &asid, smmu_domain,
> + XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL);
> + if (ret)
> + goto err_free;
> +
> + smmu_domain->cd.asid = asid;
> + smmu_domain->mmu_notifier.ops = &arm_smmu_mmu_notifier_ops;
> + ret = mmu_notifier_register(&smmu_domain->mmu_notifier, mm);
> + if (ret)
> + goto err_asid;
> +
> return &smmu_domain->domain;
> +
> +err_asid:
> + xa_erase(&arm_smmu_asid_xa, smmu_domain->cd.asid);
> +err_free:
> + kfree(smmu_domain);
> + return ERR_PTR(ret);
> }
> 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 85fc3064675931..c221ab138ebb87 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1339,22 +1339,6 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master)
> cd_table->cdtab = NULL;
> }
>
> -bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd)
> -{
> - bool free;
> - struct arm_smmu_ctx_desc *old_cd;
> -
> - if (!cd->asid)
> - return false;
> -
> - free = refcount_dec_and_test(&cd->refs);
> - if (free) {
> - old_cd = xa_erase(&arm_smmu_asid_xa, cd->asid);
> - WARN_ON(old_cd != cd);
> - }
> - return free;
> -}
> -
> /* Stream table manipulation functions */
> static void
> arm_smmu_write_strtab_l1_desc(__le64 *dst, struct arm_smmu_strtab_l1_desc *desc)
> @@ -1980,8 +1964,8 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
> return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
> }
>
> -static int __arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> - ioasid_t ssid, unsigned long iova, size_t size)
> +int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> + unsigned long iova, size_t size)
> {
> struct arm_smmu_master_domain *master_domain;
> int i;
> @@ -2019,15 +2003,7 @@ static int __arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> if (!master->ats_enabled)
> continue;
>
> - /*
> - * Non-zero ssid means SVA is co-opting the S1 domain to issue
> - * invalidations for SVA PASIDs.
> - */
> - if (ssid != IOMMU_NO_PASID)
> - arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
> - else
> - arm_smmu_atc_inv_to_cmd(master_domain->ssid, iova, size,
> - &cmd);
> + arm_smmu_atc_inv_to_cmd(master_domain->ssid, iova, size, &cmd);
>
> for (i = 0; i < master->num_streams; i++) {
> cmd.atc.sid = master->streams[i].id;
> @@ -2039,19 +2015,6 @@ static int __arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
> }
>
> -static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> - unsigned long iova, size_t size)
> -{
> - return __arm_smmu_atc_inv_domain(smmu_domain, IOMMU_NO_PASID, iova,
> - size);
> -}
> -
> -int arm_smmu_atc_inv_domain_sva(struct arm_smmu_domain *smmu_domain,
> - ioasid_t ssid, unsigned long iova, size_t size)
> -{
> - return __arm_smmu_atc_inv_domain(smmu_domain, ssid, iova, size);
> -}
> -
> /* IO_PGTABLE API */
> static void arm_smmu_tlb_inv_context(void *cookie)
> {
> @@ -2240,7 +2203,6 @@ struct arm_smmu_domain *arm_smmu_domain_alloc(void)
> mutex_init(&smmu_domain->init_mutex);
> INIT_LIST_HEAD(&smmu_domain->devices);
> spin_lock_init(&smmu_domain->devices_lock);
> - INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
>
> return smmu_domain;
> }
> @@ -2281,7 +2243,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
> if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> /* Prevent SVA from touching the CD while we're freeing it */
> mutex_lock(&arm_smmu_asid_lock);
> - arm_smmu_free_asid(&smmu_domain->cd);
> + xa_erase(&arm_smmu_asid_xa, smmu_domain->cd.asid);
> mutex_unlock(&arm_smmu_asid_lock);
> } else {
> struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
> @@ -2299,11 +2261,9 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_device *smmu,
> u32 asid;
> struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
>
> - refcount_set(&cd->refs, 1);
> -
> /* Prevent SVA from modifying the ASID until it is written to the CD */
> mutex_lock(&arm_smmu_asid_lock);
> - ret = xa_alloc(&arm_smmu_asid_xa, &asid, cd,
> + ret = xa_alloc(&arm_smmu_asid_xa, &asid, smmu_domain,
> XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL);
> cd->asid = (u16)asid;
> mutex_unlock(&arm_smmu_asid_lock);
> @@ -2715,7 +2675,10 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> struct attach_state state;
> int ret;
>
> - if (!sid_smmu_domain || sid_smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> + if (smmu_domain->smmu != master->smmu)
> + return -EINVAL;
> +
> + if (!sid_smmu_domain || !master->cd_table.used_sid)
> return -ENODEV;
>
> cdptr = arm_smmu_get_cd_ptr(master, pasid);
> @@ -2736,9 +2699,18 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> return 0;
> }
>
> -void arm_smmu_remove_pasid(struct arm_smmu_master *master,
> - struct arm_smmu_domain *smmu_domain, ioasid_t pasid)
> +static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> {
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> + struct arm_smmu_domain *smmu_domain;
> + struct iommu_domain *domain;
> +
> + domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
> + if (WARN_ON(IS_ERR(domain)) || !domain)
> + return;
> +
> + smmu_domain = to_smmu_domain(domain);
> +
> mutex_lock(&arm_smmu_asid_lock);
> arm_smmu_clear_cd(master, pasid);
> if (master->ats_enabled)
> @@ -3032,7 +3004,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>
> master->dev = dev;
> master->smmu = smmu;
> - INIT_LIST_HEAD(&master->bonds);
> dev_iommu_priv_set(dev, master);
>
> ret = arm_smmu_insert_master(smmu, master);
> @@ -3214,17 +3185,6 @@ static int arm_smmu_def_domain_type(struct device *dev)
> return 0;
> }
>
> -static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> -{
> - struct iommu_domain *domain;
> -
> - domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
> - if (WARN_ON(IS_ERR(domain)) || !domain)
> - return;
> -
> - arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
> -}
> -
> static struct iommu_ops arm_smmu_ops = {
> .identity_domain = &arm_smmu_identity_domain,
> .blocked_domain = &arm_smmu_blocked_domain,
> 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 48871c8ee8c88c..a229ad0adf6a49 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -587,9 +587,6 @@ struct arm_smmu_strtab_l1_desc {
>
> struct arm_smmu_ctx_desc {
> u16 asid;
> -
> - refcount_t refs;
> - struct mm_struct *mm;
> };
>
> struct arm_smmu_l1_ctx_desc {
> @@ -713,7 +710,6 @@ struct arm_smmu_master {
> bool stall_enabled;
> bool sva_enabled;
> bool iopf_enabled;
> - struct list_head bonds;
> unsigned int ssid_bits;
> };
>
> @@ -742,7 +738,8 @@ struct arm_smmu_domain {
> struct list_head devices;
> spinlock_t devices_lock;
>
> - struct list_head mmu_notifiers;
> + struct mmu_notifier mmu_notifier;
> + bool btm_invalidation;
> };
>
> struct arm_smmu_master_domain {
> @@ -796,9 +793,8 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
> void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
> size_t granule, bool leaf,
> struct arm_smmu_domain *smmu_domain);
> -bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd);
> -int arm_smmu_atc_inv_domain_sva(struct arm_smmu_domain *smmu_domain,
> - ioasid_t ssid, unsigned long iova, size_t size);
> +int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
> + unsigned long iova, size_t size);
>
> #ifdef CONFIG_ARM_SMMU_V3_SVA
> bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
> @@ -810,8 +806,6 @@ bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
> void arm_smmu_sva_notifier_synchronize(void);
> struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,
> struct mm_struct *mm);
> -void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
> - struct device *dev, ioasid_t id);
> #else /* CONFIG_ARM_SMMU_V3_SVA */
> static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> {
> --
> 2.42.0
>
>
More information about the linux-arm-kernel
mailing list