* [PATCH 0/5] amd/iommu: Cleanup and fixes
@ 2022-09-12 6:32 Vasant Hegde
2022-09-12 6:32 ` [PATCH 1/5] iommu/amd: Free domain id in error path Vasant Hegde
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/5] iommu/amd: Free domain id in error path
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
@ 2022-09-12 6:32 ` Vasant Hegde
2022-09-12 6:32 ` [PATCH 2/5] iommu/amd: Protect domain update with domain_lock Vasant Hegde
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/5] iommu/amd: Protect domain update with domain_lock
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
2022-09-12 6:32 ` [PATCH 1/5] iommu/amd: Free domain id in error path Vasant Hegde
@ 2022-09-12 6:32 ` Vasant Hegde
2022-09-26 11:22 ` Joerg Roedel
2022-09-12 6:32 ` [PATCH 3/5] iommu/amd: Free domain ID after domain_flush_pages Vasant Hegde
` (3 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/5] iommu/amd: Free domain ID after domain_flush_pages
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
2022-09-12 6:32 ` [PATCH 1/5] iommu/amd: Free domain id in error path Vasant Hegde
2022-09-12 6:32 ` [PATCH 2/5] iommu/amd: Protect domain update with domain_lock Vasant Hegde
@ 2022-09-12 6:32 ` Vasant Hegde
2022-09-12 6:32 ` [PATCH 4/5] iommu/amd: Remove outdated comment Vasant Hegde
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/5] iommu/amd: Remove outdated comment
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
` (2 preceding siblings ...)
2022-09-12 6:32 ` [PATCH 3/5] iommu/amd: Free domain ID after domain_flush_pages Vasant Hegde
@ 2022-09-12 6:32 ` Vasant Hegde
2022-09-12 6:32 ` [PATCH 5/5] iommu/amd: Fix sparse warning Vasant Hegde
2022-09-26 11:29 ` [PATCH 0/5] amd/iommu: Cleanup and fixes Joerg Roedel
5 siblings, 0 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/5] iommu/amd: Fix sparse warning
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
` (3 preceding siblings ...)
2022-09-12 6:32 ` [PATCH 4/5] iommu/amd: Remove outdated comment Vasant Hegde
@ 2022-09-12 6:32 ` Vasant Hegde
2022-09-26 11:29 ` [PATCH 0/5] amd/iommu: Cleanup and fixes Joerg Roedel
5 siblings, 0 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-12 6:32 UTC (permalink / raw)
To: iommu, joro; +Cc: robin.murphy, suravee.suthikulpanit, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/5] iommu/amd: Protect domain update with domain_lock
2022-09-12 6:32 ` [PATCH 2/5] iommu/amd: Protect domain update with domain_lock Vasant Hegde
@ 2022-09-26 11:22 ` Joerg Roedel
2022-09-28 5:39 ` Vasant Hegde
0 siblings, 1 reply; 9+ messages in thread
From: Joerg Roedel @ 2022-09-26 11:22 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, robin.murphy, suravee.suthikulpanit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] amd/iommu: Cleanup and fixes
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
` (4 preceding siblings ...)
2022-09-12 6:32 ` [PATCH 5/5] iommu/amd: Fix sparse warning Vasant Hegde
@ 2022-09-26 11:29 ` Joerg Roedel
5 siblings, 0 replies; 9+ messages in thread
From: Joerg Roedel @ 2022-09-26 11:29 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, robin.murphy, suravee.suthikulpanit
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/5] iommu/amd: Protect domain update with domain_lock
2022-09-26 11:22 ` Joerg Roedel
@ 2022-09-28 5:39 ` Vasant Hegde
0 siblings, 0 replies; 9+ messages in thread
From: Vasant Hegde @ 2022-09-28 5:39 UTC (permalink / raw)
To: Joerg Roedel; +Cc: iommu, robin.murphy, suravee.suthikulpanit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-28 5:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-12 6:32 [PATCH 0/5] amd/iommu: Cleanup and fixes Vasant Hegde
2022-09-12 6:32 ` [PATCH 1/5] iommu/amd: Free domain id in error path Vasant Hegde
2022-09-12 6:32 ` [PATCH 2/5] iommu/amd: Protect domain update with domain_lock Vasant Hegde
2022-09-26 11:22 ` Joerg Roedel
2022-09-28 5:39 ` Vasant Hegde
2022-09-12 6:32 ` [PATCH 3/5] iommu/amd: Free domain ID after domain_flush_pages Vasant Hegde
2022-09-12 6:32 ` [PATCH 4/5] iommu/amd: Remove outdated comment Vasant Hegde
2022-09-12 6:32 ` [PATCH 5/5] iommu/amd: Fix sparse warning Vasant Hegde
2022-09-26 11:29 ` [PATCH 0/5] amd/iommu: Cleanup and fixes Joerg Roedel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).