[PATCH 0/5] amd/iommu: Cleanup and fixes

[PATCH 0/5] amd/iommu: Cleanup and fixes

[Vasant Hegde]

This series contains few fixes like better handling of error path, fixing
sparse warning and trivial cleanup like removing outdated, etc.

Vasant Hegde (5):
  iommu/amd: Free domain id in error path
  iommu/amd: Protect domain update with domain_lock
  iommu/amd: Free domain ID after domain_flush_pages
  iommu/amd: Remove outdated comment
  iommu/amd: Fix sparse warning

 drivers/iommu/amd/amd_iommu_types.h |  2 ++
 drivers/iommu/amd/init.c            |  2 --
 drivers/iommu/amd/iommu.c           | 22 ++++++++++++++--------
 3 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/5] iommu/amd: Free domain id in error path

[Vasant Hegde]

Call domain_id_free() in error path.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6bfcac8de1f4..c55f4a129b1e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2027,8 +2027,10 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 
 	if (mode != PAGE_MODE_NONE) {
 		pt_root = (void *)get_zeroed_page(GFP_KERNEL);
-		if (!pt_root)
+		if (!pt_root) {
+			domain_id_free(domain->id);
 			return -ENOMEM;
+		}
 	}
 
 	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
@@ -2092,8 +2094,10 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 		goto out_err;
 
 	pgtbl_ops = alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl_cfg, domain);
-	if (!pgtbl_ops)
+	if (!pgtbl_ops) {
+		domain_id_free(domain->id);
 		goto out_err;
+	}
 
 	return domain;
 out_err:
-- 
2.31.1

[PATCH 2/5] iommu/amd: Protect domain update with domain_lock

[Vasant Hegde]

In protection_domain_free() path it calls amd_iommu_domain_update() to
update IOMMU.
  protection_domain_free -> free_io_pgtable_ops ->
    v1_free_pgtable -> amd_iommu_domain_update

Make sure its protected using domain->lock.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c55f4a129b1e..f49980956220 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2001,15 +2001,21 @@ static void cleanup_domain(struct protection_domain *domain)
 
 static void protection_domain_free(struct protection_domain *domain)
 {
+	unsigned long flags;
+
 	if (!domain)
 		return;
 
 	if (domain->id)
 		domain_id_free(domain->id);
 
+	spin_lock_irqsave(&domain->lock, flags);
+
 	if (domain->iop.pgtbl_cfg.tlb)
 		free_io_pgtable_ops(&domain->iop.iop.ops);
 
+	spin_unlock_irqrestore(&domain->lock, flags);
+
 	kfree(domain);
 }
 
-- 
2.31.1

Re: [PATCH 2/5] iommu/amd: Protect domain update with domain_lock

[Joerg Roedel]

Hi Vasant,

On Mon, Sep 12, 2022 at 06:32:45AM +0000, Vasant Hegde wrote:
> In protection_domain_free() path it calls amd_iommu_domain_update() to
> update IOMMU.
>   protection_domain_free -> free_io_pgtable_ops ->
>     v1_free_pgtable -> amd_iommu_domain_update
> 
> Make sure its protected using domain->lock.

A concurrent access to the domain at this point would be a bug in
itself, and the locking wouldn't help here either because the domain
including the lock is freed right after release.

Do you see and concurrent accesses to the domain at this point?

Regards,

	Joerg

Re: [PATCH 2/5] iommu/amd: Protect domain update with domain_lock

[Vasant Hegde]

Hello Joerg,


On 9/26/2022 4:52 PM, Joerg Roedel wrote:
> Hi Vasant,
> 
> On Mon, Sep 12, 2022 at 06:32:45AM +0000, Vasant Hegde wrote:
>> In protection_domain_free() path it calls amd_iommu_domain_update() to
>> update IOMMU.
>>   protection_domain_free -> free_io_pgtable_ops ->
>>     v1_free_pgtable -> amd_iommu_domain_update
>>
>> Make sure its protected using domain->lock.
> 
> A concurrent access to the domain at this point would be a bug in
> itself, and the locking wouldn't help here either because the domain
> including the lock is freed right after release.
> 
> Do you see and concurrent accesses to the domain at this point?

I didn't see any concurrent access. I was going through code and
thought lock is required here.

What you said is makes sense. We can drop this patch.

-Vasant

[PATCH 3/5] iommu/amd: Free domain ID after domain_flush_pages

[Vasant Hegde]

free_io_pgtable_ops() path uses domain ID to flush pages. Hence
free domain ID after flushing everything.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index f49980956220..4509c9c4ee20 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2006,9 +2006,6 @@ static void protection_domain_free(struct protection_domain *domain)
 	if (!domain)
 		return;
 
-	if (domain->id)
-		domain_id_free(domain->id);
-
 	spin_lock_irqsave(&domain->lock, flags);
 
 	if (domain->iop.pgtbl_cfg.tlb)
@@ -2016,6 +2013,9 @@ static void protection_domain_free(struct protection_domain *domain)
 
 	spin_unlock_irqrestore(&domain->lock, flags);
 
+	if (domain->id)
+		domain_id_free(domain->id);
+
 	kfree(domain);
 }
 
-- 
2.31.1

[PATCH 4/5] iommu/amd: Remove outdated comment

[Vasant Hegde]

Comment is not related to amd_iommu_ops variable.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 4509c9c4ee20..c589832695a7 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -66,10 +66,6 @@ LIST_HEAD(ioapic_map);
 LIST_HEAD(hpet_map);
 LIST_HEAD(acpihid_map);
 
-/*
- * Domain for untranslated devices - only allocated
- * if iommu=pt passed on kernel cmd line.
- */
 const struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
-- 
2.31.1

[PATCH 5/5] iommu/amd: Fix sparse warning

[Vasant Hegde]

CHECK   drivers/iommu/amd/iommu.c
drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 2 ++
 drivers/iommu/amd/init.c            | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 1beed57fc35d..1d0a70c85333 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -461,6 +461,8 @@ struct irq_remap_table {
 /* Interrupt remapping feature used? */
 extern bool amd_iommu_irq_remap;
 
+extern const struct iommu_ops amd_iommu_ops;
+
 /* IVRS indicates that pre-boot remapping was enabled */
 extern bool amdr_ivrs_remap_support;
 
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 688cf8387b0b..a515e837adf1 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -95,8 +95,6 @@
  * out of it.
  */
 
-extern const struct iommu_ops amd_iommu_ops;
-
 /*
  * structure describing one IOMMU in the ACPI table. Typically followed by one
  * or more ivhd_entrys.
-- 
2.31.1

Re: [PATCH 0/5] amd/iommu: Cleanup and fixes

[Joerg Roedel]

On Mon, Sep 12, 2022 at 06:32:43AM +0000, Vasant Hegde wrote:
> This series contains few fixes like better handling of error path, fixing
> sparse warning and trivial cleanup like removing outdated, etc.
> 
> Vasant Hegde (5):
>   iommu/amd: Free domain id in error path
>   iommu/amd: Protect domain update with domain_lock
>   iommu/amd: Free domain ID after domain_flush_pages
>   iommu/amd: Remove outdated comment
>   iommu/amd: Fix sparse warning

Applied patch 1 and 3-5, thanks Vasant.