* [PATCH v3 01/16] iommu/amd: Remove unused amd_io_pgtable.pt_root variable
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 02/16] iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h Vasant Hegde
` (14 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
It has been no longer used since the commit 6eedb59c18a3 ("iommu/amd:
Remove amd_iommu_domain_get_pgtable").
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/amd_iommu.h | 1 -
drivers/iommu/amd/amd_iommu_types.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index f23ad6043f04..6bf55de9fbeb 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -108,7 +108,6 @@ static inline void *iommu_phys_to_virt(unsigned long paddr)
static inline
void amd_iommu_domain_set_pt_root(struct protection_domain *domain, u64 root)
{
- atomic64_set(&domain->iop.pt_root, root);
domain->iop.root = (u64 *)(root & PAGE_MASK);
domain->iop.mode = root & 7; /* lowest 3 bits encode pgtable mode */
}
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 781ab9c2ea70..1930e918188f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -544,7 +544,6 @@ struct amd_io_pgtable {
struct io_pgtable iop;
int mode;
u64 *root;
- atomic64_t pt_root; /* pgtable root and pgtable mode */
u64 *pgd; /* v2 pgtable pgd pointer */
};
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 02/16] iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 01/16] iommu/amd: Remove unused amd_io_pgtable.pt_root variable Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 03/16] iommu/amd: Consolidate logic to allocate protection domain Vasant Hegde
` (13 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
To allow inclusion in other files in subsequent patches.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/amd_iommu_types.h | 4 ++++
drivers/iommu/amd/init.c | 10 ++++------
drivers/iommu/amd/iommu.c | 2 --
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 1930e918188f..13c582a2454a 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -451,6 +451,10 @@
#define PD_IOMMUV2_MASK BIT(3) /* domain has gcr3 table */
#define PD_GIOV_MASK BIT(4) /* domain enable GIOV support */
+/* Timeout stuff */
+#define LOOP_TIMEOUT 100000
+#define MMIO_STATUS_TIMEOUT 2000000
+
extern bool amd_iommu_dump;
#define DUMP_printk(format, arg...) \
do { \
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 45efb7e5d725..852e40b13d20 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -83,8 +83,6 @@
#define ACPI_DEVFLAG_LINT1 0x80
#define ACPI_DEVFLAG_ATSDIS 0x10000000
-#define LOOP_TIMEOUT 2000000
-
#define IVRS_GET_SBDF_ID(seg, bus, dev, fn) (((seg & 0xffff) << 16) | ((bus & 0xff) << 8) \
| ((dev & 0x1f) << 3) | (fn & 0x7))
@@ -985,14 +983,14 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
iommu_feature_enable(iommu, CONTROL_GAINT_EN);
iommu_feature_enable(iommu, CONTROL_GALOG_EN);
- for (i = 0; i < LOOP_TIMEOUT; ++i) {
+ for (i = 0; i < MMIO_STATUS_TIMEOUT; ++i) {
status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
if (status & (MMIO_STATUS_GALOG_RUN_MASK))
break;
udelay(10);
}
- if (WARN_ON(i >= LOOP_TIMEOUT))
+ if (WARN_ON(i >= MMIO_STATUS_TIMEOUT))
return -EINVAL;
return 0;
@@ -2900,14 +2898,14 @@ static void enable_iommus_vapic(void)
* Need to set and poll check the GALOGRun bit to zero before
* we can set/ modify GA Log registers safely.
*/
- for (i = 0; i < LOOP_TIMEOUT; ++i) {
+ for (i = 0; i < MMIO_STATUS_TIMEOUT; ++i) {
status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
break;
udelay(10);
}
- if (WARN_ON(i >= LOOP_TIMEOUT))
+ if (WARN_ON(i >= MMIO_STATUS_TIMEOUT))
return;
}
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 56b6cf8bf03f..dd586f69a9d8 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -44,8 +44,6 @@
#define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28))
-#define LOOP_TIMEOUT 100000
-
/* IO virtual address start page frame number */
#define IOVA_START_PFN (1)
#define IOVA_PFN(addr) ((addr) >> PAGE_SHIFT)
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 03/16] iommu/amd: Consolidate logic to allocate protection domain
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 01/16] iommu/amd: Remove unused amd_io_pgtable.pt_root variable Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 02/16] iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 04/16] iommu/amd: Refactor protection domain allocation code Vasant Hegde
` (12 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Move the logic into the common caller function to simplify the code.
No functional changes intended.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/iommu.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index dd586f69a9d8..c2cb541b0553 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2046,12 +2046,6 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
BUG_ON(mode < PAGE_MODE_NONE || mode > PAGE_MODE_6_LEVEL);
- spin_lock_init(&domain->lock);
- domain->id = domain_id_alloc();
- if (!domain->id)
- return -ENOMEM;
- INIT_LIST_HEAD(&domain->dev_list);
-
if (mode != PAGE_MODE_NONE) {
pt_root = (void *)get_zeroed_page(GFP_KERNEL);
if (!pt_root) {
@@ -2067,12 +2061,6 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
static int protection_domain_init_v2(struct protection_domain *domain)
{
- spin_lock_init(&domain->lock);
- domain->id = domain_id_alloc();
- if (!domain->id)
- return -ENOMEM;
- INIT_LIST_HEAD(&domain->dev_list);
-
domain->flags |= PD_GIOV_MASK;
domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
@@ -2112,6 +2100,13 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
if (!domain)
return NULL;
+ domain->id = domain_id_alloc();
+ if (!domain->id)
+ goto out_err;
+
+ spin_lock_init(&domain->lock);
+ INIT_LIST_HEAD(&domain->dev_list);
+
switch (pgtable) {
case AMD_IOMMU_V1:
ret = protection_domain_init_v1(domain, mode);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 04/16] iommu/amd: Refactor protection domain allocation code
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (2 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 03/16] iommu/amd: Consolidate logic to allocate protection domain Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 05/16] iommu/amd/iommu_v2: Use protection_domain in struct device_state Vasant Hegde
` (11 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
To replace if-else with switch-case statement due to increasing number of
domain types.
No functional changes intended.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/iommu.c | 45 +++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c2cb541b0553..d5569eec0fe9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2078,24 +2078,8 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
struct io_pgtable_ops *pgtbl_ops;
struct protection_domain *domain;
int pgtable;
- int mode = DEFAULT_PGTABLE_LEVEL;
int ret;
- /*
- * Force IOMMU v1 page table when iommu=pt and
- * when allocating domain for pass-through devices.
- */
- if (type == IOMMU_DOMAIN_IDENTITY) {
- pgtable = AMD_IOMMU_V1;
- mode = PAGE_MODE_NONE;
- } else if (type == IOMMU_DOMAIN_UNMANAGED) {
- pgtable = AMD_IOMMU_V1;
- } else if (type == IOMMU_DOMAIN_DMA || type == IOMMU_DOMAIN_DMA_FQ) {
- pgtable = amd_iommu_pgtable;
- } else {
- return NULL;
- }
-
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;
@@ -2106,27 +2090,42 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
spin_lock_init(&domain->lock);
INIT_LIST_HEAD(&domain->dev_list);
+ domain->nid = NUMA_NO_NODE;
+
+ switch (type) {
+ /* No need to allocate io pgtable ops in passthrough mode */
+ case IOMMU_DOMAIN_IDENTITY:
+ return domain;
+ case IOMMU_DOMAIN_DMA:
+ case IOMMU_DOMAIN_DMA_FQ:
+ pgtable = amd_iommu_pgtable;
+ break;
+ /*
+ * Force IOMMU v1 page table when allocating
+ * domain for pass-through devices.
+ */
+ case IOMMU_DOMAIN_UNMANAGED:
+ pgtable = AMD_IOMMU_V1;
+ break;
+ default:
+ goto out_err;
+ }
switch (pgtable) {
case AMD_IOMMU_V1:
- ret = protection_domain_init_v1(domain, mode);
+ ret = protection_domain_init_v1(domain, DEFAULT_PGTABLE_LEVEL);
break;
case AMD_IOMMU_V2:
ret = protection_domain_init_v2(domain);
break;
default:
ret = -EINVAL;
+ break;
}
if (ret)
goto out_err;
- /* No need to allocate io pgtable ops in passthrough mode */
- if (type == IOMMU_DOMAIN_IDENTITY)
- return domain;
-
- domain->nid = NUMA_NO_NODE;
-
pgtbl_ops = alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl_cfg, domain);
if (!pgtbl_ops) {
domain_id_free(domain->id);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 05/16] iommu/amd/iommu_v2: Use protection_domain in struct device_state
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (3 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 04/16] iommu/amd: Refactor protection domain allocation code Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table Vasant Hegde
` (10 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
Replace struct iommu_domain with struct protection_domain to simplify code
since the protection_domain contains AMD IOMMU specific information.
No functional changes intended.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/iommu_v2.c | 38 ++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index c5825e0a6770..6a423928d5b5 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -55,7 +55,7 @@ struct device_state {
atomic_t count;
struct pci_dev *pdev;
struct pasid_state **states;
- struct iommu_domain *domain;
+ struct protection_domain *pdom;
int pasid_levels;
int max_pasids;
amd_iommu_invalid_ppr_cb inv_ppr_cb;
@@ -112,6 +112,7 @@ static struct device_state *get_device_state(u32 sbdf)
static void free_device_state(struct device_state *dev_state)
{
struct iommu_group *group;
+ struct iommu_domain *domain = &dev_state->pdom->domain;
/* Get rid of any remaining pasid states */
free_pasid_states(dev_state);
@@ -130,12 +131,12 @@ static void free_device_state(struct device_state *dev_state)
if (WARN_ON(!group))
return;
- iommu_detach_group(dev_state->domain, group);
+ iommu_detach_group(domain, group);
iommu_group_put(group);
/* Everything is down now, free the IOMMUv2 domain */
- iommu_domain_free(dev_state->domain);
+ iommu_domain_free(domain);
/* Finally get rid of the device-state */
kfree(dev_state);
@@ -269,9 +270,7 @@ static void put_pasid_state_wait(struct pasid_state *pasid_state)
static void unbind_pasid(struct pasid_state *pasid_state)
{
- struct iommu_domain *domain;
-
- domain = pasid_state->device_state->domain;
+ struct device_state *dev_state = pasid_state->device_state;
/*
* Mark pasid_state as invalid, no more faults will we added to the
@@ -283,7 +282,7 @@ static void unbind_pasid(struct pasid_state *pasid_state)
smp_wmb();
/* After this the device/pasid can't access the mm anymore */
- amd_iommu_domain_clear_gcr3(domain, pasid_state->pasid);
+ amd_iommu_domain_clear_gcr3(&dev_state->pdom->domain, pasid_state->pasid);
/* Make sure no more pending faults are in the queue */
flush_workqueue(iommu_wq);
@@ -369,10 +368,10 @@ static void mn_invalidate_range(struct mmu_notifier *mn,
dev_state = pasid_state->device_state;
if ((start ^ (end - 1)) < PAGE_SIZE)
- amd_iommu_flush_page(dev_state->domain, pasid_state->pasid,
+ amd_iommu_flush_page(&dev_state->pdom->domain, pasid_state->pasid,
start);
else
- amd_iommu_flush_tlb(dev_state->domain, pasid_state->pasid);
+ amd_iommu_flush_tlb(&dev_state->pdom->domain, pasid_state->pasid);
}
static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -651,7 +650,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, u32 pasid,
if (ret)
goto out_unregister;
- ret = amd_iommu_domain_set_gcr3(dev_state->domain, pasid,
+ ret = amd_iommu_domain_set_gcr3(&dev_state->pdom->domain, pasid,
__pa(pasid_state->mm->pgd));
if (ret)
goto out_clear_state;
@@ -735,6 +734,7 @@ EXPORT_SYMBOL(amd_iommu_unbind_pasid);
int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
{
+ struct iommu_domain *domain;
struct device_state *dev_state;
struct iommu_group *group;
unsigned long flags;
@@ -779,15 +779,19 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
if (dev_state->states == NULL)
goto out_free_dev_state;
- dev_state->domain = iommu_domain_alloc(&pci_bus_type);
- if (dev_state->domain == NULL)
+ domain = iommu_domain_alloc(&pci_bus_type);
+ if (domain == NULL)
goto out_free_states;
+ /* Retrieve new protection_domain that has just been allocated */
+ dev_state->pdom = container_of(domain,
+ struct protection_domain, domain);
+
/* See iommu_is_default_domain() */
- dev_state->domain->type = IOMMU_DOMAIN_IDENTITY;
- amd_iommu_domain_direct_map(dev_state->domain);
+ domain->type = IOMMU_DOMAIN_IDENTITY;
+ amd_iommu_domain_direct_map(&dev_state->pdom->domain);
- ret = amd_iommu_domain_enable_v2(dev_state->domain, pasids);
+ ret = amd_iommu_domain_enable_v2(domain, pasids);
if (ret)
goto out_free_domain;
@@ -797,7 +801,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
goto out_free_domain;
}
- ret = iommu_attach_group(dev_state->domain, group);
+ ret = iommu_attach_group(domain, group);
if (ret != 0)
goto out_drop_group;
@@ -821,7 +825,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
iommu_group_put(group);
out_free_domain:
- iommu_domain_free(dev_state->domain);
+ iommu_domain_free(domain);
out_free_states:
free_page((unsigned long)dev_state->states);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (4 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 05/16] iommu/amd/iommu_v2: Use protection_domain in struct device_state Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:17 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 07/16] iommu/amd: Use struct protection_domain in helper functions Vasant Hegde
` (9 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Refactor domain_enable_v2() into helper functions for managing GCR3 table
(i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
subsequent patches. Also re-arrange code and remove forward declaration.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/iommu.c | 66 +++++++++++++++++++++++----------------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index d5569eec0fe9..135fa087ce47 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,7 +77,6 @@ struct iommu_cmd {
struct kmem_cache *amd_iommu_irq_cache;
static void detach_device(struct device *dev);
-static int domain_enable_v2(struct protection_domain *domain, int pasids);
/****************************************************************************
*
@@ -1575,6 +1574,43 @@ static void free_gcr3_table(struct protection_domain *domain)
free_page((unsigned long)domain->gcr3_tbl);
}
+static int get_gcr3_levels(int pasids)
+{
+ int levels = 0;
+
+ if (pasids == -1)
+ return amd_iommu_max_glx_val;
+
+ /*
+ * Number of GCR3 table levels required. Level must be 4-Kbyte
+ * page and can contain upto 512 entries.
+ */
+ for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
+ levels += 1;
+
+ return levels;
+}
+
+/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
+static int setup_gcr3_table(struct protection_domain *domain, int pasids)
+{
+ int levels = get_gcr3_levels(pasids);
+
+ if (levels > amd_iommu_max_glx_val)
+ return -EINVAL;
+
+ domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
+ if (domain->gcr3_tbl == NULL)
+ return -ENOMEM;
+
+ domain->glx = levels;
+ domain->flags |= PD_IOMMUV2_MASK;
+
+ amd_iommu_domain_update(domain);
+
+ return 0;
+}
+
static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
struct protection_domain *domain, bool ats, bool ppr)
{
@@ -2065,7 +2101,7 @@ static int protection_domain_init_v2(struct protection_domain *domain)
domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
- if (domain_enable_v2(domain, 1)) {
+ if (setup_gcr3_table(domain, 1)) {
domain_id_free(domain->id);
return -ENOMEM;
}
@@ -2514,30 +2550,6 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
}
EXPORT_SYMBOL(amd_iommu_domain_direct_map);
-/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
-static int domain_enable_v2(struct protection_domain *domain, int pasids)
-{
- int levels;
-
- /* Number of GCR3 table levels required */
- for (levels = 0; (pasids - 1) & ~0x1ff; pasids >>= 9)
- levels += 1;
-
- if (levels > amd_iommu_max_glx_val)
- return -EINVAL;
-
- domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
- if (domain->gcr3_tbl == NULL)
- return -ENOMEM;
-
- domain->glx = levels;
- domain->flags |= PD_IOMMUV2_MASK;
-
- amd_iommu_domain_update(domain);
-
- return 0;
-}
-
int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
{
struct protection_domain *pdom = to_pdomain(dom);
@@ -2556,7 +2568,7 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
goto out;
if (!pdom->gcr3_tbl)
- ret = domain_enable_v2(pdom, pasids);
+ ret = setup_gcr3_table(pdom, pasids);
out:
spin_unlock_irqrestore(&pdom->lock, flags);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table
2023-08-04 6:42 ` [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table Vasant Hegde
@ 2023-08-04 13:17 ` Jason Gunthorpe
2023-08-04 16:51 ` Vasant Hegde
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:17 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:06AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> Refactor domain_enable_v2() into helper functions for managing GCR3 table
> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
> subsequent patches. Also re-arrange code and remove forward declaration.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/iommu.c | 66 +++++++++++++++++++++++----------------
> 1 file changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index d5569eec0fe9..135fa087ce47 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -77,7 +77,6 @@ struct iommu_cmd {
> struct kmem_cache *amd_iommu_irq_cache;
>
> static void detach_device(struct device *dev);
> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>
> /****************************************************************************
> *
> @@ -1575,6 +1574,43 @@ static void free_gcr3_table(struct protection_domain *domain)
> free_page((unsigned long)domain->gcr3_tbl);
> }
>
> +static int get_gcr3_levels(int pasids)
> +{
> + int levels = 0;
> +
> + if (pasids == -1)
> + return amd_iommu_max_glx_val;
> +
> + /*
> + * Number of GCR3 table levels required. Level must be 4-Kbyte
> + * page and can contain upto 512 entries.
> + */
> + for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
> + levels += 1;
> +
> + return levels;
You missed to use the DIV_ROUND_UP
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table
2023-08-04 13:17 ` Jason Gunthorpe
@ 2023-08-04 16:51 ` Vasant Hegde
2023-08-04 18:13 ` Jason Gunthorpe
0 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 16:51 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
Hi Jason,
On 8/4/2023 6:47 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 06:42:06AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Refactor domain_enable_v2() into helper functions for managing GCR3 table
>> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
>> subsequent patches. Also re-arrange code and remove forward declaration.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> drivers/iommu/amd/iommu.c | 66 +++++++++++++++++++++++----------------
>> 1 file changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index d5569eec0fe9..135fa087ce47 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -77,7 +77,6 @@ struct iommu_cmd {
>> struct kmem_cache *amd_iommu_irq_cache;
>>
>> static void detach_device(struct device *dev);
>> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>>
>> /****************************************************************************
>> *
>> @@ -1575,6 +1574,43 @@ static void free_gcr3_table(struct protection_domain *domain)
>> free_page((unsigned long)domain->gcr3_tbl);
>> }
>>
>> +static int get_gcr3_levels(int pasids)
>> +{
>> + int levels = 0;
>> +
>> + if (pasids == -1)
>> + return amd_iommu_max_glx_val;
>> +
>> + /*
>> + * Number of GCR3 table levels required. Level must be 4-Kbyte
>> + * page and can contain upto 512 entries.
>> + */
>> + for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
>> + levels += 1;
>> +
>> + return levels;
>
> You missed to use the DIV_ROUND_UP
Sorry. I wanted to add comment to few patches separately. Somehow it got missed.
In our case,
00b=GCR3 table is single-level - PASID size 9 bits
01b=GCR3 table is two-level - PASID size 18 bits
10b=GCR3 table is three-level - - PASID size 20 bits.
Hence I didn't include DIV_ROUND_UP().
-Vasant
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table
2023-08-04 16:51 ` Vasant Hegde
@ 2023-08-04 18:13 ` Jason Gunthorpe
2023-08-07 6:13 ` Vasant Hegde
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 18:13 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 10:21:41PM +0530, Vasant Hegde wrote:
> Hi Jason,
>
>
> On 8/4/2023 6:47 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 04, 2023 at 06:42:06AM +0000, Vasant Hegde wrote:
> >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >> Refactor domain_enable_v2() into helper functions for managing GCR3 table
> >> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
> >> subsequent patches. Also re-arrange code and remove forward declaration.
> >>
> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> >> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> >> ---
> >> drivers/iommu/amd/iommu.c | 66 +++++++++++++++++++++++----------------
> >> 1 file changed, 39 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> >> index d5569eec0fe9..135fa087ce47 100644
> >> --- a/drivers/iommu/amd/iommu.c
> >> +++ b/drivers/iommu/amd/iommu.c
> >> @@ -77,7 +77,6 @@ struct iommu_cmd {
> >> struct kmem_cache *amd_iommu_irq_cache;
> >>
> >> static void detach_device(struct device *dev);
> >> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
> >>
> >> /****************************************************************************
> >> *
> >> @@ -1575,6 +1574,43 @@ static void free_gcr3_table(struct protection_domain *domain)
> >> free_page((unsigned long)domain->gcr3_tbl);
> >> }
> >>
> >> +static int get_gcr3_levels(int pasids)
> >> +{
> >> + int levels = 0;
> >> +
> >> + if (pasids == -1)
> >> + return amd_iommu_max_glx_val;
> >> +
> >> + /*
> >> + * Number of GCR3 table levels required. Level must be 4-Kbyte
> >> + * page and can contain upto 512 entries.
> >> + */
> >> + for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
> >> + levels += 1;
> >> +
> >> + return levels;
> >
> > You missed to use the DIV_ROUND_UP
>
> Sorry. I wanted to add comment to few patches separately. Somehow it got missed.
>
> In our case,
> 00b=GCR3 table is single-level - PASID size 9 bits
> 01b=GCR3 table is two-level - PASID size 18 bits
> 10b=GCR3 table is three-level - - PASID size 20 bits.
>
> Hence I didn't include DIV_ROUND_UP().
You literally coded a for loop to open code division :(
levels = DIV_ROUND_UP(get_counter_order(pasids), 9) - 1;
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table
2023-08-04 18:13 ` Jason Gunthorpe
@ 2023-08-07 6:13 ` Vasant Hegde
0 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-07 6:13 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
Hi Jason,
On 8/4/2023 11:43 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 10:21:41PM +0530, Vasant Hegde wrote:
>> Hi Jason,
>>
>>
>> On 8/4/2023 6:47 PM, Jason Gunthorpe wrote:
>>> On Fri, Aug 04, 2023 at 06:42:06AM +0000, Vasant Hegde wrote:
>>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>
>>>> Refactor domain_enable_v2() into helper functions for managing GCR3 table
>>>> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
>>>> subsequent patches. Also re-arrange code and remove forward declaration.
>>>>
>>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>>>> ---
>>>> drivers/iommu/amd/iommu.c | 66 +++++++++++++++++++++++----------------
>>>> 1 file changed, 39 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>>> index d5569eec0fe9..135fa087ce47 100644
>>>> --- a/drivers/iommu/amd/iommu.c
>>>> +++ b/drivers/iommu/amd/iommu.c
>>>> @@ -77,7 +77,6 @@ struct iommu_cmd {
>>>> struct kmem_cache *amd_iommu_irq_cache;
>>>>
>>>> static void detach_device(struct device *dev);
>>>> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>>>>
>>>> /****************************************************************************
>>>> *
>>>> @@ -1575,6 +1574,43 @@ static void free_gcr3_table(struct protection_domain *domain)
>>>> free_page((unsigned long)domain->gcr3_tbl);
>>>> }
>>>>
>>>> +static int get_gcr3_levels(int pasids)
>>>> +{
>>>> + int levels = 0;
>>>> +
>>>> + if (pasids == -1)
>>>> + return amd_iommu_max_glx_val;
>>>> +
>>>> + /*
>>>> + * Number of GCR3 table levels required. Level must be 4-Kbyte
>>>> + * page and can contain upto 512 entries.
>>>> + */
>>>> + for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
>>>> + levels += 1;
>>>> +
>>>> + return levels;
>>>
>>> You missed to use the DIV_ROUND_UP
>>
>> Sorry. I wanted to add comment to few patches separately. Somehow it got missed.
>>
>> In our case,
>> 00b=GCR3 table is single-level - PASID size 9 bits
>> 01b=GCR3 table is two-level - PASID size 18 bits
>> 10b=GCR3 table is three-level - - PASID size 20 bits.
>>
>> Hence I didn't include DIV_ROUND_UP().
>
> You literally coded a for loop to open code division :(
>
> levels = DIV_ROUND_UP(get_counter_order(pasids), 9) - 1;
Ok. Will fix it.
-Vasant
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 07/16] iommu/amd: Use struct protection_domain in helper functions
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (5 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 06/16] iommu/amd: Introduce helper functions for managing GCR3 table Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode Vasant Hegde
` (8 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
To simplify the unnecessary use of container_of() on the struct
iommu_domain to get the container structure.
No functional changes intended.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/amd_iommu.h | 13 +++++++++----
drivers/iommu/amd/io_pgtable_v2.c | 6 +++---
drivers/iommu/amd/iommu.c | 18 ++++--------------
drivers/iommu/amd/iommu_v2.c | 11 +++++------
4 files changed, 21 insertions(+), 27 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6bf55de9fbeb..a5a350ee36fe 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -58,15 +58,15 @@ int amd_iommu_register_ppr_notifier(struct notifier_block *nb);
int amd_iommu_unregister_ppr_notifier(struct notifier_block *nb);
void amd_iommu_domain_direct_map(struct iommu_domain *dom);
int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids);
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
void amd_iommu_domain_update(struct protection_domain *domain);
void amd_iommu_domain_flush_complete(struct protection_domain *domain);
void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid);
#ifdef CONFIG_IRQ_REMAP
int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
@@ -134,6 +134,11 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
return page ? page_address(page) : NULL;
}
+static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
+{
+ return container_of(dom, struct protection_domain, domain);
+}
+
bool translation_pre_enabled(struct amd_iommu *iommu);
bool amd_iommu_is_attach_deferred(struct device *dev);
int __init add_special_device(u8 type, u8 id, u32 *devid, bool cmd_line);
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index e9ef2e0a62f6..a4fa830eab54 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -274,9 +274,9 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
out:
if (updated) {
if (count > 1)
- amd_iommu_flush_tlb(&pdom->domain, 0);
+ amd_iommu_flush_tlb(pdom, 0);
else
- amd_iommu_flush_page(&pdom->domain, 0, o_iova);
+ amd_iommu_flush_page(pdom, 0, o_iova);
}
if (mapped)
@@ -384,7 +384,7 @@ static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *coo
if (!pgtable->pgd)
return NULL;
- ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
+ ret = amd_iommu_domain_set_gcr3(pdom, 0, iommu_virt_to_phys(pgtable->pgd));
if (ret)
goto err_free_pgd;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 135fa087ce47..1836df41fc7e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -174,11 +174,6 @@ static struct amd_iommu *rlookup_amd_iommu(struct device *dev)
return __rlookup_amd_iommu(seg, PCI_SBDF_TO_DEVID(devid));
}
-static struct protection_domain *to_pdomain(struct iommu_domain *dom)
-{
- return container_of(dom, struct protection_domain, domain);
-}
-
static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
{
struct iommu_dev_data *dev_data;
@@ -2644,10 +2639,8 @@ static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
return __flush_pasid(domain, pasid, address, false);
}
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
- u64 address)
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address)
{
- struct protection_domain *domain = to_pdomain(dom);
unsigned long flags;
int ret;
@@ -2665,9 +2658,8 @@ static int __amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
true);
}
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
{
- struct protection_domain *domain = to_pdomain(dom);
unsigned long flags;
int ret;
@@ -2744,10 +2736,9 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
return __amd_iommu_flush_tlb(domain, pasid);
}
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
unsigned long cr3)
{
- struct protection_domain *domain = to_pdomain(dom);
unsigned long flags;
int ret;
@@ -2759,9 +2750,8 @@ int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
}
EXPORT_SYMBOL(amd_iommu_domain_set_gcr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid)
{
- struct protection_domain *domain = to_pdomain(dom);
unsigned long flags;
int ret;
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 6a423928d5b5..9bf114382c00 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -282,7 +282,7 @@ static void unbind_pasid(struct pasid_state *pasid_state)
smp_wmb();
/* After this the device/pasid can't access the mm anymore */
- amd_iommu_domain_clear_gcr3(&dev_state->pdom->domain, pasid_state->pasid);
+ amd_iommu_domain_clear_gcr3(dev_state->pdom, pasid_state->pasid);
/* Make sure no more pending faults are in the queue */
flush_workqueue(iommu_wq);
@@ -368,10 +368,10 @@ static void mn_invalidate_range(struct mmu_notifier *mn,
dev_state = pasid_state->device_state;
if ((start ^ (end - 1)) < PAGE_SIZE)
- amd_iommu_flush_page(&dev_state->pdom->domain, pasid_state->pasid,
+ amd_iommu_flush_page(dev_state->pdom, pasid_state->pasid,
start);
else
- amd_iommu_flush_tlb(&dev_state->pdom->domain, pasid_state->pasid);
+ amd_iommu_flush_tlb(dev_state->pdom, pasid_state->pasid);
}
static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -650,7 +650,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, u32 pasid,
if (ret)
goto out_unregister;
- ret = amd_iommu_domain_set_gcr3(&dev_state->pdom->domain, pasid,
+ ret = amd_iommu_domain_set_gcr3(dev_state->pdom, pasid,
__pa(pasid_state->mm->pgd));
if (ret)
goto out_clear_state;
@@ -784,8 +784,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
goto out_free_states;
/* Retrieve new protection_domain that has just been allocated */
- dev_state->pdom = container_of(domain,
- struct protection_domain, domain);
+ dev_state->pdom = to_pdomain(domain);
/* See iommu_is_default_domain() */
domain->type = IOMMU_DOMAIN_IDENTITY;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (6 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 07/16] iommu/amd: Use struct protection_domain in helper functions Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 09/16] iommu/amd: Miscellaneous clean up when free domain Vasant Hegde
` (7 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
Since AMD IOMMU page table is not used in passthrough mode, switching to
v1 page table is not required.
Therefore, remove redundant amd_iommu_pgtable update and misleading
warning message.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/init.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 852e40b13d20..2b01dfde6cab 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2134,9 +2134,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
!iommu_feature(iommu, FEATURE_GT)) {
pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
amd_iommu_pgtable = AMD_IOMMU_V1;
- } else if (iommu_default_passthrough()) {
- pr_warn("V2 page table doesn't support passthrough mode. Fallback to v1.\n");
- amd_iommu_pgtable = AMD_IOMMU_V1;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 09/16] iommu/amd: Miscellaneous clean up when free domain
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (7 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:19 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features Vasant Hegde
` (6 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
* Use the protection_domain_free() helper function to free domain.
The function has been modified to also free memory used for the v1 and v2
page tables. Also clear gcr3 table in v2 page table free path.
* Refactor code into cleanup_domain() for reusability. Change BUG_ON to
WARN_ON in cleanup path.
* Protection domain dev_cnt should be read when the domain is locked.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/io_pgtable_v2.c | 8 +++---
drivers/iommu/amd/iommu.c | 44 +++++++++++++++----------------
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index a4fa830eab54..c17cda83bca5 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -363,10 +363,10 @@ static void v2_free_pgtable(struct io_pgtable *iop)
if (!(pdom->flags & PD_IOMMUV2_MASK))
return;
- /*
- * Make changes visible to IOMMUs. No need to clear gcr3 entry
- * as gcr3 table is already freed.
- */
+ /* Clear gcr3 entry */
+ amd_iommu_domain_clear_gcr3(pdom, 0);
+
+ /* Make changes visible to IOMMUs */
amd_iommu_domain_update(pdom);
/* Free page table */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1836df41fc7e..22b38712b28b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2043,9 +2043,11 @@ void amd_iommu_domain_update(struct protection_domain *domain)
static void cleanup_domain(struct protection_domain *domain)
{
struct iommu_dev_data *entry;
- unsigned long flags;
- spin_lock_irqsave(&domain->lock, flags);
+ lockdep_assert_held(&domain->lock);
+
+ if (!domain->dev_cnt)
+ return;
while (!list_empty(&domain->dev_list)) {
entry = list_first_entry(&domain->dev_list,
@@ -2053,8 +2055,7 @@ static void cleanup_domain(struct protection_domain *domain)
BUG_ON(!entry->domain);
do_detach(entry);
}
-
- spin_unlock_irqrestore(&domain->lock, flags);
+ WARN_ON(domain->dev_cnt != 0);
}
static void protection_domain_free(struct protection_domain *domain)
@@ -2065,6 +2066,12 @@ static void protection_domain_free(struct protection_domain *domain)
if (domain->iop.pgtbl_cfg.tlb)
free_io_pgtable_ops(&domain->iop.iop.ops);
+ if (domain->flags & PD_IOMMUV2_MASK)
+ free_gcr3_table(domain);
+
+ if (domain->iop.root)
+ free_page((unsigned long)domain->iop.root);
+
if (domain->id)
domain_id_free(domain->id);
@@ -2079,10 +2086,8 @@ 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) {
- domain_id_free(domain->id);
+ if (!pt_root)
return -ENOMEM;
- }
}
amd_iommu_domain_set_pgtable(domain, pt_root, mode);
@@ -2096,10 +2101,8 @@ static int protection_domain_init_v2(struct protection_domain *domain)
domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
- if (setup_gcr3_table(domain, 1)) {
- domain_id_free(domain->id);
+ if (setup_gcr3_table(domain, 1))
return -ENOMEM;
- }
return 0;
}
@@ -2158,14 +2161,12 @@ 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) {
- domain_id_free(domain->id);
+ if (!pgtbl_ops)
goto out_err;
- }
return domain;
out_err:
- kfree(domain);
+ protection_domain_free(domain);
return NULL;
}
@@ -2203,19 +2204,18 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
static void amd_iommu_domain_free(struct iommu_domain *dom)
{
struct protection_domain *domain;
+ unsigned long flags;
- domain = to_pdomain(dom);
+ if (!dom)
+ return;
- if (domain->dev_cnt > 0)
- cleanup_domain(domain);
+ domain = to_pdomain(dom);
- BUG_ON(domain->dev_cnt != 0);
+ spin_lock_irqsave(&domain->lock, flags);
- if (!dom)
- return;
+ cleanup_domain(domain);
- if (domain->flags & PD_IOMMUV2_MASK)
- free_gcr3_table(domain);
+ spin_unlock_irqrestore(&domain->lock, flags);
protection_domain_free(domain);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 09/16] iommu/amd: Miscellaneous clean up when free domain
2023-08-04 6:42 ` [PATCH v3 09/16] iommu/amd: Miscellaneous clean up when free domain Vasant Hegde
@ 2023-08-04 13:19 ` Jason Gunthorpe
0 siblings, 0 replies; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:19 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:09AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> * Use the protection_domain_free() helper function to free domain.
> The function has been modified to also free memory used for the v1 and v2
> page tables. Also clear gcr3 table in v2 page table free path.
>
> * Refactor code into cleanup_domain() for reusability. Change BUG_ON to
> WARN_ON in cleanup path.
>
> * Protection domain dev_cnt should be read when the domain is locked.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/io_pgtable_v2.c | 8 +++---
> drivers/iommu/amd/iommu.c | 44 +++++++++++++++----------------
> 2 files changed, 26 insertions(+), 26 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (8 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 09/16] iommu/amd: Miscellaneous clean up when free domain Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:19 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 11/16] iommu/amd: Rename ats related variables Vasant Hegde
` (5 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
In order to support v2 page table, IOMMU driver need to check if the
hardware can support Guest Translation (GT) and Peripheral Page Requet
(PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
features. There variables area redundant since we could simply just check
the global EFR mask.
Therefore, replace it with a helper function with appropriate name.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/amd_iommu.h | 11 +++++++++++
drivers/iommu/amd/amd_iommu_types.h | 9 ++++-----
drivers/iommu/amd/init.c | 14 +-------------
drivers/iommu/amd/iommu.c | 2 +-
4 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index a5a350ee36fe..0605f02fa711 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
return !!(iommu->features & mask);
}
+static inline bool check_feature_on_all_iommus(u64 mask)
+{
+ return !!(amd_iommu_efr & mask);
+}
+
+static inline bool amd_iommu_gt_ppr_supported(void)
+{
+ return (check_feature_on_all_iommus(FEATURE_GT) &&
+ check_feature_on_all_iommus(FEATURE_PPR));
+}
+
static inline u64 iommu_virt_to_phys(void *vaddr)
{
return (u64)__sme_set(virt_to_phys(vaddr));
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 13c582a2454a..bbb5ac4f0d39 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -484,6 +484,10 @@ extern bool amdr_ivrs_remap_support;
/* kmem_cache to get tables with 128 byte alignement */
extern struct kmem_cache *amd_iommu_irq_cache;
+/* Global EFR and EFR2 registers */
+extern u64 amd_iommu_efr;
+extern u64 amd_iommu_efr2;
+
#define PCI_SBDF_TO_SEGID(sbdf) (((sbdf) >> 16) & 0xffff)
#define PCI_SBDF_TO_DEVID(sbdf) ((sbdf) & 0xffff)
#define PCI_SEG_DEVID_TO_SBDF(seg, devid) ((((u32)(seg) & 0xffff) << 16) | \
@@ -679,9 +683,6 @@ struct amd_iommu {
/* Extended features 2 */
u64 features2;
- /* IOMMUv2 */
- bool is_iommu_v2;
-
/* PCI device id of the IOMMU device */
u16 devid;
@@ -890,8 +891,6 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
/* Smallest max PASID supported by any IOMMU in the system */
extern u32 amd_iommu_max_pasid;
-extern bool amd_iommu_v2_present;
-
extern bool amd_iommu_force_isolation;
/* Max levels of glxval supported */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 2b01dfde6cab..1f56478ae74e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -187,7 +187,6 @@ bool amd_iommu_iotlb_sup __read_mostly = true;
u32 amd_iommu_max_pasid __read_mostly = ~0;
-bool amd_iommu_v2_present __read_mostly;
static bool amd_iommu_pc_present __read_mostly;
bool amdr_ivrs_remap_support __read_mostly;
@@ -302,11 +301,6 @@ static void get_global_efr(void)
pr_info("Using global IVHD EFR:%#llx, EFR2:%#llx\n", amd_iommu_efr, amd_iommu_efr2);
}
-static bool check_feature_on_all_iommus(u64 mask)
-{
- return !!(amd_iommu_efr & mask);
-}
-
static inline int check_feature_gpt_level(void)
{
return ((amd_iommu_efr >> FEATURE_GATS_SHIFT) & FEATURE_GATS_MASK);
@@ -2112,12 +2106,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
amd_iommu_max_glx_val = min(amd_iommu_max_glx_val, glxval);
}
- if (iommu_feature(iommu, FEATURE_GT) &&
- iommu_feature(iommu, FEATURE_PPR)) {
- iommu->is_iommu_v2 = true;
- amd_iommu_v2_present = true;
- }
-
if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
return -ENOMEM;
@@ -3693,7 +3681,7 @@ bool amd_iommu_v2_supported(void)
* (i.e. EFR[SNPSup]=1), IOMMUv2 page table cannot be used without
* setting up IOMMUv1 page table.
*/
- return amd_iommu_v2_present && !amd_iommu_snp_en;
+ return amd_iommu_gt_ppr_supported() && !amd_iommu_snp_en;
}
EXPORT_SYMBOL(amd_iommu_v2_supported);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 22b38712b28b..e5b2dbf841c6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -392,7 +392,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
*/
if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
- dev_data->iommu_v2 = iommu->is_iommu_v2;
+ dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
}
dev_iommu_priv_set(dev, dev_data);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-04 6:42 ` [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features Vasant Hegde
@ 2023-08-04 13:19 ` Jason Gunthorpe
2023-08-07 16:44 ` Vasant Hegde
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:19 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:10AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> In order to support v2 page table, IOMMU driver need to check if the
> hardware can support Guest Translation (GT) and Peripheral Page Requet
> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
> features. There variables area redundant since we could simply just check
> the global EFR mask.
>
> Therefore, replace it with a helper function with appropriate name.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/amd_iommu.h | 11 +++++++++++
> drivers/iommu/amd/amd_iommu_types.h | 9 ++++-----
> drivers/iommu/amd/init.c | 14 +-------------
> drivers/iommu/amd/iommu.c | 2 +-
> 4 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index a5a350ee36fe..0605f02fa711 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
> return !!(iommu->features & mask);
> }
>
> +static inline bool check_feature_on_all_iommus(u64 mask)
> +{
> + return !!(amd_iommu_efr & mask);
> +}
> +
> +static inline bool amd_iommu_gt_ppr_supported(void)
> +{
> + return (check_feature_on_all_iommus(FEATURE_GT) &&
> + check_feature_on_all_iommus(FEATURE_PPR));
> +}
> +
I'm still against adding more globals, the iommu struct was available,
just use it directly in this patch.
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-04 13:19 ` Jason Gunthorpe
@ 2023-08-07 16:44 ` Vasant Hegde
2023-08-08 13:55 ` Jason Gunthorpe
0 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-07 16:44 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
Hi Jason,
On 8/4/2023 6:49 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 06:42:10AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> In order to support v2 page table, IOMMU driver need to check if the
>> hardware can support Guest Translation (GT) and Peripheral Page Requet
>> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
>> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
>> features. There variables area redundant since we could simply just check
>> the global EFR mask.
>>
>> Therefore, replace it with a helper function with appropriate name.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> drivers/iommu/amd/amd_iommu.h | 11 +++++++++++
>> drivers/iommu/amd/amd_iommu_types.h | 9 ++++-----
>> drivers/iommu/amd/init.c | 14 +-------------
>> drivers/iommu/amd/iommu.c | 2 +-
>> 4 files changed, 17 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index a5a350ee36fe..0605f02fa711 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
>> return !!(iommu->features & mask);
>> }
>>
>> +static inline bool check_feature_on_all_iommus(u64 mask)
>> +{
>> + return !!(amd_iommu_efr & mask);
>> +}
>> +
>> +static inline bool amd_iommu_gt_ppr_supported(void)
>> +{
>> + return (check_feature_on_all_iommus(FEATURE_GT) &&
>> + check_feature_on_all_iommus(FEATURE_PPR));
>> +}
>> +
>
> I'm still against adding more globals, the iommu struct was available,
> just use it directly in this patch.
Sorry. I missed to append the reason after re-generating patch series.
We want to make sure features supported by all IOMMUs are consistent.
Hence we introduced this function. This function will be used in
subsequent series (ex: before enabling IOMMU PPR feature, etc).
Also there are features like SNP which has requirement that all IOMMUs supports
features before enabling it. So moving all checks to
check_feature_on_all_iommu() makes it easy.
-Vasant
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-07 16:44 ` Vasant Hegde
@ 2023-08-08 13:55 ` Jason Gunthorpe
2023-08-10 20:31 ` Suthikulpanit, Suravee
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-08 13:55 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Mon, Aug 07, 2023 at 10:14:23PM +0530, Vasant Hegde wrote:
> Hi Jason,
>
>
> On 8/4/2023 6:49 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 04, 2023 at 06:42:10AM +0000, Vasant Hegde wrote:
> >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >> In order to support v2 page table, IOMMU driver need to check if the
> >> hardware can support Guest Translation (GT) and Peripheral Page Requet
> >> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
> >> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
> >> features. There variables area redundant since we could simply just check
> >> the global EFR mask.
> >>
> >> Therefore, replace it with a helper function with appropriate name.
> >>
> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> >> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> >> ---
> >> drivers/iommu/amd/amd_iommu.h | 11 +++++++++++
> >> drivers/iommu/amd/amd_iommu_types.h | 9 ++++-----
> >> drivers/iommu/amd/init.c | 14 +-------------
> >> drivers/iommu/amd/iommu.c | 2 +-
> >> 4 files changed, 17 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> >> index a5a350ee36fe..0605f02fa711 100644
> >> --- a/drivers/iommu/amd/amd_iommu.h
> >> +++ b/drivers/iommu/amd/amd_iommu.h
> >> @@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
> >> return !!(iommu->features & mask);
> >> }
> >>
> >> +static inline bool check_feature_on_all_iommus(u64 mask)
> >> +{
> >> + return !!(amd_iommu_efr & mask);
> >> +}
> >> +
> >> +static inline bool amd_iommu_gt_ppr_supported(void)
> >> +{
> >> + return (check_feature_on_all_iommus(FEATURE_GT) &&
> >> + check_feature_on_all_iommus(FEATURE_PPR));
> >> +}
> >> +
> >
> > I'm still against adding more globals, the iommu struct was available,
> > just use it directly in this patch.
>
> Sorry. I missed to append the reason after re-generating patch series.
>
> We want to make sure features supported by all IOMMUs are consistent.
> Hence we introduced this function. This function will be used in
> subsequent series (ex: before enabling IOMMU PPR feature, etc).
That isn't really the right direction. iommus are per-instance things,
each instance should attend to its own stuff. Globals should be avoided.
> Also there are features like SNP which has requirement that all
> IOMMUs supports features before enabling it. So moving all checks to
> check_feature_on_all_iommu() makes it easy.
It is confusing why this would be necessary, but even if it is, it
should be limitd to SNP with a big comment explaining why SNP is
broken.
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-08 13:55 ` Jason Gunthorpe
@ 2023-08-10 20:31 ` Suthikulpanit, Suravee
2023-08-11 13:15 ` Jason Gunthorpe
0 siblings, 1 reply; 33+ messages in thread
From: Suthikulpanit, Suravee @ 2023-08-10 20:31 UTC (permalink / raw)
To: Jason Gunthorpe, Vasant Hegde; +Cc: iommu, joro, wei.huang2, jsnitsel
Jason,
On 8/8/2023 6:55 AM, Jason Gunthorpe wrote:
> On Mon, Aug 07, 2023 at 10:14:23PM +0530, Vasant Hegde wrote:
>> Hi Jason,
>>
>>
>> On 8/4/2023 6:49 PM, Jason Gunthorpe wrote:
>>> On Fri, Aug 04, 2023 at 06:42:10AM +0000, Vasant Hegde wrote:
>>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>
>>>> In order to support v2 page table, IOMMU driver need to check if the
>>>> hardware can support Guest Translation (GT) and Peripheral Page Requet
>>>> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
>>>> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
>>>> features. There variables area redundant since we could simply just check
>>>> the global EFR mask.
>>>>
>>>> Therefore, replace it with a helper function with appropriate name.
>>>>
>>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>>>> ---
>>>> drivers/iommu/amd/amd_iommu.h | 11 +++++++++++
>>>> drivers/iommu/amd/amd_iommu_types.h | 9 ++++-----
>>>> drivers/iommu/amd/init.c | 14 +-------------
>>>> drivers/iommu/amd/iommu.c | 2 +-
>>>> 4 files changed, 17 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>>>> index a5a350ee36fe..0605f02fa711 100644
>>>> --- a/drivers/iommu/amd/amd_iommu.h
>>>> +++ b/drivers/iommu/amd/amd_iommu.h
>>>> @@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
>>>> return !!(iommu->features & mask);
>>>> }
>>>>
>>>> +static inline bool check_feature_on_all_iommus(u64 mask)
>>>> +{
>>>> + return !!(amd_iommu_efr & mask);
>>>> +}
>>>> +
>>>> +static inline bool amd_iommu_gt_ppr_supported(void)
>>>> +{
>>>> + return (check_feature_on_all_iommus(FEATURE_GT) &&
>>>> + check_feature_on_all_iommus(FEATURE_PPR));
>>>> +}
>>>> +
>>>
>>> I'm still against adding more globals, the iommu struct was available,
>>> just use it directly in this patch.
>>
>> Sorry. I missed to append the reason after re-generating patch series.
>>
>> We want to make sure features supported by all IOMMUs are consistent.
>> Hence we introduced this function. This function will be used in
>> subsequent series (ex: before enabling IOMMU PPR feature, etc).
>
> That isn't really the right direction. iommus are per-instance things,
> each instance should attend to its own stuff. Globals should be avoided.
>
>> Also there are features like SNP which has requirement that all
>> IOMMUs supports features before enabling it. So moving all checks to
>> check_feature_on_all_iommu() makes it easy.
>
> It is confusing why this would be necessary, but even if it is, it
> should be limitd to SNP with a big comment explaining why SNP is
> broken.
>
> Jason
Actually, it is not just for the SNP.
Please let me further clarify the existence of the global amd_iommu_efr
and amd_iommu_efr2 variables.
EFR and EFR2 are provided in two places:
1. The IVHD block in the ACPI IVRS table.
2. The register offset 0030h (EFR) and 01A0h (EFR2).
where information in the IVHD block supersedes the MMIO registers.
The EFR/EFR2 information is needed early in the initialization process
prior to amd_iomu_init_pci() (where we can start accessing the MMIO
registers). So, current code ignore information in the MMIO registers
(only check and warn in case of the IVHD and MMIO are mismatch).
Currently, IOMMU driver assumes capabilities on all IOMMU instances to
be homogeneous. Therefore, during early_amd_iommu_init(), the driver
probes all IVHD blocks and do sanity check to make sure that only
features common among all IOMMU instances are supported. This is tracked
in the global amd_iommu_efr / amd_iommu_efr2.
Therefore, the helper function check_feature_on_all_iommus() uses the
effective amd_iommu_efr/amd_iommu_efr2 instead of iommu->features. In
facts, we should remove the code which directly uses the
iommu->features, iommu->features2, and the iommu_feature() helper
function to simply and make the code less confusing.
We can do this in subsequent patch separately.
Thanks,
Suravee
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-10 20:31 ` Suthikulpanit, Suravee
@ 2023-08-11 13:15 ` Jason Gunthorpe
2023-08-11 17:02 ` Suthikulpanit, Suravee
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-11 13:15 UTC (permalink / raw)
To: Suthikulpanit, Suravee; +Cc: Vasant Hegde, iommu, joro, wei.huang2, jsnitsel
On Thu, Aug 10, 2023 at 01:31:41PM -0700, Suthikulpanit, Suravee wrote:
>
> Actually, it is not just for the SNP.
>
> Please let me further clarify the existence of the global amd_iommu_efr and
> amd_iommu_efr2 variables.
>
> EFR and EFR2 are provided in two places:
> 1. The IVHD block in the ACPI IVRS table.
> 2. The register offset 0030h (EFR) and 01A0h (EFR2).
> where information in the IVHD block supersedes the MMIO registers.
>
> The EFR/EFR2 information is needed early in the initialization process prior
> to amd_iomu_init_pci() (where we can start accessing the MMIO registers).
> So, current code ignore information in the MMIO registers (only check and
> warn in case of the IVHD and MMIO are mismatch).
>
> Currently, IOMMU driver assumes capabilities on all IOMMU instances to be
> homogeneous. Therefore, during early_amd_iommu_init(), the driver probes all
> IVHD blocks and do sanity check to make sure that only features common among
> all IOMMU instances are supported. This is tracked in the global
> amd_iommu_efr / amd_iommu_efr2.
Assuming homogeneous is sort of the point - things should be localized
to their instances so you don't need to make this assumption, that
means minimizing the places that touch the global version
> Therefore, the helper function check_feature_on_all_iommus() uses the
> effective amd_iommu_efr/amd_iommu_efr2 instead of iommu->features. In facts,
> we should remove the code which directly uses the iommu->features,
> iommu->features2, and the iommu_feature() helper function to simply and make
> the code less confusing.
But if you do this I would not object - I just don't think it is good
driver design *for the iommu*
However..
Looking at it I only see two places that need the global 'all iommus'
check - one is amd_iommu_snp_enable() which is never called (seriously
stop adding dead code to support your out of tree patch sets!!!)
And the other is enable_iommus_vapic() which looks like it has the
iommus probed already.
So really this is 'we need it global for some SNP thing that isn't
even in the kernel' which is a really, really, bad justification.
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features
2023-08-11 13:15 ` Jason Gunthorpe
@ 2023-08-11 17:02 ` Suthikulpanit, Suravee
0 siblings, 0 replies; 33+ messages in thread
From: Suthikulpanit, Suravee @ 2023-08-11 17:02 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Vasant Hegde, iommu, joro, wei.huang2, jsnitsel
On 8/11/2023 6:15 AM, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 01:31:41PM -0700, Suthikulpanit, Suravee wrote:
>> Actually, it is not just for the SNP.
>>
>> Please let me further clarify the existence of the global amd_iommu_efr and
>> amd_iommu_efr2 variables.
>>
>> EFR and EFR2 are provided in two places:
>> 1. The IVHD block in the ACPI IVRS table.
>> 2. The register offset 0030h (EFR) and 01A0h (EFR2).
>> where information in the IVHD block supersedes the MMIO registers.
>>
>> The EFR/EFR2 information is needed early in the initialization process prior
>> to amd_iomu_init_pci() (where we can start accessing the MMIO registers).
>> So, current code ignore information in the MMIO registers (only check and
>> warn in case of the IVHD and MMIO are mismatch).
>>
>> Currently, IOMMU driver assumes capabilities on all IOMMU instances to be
>> homogeneous. Therefore, during early_amd_iommu_init(), the driver probes all
>> IVHD blocks and do sanity check to make sure that only features common among
>> all IOMMU instances are supported. This is tracked in the global
>> amd_iommu_efr / amd_iommu_efr2.
> Assuming homogeneous is sort of the point - things should be localized
> to their instances so you don't need to make this assumption, that
> means minimizing the places that touch the global version
>
>> Therefore, the helper function check_feature_on_all_iommus() uses the
>> effective amd_iommu_efr/amd_iommu_efr2 instead of iommu->features. In facts,
>> we should remove the code which directly uses the iommu->features,
>> iommu->features2, and the iommu_feature() helper function to simply and make
>> the code less confusing.
> But if you do this I would not object - I just don't think it is good
> driver design*for the iommu*
>
> However..
>
> Looking at it I only see two places that need the global 'all iommus'
> check - one is amd_iommu_snp_enable() which is never called (seriously
> stop adding dead code to support your out of tree patch sets!!!)
FYI: The SNP host kernel support is a WIP, and has been going on for
quite sometime.
https://lore.kernel.org/linux-mm/72939733-c736-98ec-ed09-e835d8f988b2@amd.com/T/
We upstreamed the amd_iommu_snp_enable() separately to IOMMU mailing
list to help prepare for the on-going SNP work, which is a large patch
series. This helps reduce the amount of patches to be managed and
reviewed in that series.
In addition, this the IOMMU support for SNP is part of IOMMU subsystem,
which is reviewed and pulled separately from the SNP stuff. It just
happened to be accepted first.
Thanks,
Suravee
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 11/16] iommu/amd: Rename ats related variables
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (9 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 10/16] iommu/amd: Modify logic for checking GT and PPR features Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler Vasant Hegde
` (4 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
Remove nested structure and make it as 'ats_{enable/qdep}'.
Also convert 'dev_data.pri_tlp' to bit field.
No functional changes intended.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/amd_iommu_types.h | 8 +++-----
drivers/iommu/amd/iommu.c | 28 ++++++++++++++--------------
2 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index bbb5ac4f0d39..b79c6e10684d 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -816,11 +816,9 @@ struct iommu_dev_data {
struct device *dev;
u16 devid; /* PCI Device ID */
bool iommu_v2; /* Device can make use of IOMMUv2 */
- struct {
- bool enabled;
- int qdep;
- } ats; /* ATS state */
- bool pri_tlp; /* PASID TLB required for
+ int ats_qdep;
+ u8 ats_enabled :1; /* ATS state */
+ u8 pri_tlp :1; /* PASID TLB required for
PPR completions */
bool use_vapic; /* Enable device to use vapic mode */
bool defer_attach;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index e5b2dbf841c6..5968eb8d265c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1086,7 +1086,7 @@ static void build_inv_iotlb_pasid(struct iommu_cmd *cmd, u16 devid, u32 pasid,
}
static void build_complete_ppr(struct iommu_cmd *cmd, u16 devid, u32 pasid,
- int status, int tag, bool gn)
+ int status, int tag, u8 gn)
{
memset(cmd, 0, sizeof(*cmd));
@@ -1309,7 +1309,7 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
struct iommu_cmd cmd;
int qdep;
- qdep = dev_data->ats.qdep;
+ qdep = dev_data->ats_qdep;
iommu = rlookup_amd_iommu(dev_data->dev);
if (!iommu)
return -EINVAL;
@@ -1360,7 +1360,7 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
return ret;
}
- if (dev_data->ats.enabled)
+ if (dev_data->ats_enabled)
ret = device_flush_iotlb(dev_data, 0, ~0UL);
return ret;
@@ -1393,7 +1393,7 @@ static void __domain_flush_pages(struct protection_domain *domain,
list_for_each_entry(dev_data, &domain->dev_list, list) {
- if (!dev_data->ats.enabled)
+ if (!dev_data->ats_enabled)
continue;
ret |= device_flush_iotlb(dev_data, address, size);
@@ -1714,7 +1714,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
iommu = rlookup_amd_iommu(dev_data->dev);
if (!iommu)
return;
- ats = dev_data->ats.enabled;
+ ats = dev_data->ats_enabled;
/* Update data structures */
dev_data->domain = domain;
@@ -1852,14 +1852,14 @@ static int attach_device(struct device *dev,
if (pdev_pri_ats_enable(pdev) != 0)
goto out;
- dev_data->ats.enabled = true;
- dev_data->ats.qdep = pci_ats_queue_depth(pdev);
+ dev_data->ats_enabled = 1;
+ dev_data->ats_qdep = pci_ats_queue_depth(pdev);
dev_data->pri_tlp = pci_prg_resp_pasid_required(pdev);
}
} else if (amd_iommu_iotlb_sup &&
pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
- dev_data->ats.enabled = true;
- dev_data->ats.qdep = pci_ats_queue_depth(pdev);
+ dev_data->ats_enabled = 1;
+ dev_data->ats_qdep = pci_ats_queue_depth(pdev);
}
skip_ats_check:
@@ -1916,10 +1916,10 @@ static void detach_device(struct device *dev)
if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
pdev_iommuv2_disable(to_pci_dev(dev));
- else if (dev_data->ats.enabled)
+ else if (dev_data->ats_enabled)
pci_disable_ats(to_pci_dev(dev));
- dev_data->ats.enabled = false;
+ dev_data->ats_enabled = 0;
out:
spin_unlock(&dev_data->lock);
@@ -2009,7 +2009,7 @@ static void update_device_table(struct protection_domain *domain)
if (!iommu)
continue;
set_dte_entry(iommu, dev_data->devid, domain,
- dev_data->ats.enabled, dev_data->iommu_v2);
+ dev_data->ats_enabled, dev_data->iommu_v2);
clone_aliases(iommu, dev_data->dev);
}
}
@@ -2608,10 +2608,10 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
There might be non-IOMMUv2 capable devices in an IOMMUv2
* domain.
*/
- if (!dev_data->ats.enabled)
+ if (!dev_data->ats_enabled)
continue;
- qdep = dev_data->ats.qdep;
+ qdep = dev_data->ats_qdep;
iommu = rlookup_amd_iommu(dev_data->dev);
if (!iommu)
continue;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (10 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 11/16] iommu/amd: Rename ats related variables Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:28 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities Vasant Hegde
` (3 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
PPR is enabled per PCI end-point device, and is configured by the PPR bit
in the IOMMU device table entry (i.e DTE[PPR]).
Currently, PPR is enabled for a device when it is initialized for AMD
IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
updated when enable/disable IOPF feature.
Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
setting for each device.
Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
remove redundant feature support check in set_dte_entry().
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
drivers/iommu/amd/iommu.c | 10 ++++------
drivers/iommu/amd/iommu_v2.c | 14 +++++++++++++-
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index b79c6e10684d..443b13b56235 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -803,6 +803,11 @@ struct devid_map {
bool cmd_line;
};
+enum ppr_handlers {
+ PPR_HANDLER_NONE, /* No handler specified */
+ PPR_HANDLER_V2API, /* IOMMU v2 API ppr handler */
+};
+
/*
* This struct contains device specific data for the IOMMU
*/
@@ -820,6 +825,7 @@ struct iommu_dev_data {
u8 ats_enabled :1; /* ATS state */
u8 pri_tlp :1; /* PASID TLB required for
PPR completions */
+ enum ppr_handlers ppr;
bool use_vapic; /* Enable device to use vapic mode */
bool defer_attach;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5968eb8d265c..888bce5abe56 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1634,10 +1634,8 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
if (ats)
flags |= DTE_FLAG_IOTLB;
- if (ppr) {
- if (iommu_feature(iommu, FEATURE_EPHSUP))
- pte_root |= 1ULL << DEV_ENTRY_PPR;
- }
+ if (ppr)
+ pte_root |= 1ULL << DEV_ENTRY_PPR;
if (domain->flags & PD_IOMMUV2_MASK) {
u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
@@ -1730,7 +1728,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
/* Update device table */
set_dte_entry(iommu, dev_data->devid, domain,
- ats, dev_data->iommu_v2);
+ ats, dev_data->ppr);
clone_aliases(iommu, dev_data->dev);
device_flush_dte(dev_data);
@@ -2009,7 +2007,7 @@ static void update_device_table(struct protection_domain *domain)
if (!iommu)
continue;
set_dte_entry(iommu, dev_data->devid, domain,
- dev_data->ats_enabled, dev_data->iommu_v2);
+ dev_data->ats_enabled, dev_data->ppr);
clone_aliases(iommu, dev_data->dev);
}
}
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 9bf114382c00..4148cbf069dc 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -740,6 +740,10 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
unsigned long flags;
int ret, tmp;
u32 sbdf;
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (!dev_data)
+ return -ENODEV;
might_sleep();
@@ -762,6 +766,9 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
if (dev_state == NULL)
return -ENOMEM;
+ /* Enable device for PPR support */
+ dev_data->ppr = PPR_HANDLER_V2API;
+
spin_lock_init(&dev_state->lock);
init_waitqueue_head(&dev_state->wq);
dev_state->pdev = pdev;
@@ -832,6 +839,8 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
out_free_dev_state:
kfree(dev_state);
+ dev_data->ppr = PPR_HANDLER_NONE;
+
return ret;
}
EXPORT_SYMBOL(amd_iommu_init_device);
@@ -841,8 +850,9 @@ void amd_iommu_free_device(struct pci_dev *pdev)
struct device_state *dev_state;
unsigned long flags;
u32 sbdf;
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
- if (!amd_iommu_v2_supported())
+ if (!dev_data || !amd_iommu_v2_supported())
return;
sbdf = get_pci_sbdf_id(pdev);
@@ -859,6 +869,8 @@ void amd_iommu_free_device(struct pci_dev *pdev)
spin_unlock_irqrestore(&state_lock, flags);
+ dev_data->ppr = PPR_HANDLER_NONE;
+
put_device_state(dev_state);
free_device_state(dev_state);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler
2023-08-04 6:42 ` [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler Vasant Hegde
@ 2023-08-04 13:28 ` Jason Gunthorpe
2023-08-04 16:22 ` Vasant Hegde
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:28 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:12AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
> PPR is enabled per PCI end-point device, and is configured by the PPR bit
> in the IOMMU device table entry (i.e DTE[PPR]).
>
> Currently, PPR is enabled for a device when it is initialized for AMD
> IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
> updated when enable/disable IOPF feature.
>
> Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
> setting for each device.
>
> Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
> remove redundant feature support check in set_dte_entry().
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
> drivers/iommu/amd/iommu.c | 10 ++++------
> drivers/iommu/amd/iommu_v2.c | 14 +++++++++++++-
> 3 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index b79c6e10684d..443b13b56235 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -803,6 +803,11 @@ struct devid_map {
> bool cmd_line;
> };
>
> +enum ppr_handlers {
> + PPR_HANDLER_NONE, /* No handler specified */
> + PPR_HANDLER_V2API, /* IOMMU v2 API ppr handler */
> +};
> +
> /*
> * This struct contains device specific data for the IOMMU
> */
> @@ -820,6 +825,7 @@ struct iommu_dev_data {
> u8 ats_enabled :1; /* ATS state */
> u8 pri_tlp :1; /* PASID TLB required for
> PPR completions */
> + enum ppr_handlers ppr;
> bool use_vapic; /* Enable device to use vapic mode */
> bool defer_attach;
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 5968eb8d265c..888bce5abe56 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1634,10 +1634,8 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
> if (ats)
> flags |= DTE_FLAG_IOTLB;
>
> - if (ppr) {
> - if (iommu_feature(iommu, FEATURE_EPHSUP))
> - pte_root |= 1ULL << DEV_ENTRY_PPR;
> - }
> + if (ppr)
> + pte_root |= 1ULL << DEV_ENTRY_PPR;
>
> if (domain->flags & PD_IOMMUV2_MASK) {
> u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
> @@ -1730,7 +1728,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
>
> /* Update device table */
> set_dte_entry(iommu, dev_data->devid, domain,
> - ats, dev_data->iommu_v2);
> + ats, dev_data->ppr);
This is casting an enum to a bool,
dev_data->ppr != PPR_HANDLER_NONE
?
And in other places
Or maybe just pass in the enum.
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler
2023-08-04 13:28 ` Jason Gunthorpe
@ 2023-08-04 16:22 ` Vasant Hegde
0 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 16:22 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
Hi Jason,
On 8/4/2023 6:58 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 06:42:12AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
>> PPR is enabled per PCI end-point device, and is configured by the PPR bit
>> in the IOMMU device table entry (i.e DTE[PPR]).
>>
>> Currently, PPR is enabled for a device when it is initialized for AMD
>> IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
>> updated when enable/disable IOPF feature.
>>
>> Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
>> setting for each device.
>>
>> Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
>> remove redundant feature support check in set_dte_entry().
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
>> drivers/iommu/amd/iommu.c | 10 ++++------
>> drivers/iommu/amd/iommu_v2.c | 14 +++++++++++++-
>> 3 files changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index b79c6e10684d..443b13b56235 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -803,6 +803,11 @@ struct devid_map {
>> bool cmd_line;
>> };
>>
>> +enum ppr_handlers {
>> + PPR_HANDLER_NONE, /* No handler specified */
>> + PPR_HANDLER_V2API, /* IOMMU v2 API ppr handler */
>> +};
>> +
>> /*
>> * This struct contains device specific data for the IOMMU
>> */
>> @@ -820,6 +825,7 @@ struct iommu_dev_data {
>> u8 ats_enabled :1; /* ATS state */
>> u8 pri_tlp :1; /* PASID TLB required for
>> PPR completions */
>> + enum ppr_handlers ppr;
>> bool use_vapic; /* Enable device to use vapic mode */
>> bool defer_attach;
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 5968eb8d265c..888bce5abe56 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1634,10 +1634,8 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
>> if (ats)
>> flags |= DTE_FLAG_IOTLB;
>>
>> - if (ppr) {
>> - if (iommu_feature(iommu, FEATURE_EPHSUP))
>> - pte_root |= 1ULL << DEV_ENTRY_PPR;
>> - }
>> + if (ppr)
>> + pte_root |= 1ULL << DEV_ENTRY_PPR;
>>
>> if (domain->flags & PD_IOMMUV2_MASK) {
>> u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
>> @@ -1730,7 +1728,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
>>
>> /* Update device table */
>> set_dte_entry(iommu, dev_data->devid, domain,
>> - ats, dev_data->iommu_v2);
>> + ats, dev_data->ppr);
>
> This is casting an enum to a bool,
>
> dev_data->ppr != PPR_HANDLER_NONE
>
> ?
>
> And in other places
>
> Or maybe just pass in the enum.
My bad. I missed it. I will pass enum in next iteration.
-Vasant
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (11 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:36 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 14/16] iommu/amd: Enable device ATS/PASID/PRI capabilities independently Vasant Hegde
` (2 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
Currently we use struct iommu_dev_data.iommu_v2 to keep track of the device
ATS, PRI, and PASID capabilities. But these capabilities can be enabled
independently (except PRI requires ATS support). Hence, replace
the iommu_v2 variable with a flags variable, which keep track of the device
capabilities.
Device PRI/PASID is shared between PF and any associated VFs (See commit
9bf49e36d718 ("PCI/ATS: Handle sharing of PF PRI Capability with
all VFs")). Hence use pci_pri_supported() and pci_pasid_features()
instead of pci_find_ext_capability() to check device PRI/PASID support.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/amd_iommu_types.h | 3 ++-
drivers/iommu/amd/iommu.c | 39 +++++++++++++++--------------
2 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 443b13b56235..89eb65117c1f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -820,7 +820,8 @@ struct iommu_dev_data {
struct protection_domain *domain; /* Domain the device is bound to */
struct device *dev;
u16 devid; /* PCI Device ID */
- bool iommu_v2; /* Device can make use of IOMMUv2 */
+
+ u32 flags; /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
int ats_qdep;
u8 ats_enabled :1; /* ATS state */
u8 pri_tlp :1; /* PASID TLB required for
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 888bce5abe56..490c19284a4b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -314,24 +314,25 @@ static struct iommu_group *acpihid_device_group(struct device *dev)
return entry->group;
}
-static bool pci_iommuv2_capable(struct pci_dev *pdev)
+static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
{
- static const int caps[] = {
- PCI_EXT_CAP_ID_PRI,
- PCI_EXT_CAP_ID_PASID,
- };
- int i, pos;
+ return (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP);
+}
- if (!pci_ats_supported(pdev))
- return false;
+static u32 pdev_get_caps(struct pci_dev *pdev)
+{
+ u32 flags = 0;
- for (i = 0; i < 2; ++i) {
- pos = pci_find_ext_capability(pdev, caps[i]);
- if (pos == 0)
- return false;
- }
+ if (pci_ats_supported(pdev))
+ flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
- return true;
+ if (pci_pri_supported(pdev))
+ flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
+
+ if (pci_pasid_features(pdev) >= 0)
+ flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
+
+ return flags;
}
/*
@@ -391,8 +392,8 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
* it'll be forced to go into translation mode.
*/
if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
- dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
- dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
+ dev_is_pci(dev) && amd_iommu_gt_ppr_supported()) {
+ dev_data->flags = pdev_get_caps(to_pci_dev(dev));
}
dev_iommu_priv_set(dev, dev_data);
@@ -1846,7 +1847,7 @@ static int attach_device(struct device *dev,
goto out;
}
- if (dev_data->iommu_v2) {
+ if (pdev_pasid_supported(dev_data)) {
if (pdev_pri_ats_enable(pdev) != 0)
goto out;
@@ -1912,7 +1913,7 @@ static void detach_device(struct device *dev)
if (!dev_is_pci(dev))
goto out;
- if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
+ if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
pdev_iommuv2_disable(to_pci_dev(dev));
else if (dev_data->ats_enabled)
pci_disable_ats(to_pci_dev(dev));
@@ -2467,7 +2468,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
* and require remapping.
* - SNP is enabled, because it prohibits DTE[Mode]=0.
*/
- if (dev_data->iommu_v2 &&
+ if (pdev_pasid_supported(dev_data) &&
!cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
!amd_iommu_snp_en) {
return IOMMU_DOMAIN_IDENTITY;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities
2023-08-04 6:42 ` [PATCH v3 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities Vasant Hegde
@ 2023-08-04 13:36 ` Jason Gunthorpe
0 siblings, 0 replies; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:36 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:13AM +0000, Vasant Hegde wrote:
> Currently we use struct iommu_dev_data.iommu_v2 to keep track of the device
> ATS, PRI, and PASID capabilities. But these capabilities can be enabled
> independently (except PRI requires ATS support). Hence, replace
> the iommu_v2 variable with a flags variable, which keep track of the device
> capabilities.
>
> Device PRI/PASID is shared between PF and any associated VFs (See commit
> 9bf49e36d718 ("PCI/ATS: Handle sharing of PF PRI Capability with
> all VFs")). Hence use pci_pri_supported() and pci_pasid_features()
> instead of pci_find_ext_capability() to check device PRI/PASID support.
>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/amd_iommu_types.h | 3 ++-
> drivers/iommu/amd/iommu.c | 39 +++++++++++++++--------------
> 2 files changed, 22 insertions(+), 20 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 14/16] iommu/amd: Enable device ATS/PASID/PRI capabilities independently
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (12 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids Vasant Hegde
2023-08-04 6:42 ` [PATCH v3 16/16] iommu/amd: Simplify amd_iommu_device_info() Vasant Hegde
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
Introduce helper functions to enable/disable device ATS/PASID/PRI
capabilities independently along with the new pasid_enabled and
pri_enabled variables in struct iommu_dev_data to keep track,
which allows attach_device() and detach_device() to be simplified.
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/amd_iommu.h | 4 +
drivers/iommu/amd/amd_iommu_types.h | 2 +
drivers/iommu/amd/iommu.c | 203 ++++++++++++++++------------
3 files changed, 120 insertions(+), 89 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 0605f02fa711..56996dc2868d 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -54,6 +54,10 @@ int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
u8 fxn, u64 *value);
+/* Device capabilities */
+int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
+void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
+
int amd_iommu_register_ppr_notifier(struct notifier_block *nb);
int amd_iommu_unregister_ppr_notifier(struct notifier_block *nb);
void amd_iommu_domain_direct_map(struct iommu_domain *dom);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 89eb65117c1f..e114e4af0f05 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -824,6 +824,8 @@ struct iommu_dev_data {
u32 flags; /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
int ats_qdep;
u8 ats_enabled :1; /* ATS state */
+ u8 pri_enabled :1; /* PRI state */
+ u8 pasid_enabled:1; /* PASID state */
u8 pri_tlp :1; /* PASID TLB required for
PPR completions */
enum ppr_handlers ppr;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 490c19284a4b..208d69e7a6b4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -335,6 +335,113 @@ static u32 pdev_get_caps(struct pci_dev *pdev)
return flags;
}
+static inline int pdev_enable_cap_ats(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+ int ret = -EINVAL;
+
+ if (dev_data->ats_enabled)
+ return 0;
+
+ if (amd_iommu_iotlb_sup &&
+ (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_ATS_SUP)) {
+ ret = pci_enable_ats(pdev, PAGE_SHIFT);
+ if (!ret) {
+ dev_data->ats_enabled = 1;
+ dev_data->ats_qdep = pci_ats_queue_depth(pdev);
+ }
+ }
+
+ return ret;
+}
+
+static inline void pdev_disable_cap_ats(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (dev_data->ats_enabled) {
+ pci_disable_ats(pdev);
+ dev_data->ats_enabled = 0;
+ }
+}
+
+int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+ int ret = -EINVAL;
+
+ if (dev_data->pri_enabled)
+ return 0;
+
+ if (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PRI_SUP) {
+ /*
+ * First reset the PRI state of the device.
+ * FIXME: Hardcode number of outstanding requests for now
+ */
+ if (!pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32)) {
+ dev_data->pri_enabled = 1;
+ dev_data->pri_tlp = pci_prg_resp_pasid_required(pdev);
+
+ ret = 0;
+ }
+ }
+
+ return ret;
+}
+
+void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (dev_data->pri_enabled) {
+ pci_disable_pri(pdev);
+ dev_data->pri_enabled = 0;
+ }
+}
+
+static inline int pdev_enable_cap_pasid(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+ int ret = -EINVAL;
+
+ if (dev_data->pasid_enabled)
+ return 0;
+
+ if (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP) {
+ /* Only allow access to user-accessible pages */
+ ret = pci_enable_pasid(pdev, 0);
+ if (!ret)
+ dev_data->pasid_enabled = 1;
+ }
+
+ return ret;
+}
+
+static inline void pdev_disable_cap_pasid(struct pci_dev *pdev)
+{
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (dev_data->pasid_enabled) {
+ pci_disable_pasid(pdev);
+ dev_data->pasid_enabled = 0;
+ }
+}
+
+static void pdev_enable_caps(struct pci_dev *pdev)
+{
+ pdev_enable_cap_ats(pdev);
+ pdev_enable_cap_pasid(pdev);
+ amd_iommu_pdev_enable_cap_pri(pdev);
+
+}
+
+static void pdev_disable_caps(struct pci_dev *pdev)
+{
+ pdev_disable_cap_ats(pdev);
+ pdev_disable_cap_pasid(pdev);
+ amd_iommu_pdev_disable_cap_pri(pdev);
+}
+
/*
* This function checks if the driver got a valid device from the caller to
* avoid dereferencing invalid pointers.
@@ -1764,48 +1871,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
domain->dev_cnt -= 1;
}
-static void pdev_iommuv2_disable(struct pci_dev *pdev)
-{
- pci_disable_ats(pdev);
- pci_disable_pri(pdev);
- pci_disable_pasid(pdev);
-}
-
-static int pdev_pri_ats_enable(struct pci_dev *pdev)
-{
- int ret;
-
- /* Only allow access to user-accessible pages */
- ret = pci_enable_pasid(pdev, 0);
- if (ret)
- return ret;
-
- /* First reset the PRI state of the device */
- ret = pci_reset_pri(pdev);
- if (ret)
- goto out_err_pasid;
-
- /* Enable PRI */
- /* FIXME: Hardcode number of outstanding requests for now */
- ret = pci_enable_pri(pdev, 32);
- if (ret)
- goto out_err_pasid;
-
- ret = pci_enable_ats(pdev, PAGE_SHIFT);
- if (ret)
- goto out_err_pri;
-
- return 0;
-
-out_err_pri:
- pci_disable_pri(pdev);
-
-out_err_pasid:
- pci_disable_pasid(pdev);
-
- return ret;
-}
-
/*
* If a device is not yet associated with a domain, this function makes the
* device visible in the domain
@@ -1814,9 +1879,8 @@ static int attach_device(struct device *dev,
struct protection_domain *domain)
{
struct iommu_dev_data *dev_data;
- struct pci_dev *pdev;
unsigned long flags;
- int ret;
+ int ret = 0;
spin_lock_irqsave(&domain->lock, flags);
@@ -1824,45 +1888,13 @@ static int attach_device(struct device *dev,
spin_lock(&dev_data->lock);
- ret = -EBUSY;
- if (dev_data->domain != NULL)
+ if (dev_data->domain != NULL) {
+ ret = -EBUSY;
goto out;
-
- if (!dev_is_pci(dev))
- goto skip_ats_check;
-
- pdev = to_pci_dev(dev);
- if (domain->flags & PD_IOMMUV2_MASK) {
- struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
-
- ret = -EINVAL;
-
- /*
- * In case of using AMD_IOMMU_V1 page table mode and the device
- * is enabling for PPR/ATS support (using v2 table),
- * we need to make sure that the domain type is identity map.
- */
- if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
- def_domain->type != IOMMU_DOMAIN_IDENTITY) {
- goto out;
- }
-
- if (pdev_pasid_supported(dev_data)) {
- if (pdev_pri_ats_enable(pdev) != 0)
- goto out;
-
- dev_data->ats_enabled = 1;
- dev_data->ats_qdep = pci_ats_queue_depth(pdev);
- dev_data->pri_tlp = pci_prg_resp_pasid_required(pdev);
- }
- } else if (amd_iommu_iotlb_sup &&
- pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
- dev_data->ats_enabled = 1;
- dev_data->ats_qdep = pci_ats_queue_depth(pdev);
}
-skip_ats_check:
- ret = 0;
+ if (dev_is_pci(dev))
+ pdev_enable_caps(to_pci_dev(dev));
do_attach(dev_data, domain);
@@ -1910,15 +1942,8 @@ static void detach_device(struct device *dev)
do_detach(dev_data);
- if (!dev_is_pci(dev))
- goto out;
-
- if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
- pdev_iommuv2_disable(to_pci_dev(dev));
- else if (dev_data->ats_enabled)
- pci_disable_ats(to_pci_dev(dev));
-
- dev_data->ats_enabled = 0;
+ if (dev_is_pci(dev))
+ pdev_disable_caps(to_pci_dev(dev));
out:
spin_unlock(&dev_data->lock);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (13 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 14/16] iommu/amd: Enable device ATS/PASID/PRI capabilities independently Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
2023-08-04 13:14 ` Jason Gunthorpe
2023-08-04 6:42 ` [PATCH v3 16/16] iommu/amd: Simplify amd_iommu_device_info() Vasant Hegde
15 siblings, 1 reply; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde
Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
introduced a variable struct iommu_device.max_pasids to track max
PASIDS supported by each IOMMU.
Let us initialize this field for AMD IOMMU. IOMMU core will use this value
to set max PASIDs per device (see __iommu_probe_device()).
Also remove unused global 'amd_iommu_max_pasid' variable.
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
drivers/iommu/amd/amd_iommu_types.h | 3 ---
drivers/iommu/amd/init.c | 9 ++-------
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index e114e4af0f05..0d339e022572 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -895,9 +895,6 @@ extern unsigned amd_iommu_aperture_order;
/* allocation bitmap for domain ids */
extern unsigned long *amd_iommu_pd_alloc_bitmap;
-/* Smallest max PASID supported by any IOMMU in the system */
-extern u32 amd_iommu_max_pasid;
-
extern bool amd_iommu_force_isolation;
/* Max levels of glxval supported */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 1f56478ae74e..e0b0e48d3d38 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -185,8 +185,6 @@ static int amd_iommus_present;
bool amd_iommu_np_cache __read_mostly;
bool amd_iommu_iotlb_sup __read_mostly = true;
-u32 amd_iommu_max_pasid __read_mostly = ~0;
-
static bool amd_iommu_pc_present __read_mostly;
bool amdr_ivrs_remap_support __read_mostly;
@@ -2086,16 +2084,13 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
if (iommu_feature(iommu, FEATURE_GT)) {
int glxval;
- u32 max_pasid;
u64 pasmax;
pasmax = iommu->features & FEATURE_PASID_MASK;
pasmax >>= FEATURE_PASID_SHIFT;
- max_pasid = (1 << (pasmax + 1)) - 1;
-
- amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
+ iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
- BUG_ON(amd_iommu_max_pasid & ~PASID_MASK);
+ BUG_ON(iommu->iommu.max_pasids & ~PASID_MASK);
glxval = iommu->features & FEATURE_GLXVAL_MASK;
glxval >>= FEATURE_GLXVAL_SHIFT;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids
2023-08-04 6:42 ` [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids Vasant Hegde
@ 2023-08-04 13:14 ` Jason Gunthorpe
2023-08-07 16:45 ` Vasant Hegde
0 siblings, 1 reply; 33+ messages in thread
From: Jason Gunthorpe @ 2023-08-04 13:14 UTC (permalink / raw)
To: Vasant Hegde; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
On Fri, Aug 04, 2023 at 06:42:15AM +0000, Vasant Hegde wrote:
> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
> introduced a variable struct iommu_device.max_pasids to track max
> PASIDS supported by each IOMMU.
>
> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
> to set max PASIDs per device (see __iommu_probe_device()).
>
> Also remove unused global 'amd_iommu_max_pasid' variable.
>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> drivers/iommu/amd/amd_iommu_types.h | 3 ---
> drivers/iommu/amd/init.c | 9 ++-------
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index e114e4af0f05..0d339e022572 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -895,9 +895,6 @@ extern unsigned amd_iommu_aperture_order;
> /* allocation bitmap for domain ids */
> extern unsigned long *amd_iommu_pd_alloc_bitmap;
>
> -/* Smallest max PASID supported by any IOMMU in the system */
> -extern u32 amd_iommu_max_pasid;
> -
> extern bool amd_iommu_force_isolation;
>
> /* Max levels of glxval supported */
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 1f56478ae74e..e0b0e48d3d38 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -185,8 +185,6 @@ static int amd_iommus_present;
> bool amd_iommu_np_cache __read_mostly;
> bool amd_iommu_iotlb_sup __read_mostly = true;
>
> -u32 amd_iommu_max_pasid __read_mostly = ~0;
> -
> static bool amd_iommu_pc_present __read_mostly;
> bool amdr_ivrs_remap_support __read_mostly;
>
> @@ -2086,16 +2084,13 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
>
> if (iommu_feature(iommu, FEATURE_GT)) {
> int glxval;
> - u32 max_pasid;
> u64 pasmax;
>
> pasmax = iommu->features & FEATURE_PASID_MASK;
> pasmax >>= FEATURE_PASID_SHIFT;
> - max_pasid = (1 << (pasmax + 1)) - 1;
> -
> - amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
> + iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
>
> - BUG_ON(amd_iommu_max_pasid & ~PASID_MASK);
> + BUG_ON(iommu->iommu.max_pasids & ~PASID_MASK);
You said you'd fix this to remove the BUG_ON.
Jason
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids
2023-08-04 13:14 ` Jason Gunthorpe
@ 2023-08-07 16:45 ` Vasant Hegde
0 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-07 16:45 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: iommu, joro, suravee.suthikulpanit, wei.huang2, jsnitsel
Hi Jason,
On 8/4/2023 6:44 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 06:42:15AM +0000, Vasant Hegde wrote:
>> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
>> introduced a variable struct iommu_device.max_pasids to track max
>> PASIDS supported by each IOMMU.
>>
>> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
>> to set max PASIDs per device (see __iommu_probe_device()).
>>
>> Also remove unused global 'amd_iommu_max_pasid' variable.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> drivers/iommu/amd/amd_iommu_types.h | 3 ---
>> drivers/iommu/amd/init.c | 9 ++-------
>> 2 files changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index e114e4af0f05..0d339e022572 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -895,9 +895,6 @@ extern unsigned amd_iommu_aperture_order;
>> /* allocation bitmap for domain ids */
>> extern unsigned long *amd_iommu_pd_alloc_bitmap;
>>
>> -/* Smallest max PASID supported by any IOMMU in the system */
>> -extern u32 amd_iommu_max_pasid;
>> -
>> extern bool amd_iommu_force_isolation;
>>
>> /* Max levels of glxval supported */
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 1f56478ae74e..e0b0e48d3d38 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -185,8 +185,6 @@ static int amd_iommus_present;
>> bool amd_iommu_np_cache __read_mostly;
>> bool amd_iommu_iotlb_sup __read_mostly = true;
>>
>> -u32 amd_iommu_max_pasid __read_mostly = ~0;
>> -
>> static bool amd_iommu_pc_present __read_mostly;
>> bool amdr_ivrs_remap_support __read_mostly;
>>
>> @@ -2086,16 +2084,13 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
>>
>> if (iommu_feature(iommu, FEATURE_GT)) {
>> int glxval;
>> - u32 max_pasid;
>> u64 pasmax;
>>
>> pasmax = iommu->features & FEATURE_PASID_MASK;
>> pasmax >>= FEATURE_PASID_SHIFT;
>> - max_pasid = (1 << (pasmax + 1)) - 1;
>> -
>> - amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
>> + iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
>>
>> - BUG_ON(amd_iommu_max_pasid & ~PASID_MASK);
>> + BUG_ON(iommu->iommu.max_pasids & ~PASID_MASK);
>
> You said you'd fix this to remove the BUG_ON.
>
Sorry. Missed to update patch comment section before sending.
For now we are just fixing the max_pasids support. Even current generations
supports upto 16bit PASIDs only.
We will consider removing BUG_ON later.
-Vasant
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 16/16] iommu/amd: Simplify amd_iommu_device_info()
2023-08-04 6:42 [PATCH v3 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring Vasant Hegde
` (14 preceding siblings ...)
2023-08-04 6:42 ` [PATCH v3 15/16] iommu/amd: Initialize iommu_device->max_pasids Vasant Hegde
@ 2023-08-04 6:42 ` Vasant Hegde
15 siblings, 0 replies; 33+ messages in thread
From: Vasant Hegde @ 2023-08-04 6:42 UTC (permalink / raw)
To: iommu, joro
Cc: suravee.suthikulpanit, wei.huang2, jsnitsel, jgg, Vasant Hegde,
Jason Gunthorpe
Let 'iommu_dev_data.flags' track device exec and priv support as well.
So that in amd_iommu_device_info() we don't need to check device
capabilities again.
Also in probe path __iommu_probe_device() updates dev->iommu->max_pasids.
Just use that instead of calculating max PASIDs again
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/amd/iommu.c | 41 ++++++++++++++-------------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 208d69e7a6b4..3c91b68ca8eb 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -321,6 +321,7 @@ static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
static u32 pdev_get_caps(struct pci_dev *pdev)
{
+ int features;
u32 flags = 0;
if (pci_ats_supported(pdev))
@@ -329,9 +330,17 @@ static u32 pdev_get_caps(struct pci_dev *pdev)
if (pci_pri_supported(pdev))
flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
- if (pci_pasid_features(pdev) >= 0)
+ features = pci_pasid_features(pdev);
+ if (features >= 0) {
flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
+ if (features & PCI_PASID_CAP_EXEC)
+ flags |= AMD_IOMMU_DEVICE_FLAG_EXEC_SUP;
+
+ if (features & PCI_PASID_CAP_PRIV)
+ flags |= AMD_IOMMU_DEVICE_FLAG_PRIV_SUP;
+ }
+
return flags;
}
@@ -2809,8 +2818,8 @@ EXPORT_SYMBOL(amd_iommu_complete_ppr);
int amd_iommu_device_info(struct pci_dev *pdev,
struct amd_iommu_device_info *info)
{
- int max_pasids;
- int pos;
+ struct device *dev = &pdev->dev;
+ struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
if (pdev == NULL || info == NULL)
return -EINVAL;
@@ -2820,29 +2829,9 @@ int amd_iommu_device_info(struct pci_dev *pdev,
memset(info, 0, sizeof(*info));
- if (pci_ats_supported(pdev))
- info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
-
- pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
- if (pos)
- info->flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
-
- pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
- if (pos) {
- int features;
-
- max_pasids = 1 << (9 * (amd_iommu_max_glx_val + 1));
- max_pasids = min(max_pasids, (1 << 20));
-
- info->flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
- info->max_pasids = min(pci_max_pasids(pdev), max_pasids);
-
- features = pci_pasid_features(pdev);
- if (features & PCI_PASID_CAP_EXEC)
- info->flags |= AMD_IOMMU_DEVICE_FLAG_EXEC_SUP;
- if (features & PCI_PASID_CAP_PRIV)
- info->flags |= AMD_IOMMU_DEVICE_FLAG_PRIV_SUP;
- }
+ info->flags = dev_data->flags;
+ if (dev->iommu)
+ info->max_pasids = dev->iommu->max_pasids;
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread