| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZMkULTvwIIE3zo5+@nvidia.com>
Date: Tue, 1 Aug 2023 11:18:21 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Michael Shavit <mshavit@...gle.com>
Cc: iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, will@...nel.org,
robin.murphy@....com, nicolinc@...dia.com, jean-philippe@...aro.org
Subject: Re: [PATCH v2 6/8] iommu/arm-smmu-v3: Refactor write_ctx_desc
On Mon, Jul 31, 2023 at 06:48:16PM +0800, Michael Shavit wrote:
> Update arm_smmu_write_ctx_desc and downstream functions to operate on
> a master instead of an smmu domain. We expect arm_smmu_write_ctx_desc()
> to only be called to write a CD entry into a CD table owned by the
> master. Under the hood, arm_smmu_write_ctx_desc still fetches the CD
> table from the domain that is attached to the master, but a subsequent
> commit will move that table's ownership to the master.
>
> Note that this change isn't a nop refactor since SVA will call
> arm_smmu_write_ctx_desc in a loop for every master the domain is
> attached to despite the fact that they all share the same CD table. This
> loop may look weird but becomes necessary when the CD table becomes
> per-master in a subsequent commit.
>
> Signed-off-by: Michael Shavit <mshavit@...gle.com>
> ---
>
> Changes in v2:
> - minor style fixes
>
> Changes in v1:
> - arm_smmu_write_ctx_desc now get's the CD table to write to from the
> master parameter instead of a distinct parameter. This works well
> because the CD table being written to should always be owned by the
> master by the end of this series. This version no longer allows master
> to be NULL.
>
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 33 +++++++++--
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++-----------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +-
> 3 files changed, 53 insertions(+), 41 deletions(-)
>
> 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 968559d625c40..8242ee3405f2d 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
> @@ -45,9 +45,11 @@ static struct arm_smmu_ctx_desc *
> arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
> {
> int ret;
> + unsigned long flags;
> u32 new_asid;
> struct arm_smmu_ctx_desc *cd;
> struct arm_smmu_device *smmu;
> + struct arm_smmu_master *master;
> struct arm_smmu_domain *smmu_domain;
>
> cd = xa_load(&arm_smmu_asid_xa, asid);
> @@ -80,7 +82,11 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
> * be some overlap between use of both ASIDs, until we invalidate the
> * TLB.
> */
> - arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
> + spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> + list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> + arm_smmu_write_ctx_desc(master, 0, cd);
> + }
I think it is typical kernel style to not include the single statement
{}
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> /* Invalidate TLB entries previously associated with that context */
> arm_smmu_tlb_inv_asid(smmu, asid);
> @@ -211,6 +217,8 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn);
> struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
> + struct arm_smmu_master *master;
> + unsigned long flags;
>
> mutex_lock(&sva_lock);
> if (smmu_mn->cleared) {
> @@ -222,7 +230,11 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events,
> * but disable translation.
> */
> - arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, &quiet_cd);
> + spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> + list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> + arm_smmu_write_ctx_desc(master, mm->pasid, &quiet_cd);
> + }
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
And here
> arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
> arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
> @@ -248,7 +260,9 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
> struct mm_struct *mm)
> {
> int ret;
> + unsigned long flags;
> struct arm_smmu_ctx_desc *cd;
> + struct arm_smmu_master *master;
> struct arm_smmu_mmu_notifier *smmu_mn;
>
> list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) {
> @@ -279,7 +293,11 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
> goto err_free_cd;
> }
>
> - ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd);
> + spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> + list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> + ret = arm_smmu_write_ctx_desc(master, mm->pasid, cd);
> + }
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
And here..
> if (ret)
> goto err_put_notifier;
>
> @@ -296,6 +314,8 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
>
> static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
> {
> + unsigned long flags;
> + struct arm_smmu_master *master;
> 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;
> @@ -304,7 +324,12 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
> return;
>
> list_del(&smmu_mn->list);
> - arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL);
> +
> + spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> + list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> + arm_smmu_write_ctx_desc(master, mm->pasid, NULL);
> + }
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
And here..
You know, you should try to keep the function instead of duplicating
these
arm_smmu_write_ctx_desc_devices()
And put the four lines in there?
> @@ -987,19 +985,14 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> },
> };
>
> - if (!smmu_domain->cd_table.installed)
> + if (!master->domain->cd_table.installed)
> return;
BTW, do you have locking for this? I didn't check carefully
Jason
Powered by blists - more mailing lists