Hi folks,
This is a follow-up series of changes proposed by this patch:
https://lore.kernel.org/linux-iommu/[email protected]/
It removes several static arrays of size DMAR_UNITS_SUPPORTED and sets
the DMAR_UNITS_SUPPORTED to 1024.
Please help review and suggest.
Best regards,
baolu
Lu Baolu (6):
iommu/vt-d: Remove unused domain_get_iommu()
iommu/vt-d: Use IDA interface to manage iommu sequence id
iommu/vt-d: Refactor iommu information of each domain
iommu/vt-d: Add VTD_FLAG_IOMMU_PROBED flag
iommu/vt-d: Remove global g_iommus array
iommu/vt-d: Make DMAR_UNITS_SUPPORTED default 1024
include/linux/dmar.h | 6 +-
drivers/iommu/intel/iommu.h | 29 ++++--
drivers/iommu/intel/dmar.c | 33 ++-----
drivers/iommu/intel/iommu.c | 188 ++++++++++++++----------------------
drivers/iommu/intel/pasid.c | 2 +-
drivers/iommu/intel/svm.c | 2 +-
6 files changed, 103 insertions(+), 157 deletions(-)
--
2.25.1
If the available hardware exceeds DMAR_UNITS_SUPPORTED (previously set
to MAX_IO_APICS, or 128), it causes these messages: "DMAR: Failed to
allocate seq_id", "DMAR: Parse DMAR table failure.", and "x2apic: IRQ
remapping doesn't support X2APIC mode x2apic disabled"; and the system
fails to boot properly.
To support up to 64 sockets with 10 DMAR units each (640), make the
value of DMAR_UNITS_SUPPORTED default 1024.
Signed-off-by: Steve Wahl<[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/dmar.h | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index cbd714a198a0..d81a51978d01 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -18,11 +18,7 @@
struct acpi_dmar_header;
-#ifdef CONFIG_X86
-# define DMAR_UNITS_SUPPORTED MAX_IO_APICS
-#else
-# define DMAR_UNITS_SUPPORTED 64
-#endif
+#define DMAR_UNITS_SUPPORTED 1024
/* DMAR Flags */
#define DMAR_INTR_REMAP 0x1
--
2.25.1
The g_iommus is not used anywhere. Remove it to avoid dead code.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 42 -------------------------------------
1 file changed, 42 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f6d7055cffd7..9a284394b2c5 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -126,9 +126,6 @@ static inline unsigned long virt_to_dma_pfn(void *p)
return page_to_dma_pfn(virt_to_page(p));
}
-/* global iommu list, set NULL for ignored DMAR units */
-static struct intel_iommu **g_iommus;
-
static void __init check_tylersburg_isoch(void);
static int rwbf_quirk;
@@ -287,9 +284,6 @@ static LIST_HEAD(dmar_satc_units);
#define for_each_rmrr_units(rmrr) \
list_for_each_entry(rmrr, &dmar_rmrr_units, list)
-/* bitmap for indexing intel_iommus */
-static int g_num_of_iommus;
-
static void dmar_remove_one_dev_info(struct device *dev);
int dmar_disabled = !IS_ENABLED(CONFIG_INTEL_IOMMU_DEFAULT_ON);
@@ -1694,7 +1688,6 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
iommu->domain_ids = NULL;
}
- g_iommus[iommu->seq_id] = NULL;
iommu->flags &= ~VTD_FLAG_IOMMU_PROBED;
/* free context mapping */
@@ -2899,36 +2892,6 @@ static int __init init_dmars(void)
struct intel_iommu *iommu;
int ret;
- /*
- * for each drhd
- * allocate root
- * initialize and program root entry to not present
- * endfor
- */
- for_each_drhd_unit(drhd) {
- /*
- * lock not needed as this is only incremented in the single
- * threaded kernel __init code path all other access are read
- * only
- */
- if (g_num_of_iommus < DMAR_UNITS_SUPPORTED) {
- g_num_of_iommus++;
- continue;
- }
- pr_err_once("Exceeded %d IOMMUs\n", DMAR_UNITS_SUPPORTED);
- }
-
- /* Preallocate enough resources for IOMMU hot-addition */
- if (g_num_of_iommus < DMAR_UNITS_SUPPORTED)
- g_num_of_iommus = DMAR_UNITS_SUPPORTED;
-
- g_iommus = kcalloc(g_num_of_iommus, sizeof(struct intel_iommu *),
- GFP_KERNEL);
- if (!g_iommus) {
- ret = -ENOMEM;
- goto error;
- }
-
ret = intel_cap_audit(CAP_AUDIT_STATIC_DMAR, NULL);
if (ret)
goto free_iommu;
@@ -2951,7 +2914,6 @@ static int __init init_dmars(void)
intel_pasid_max_id);
}
- g_iommus[iommu->seq_id] = iommu;
iommu->flags |= VTD_FLAG_IOMMU_PROBED;
intel_iommu_init_qi(iommu);
@@ -3079,9 +3041,6 @@ static int __init init_dmars(void)
free_dmar_iommu(iommu);
}
- kfree(g_iommus);
-
-error:
return ret;
}
@@ -3488,7 +3447,6 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
if (iommu->gcmd & DMA_GCMD_TE)
iommu_disable_translation(iommu);
- g_iommus[iommu->seq_id] = iommu;
iommu->flags |= VTD_FLAG_IOMMU_PROBED;
ret = iommu_init_domains(iommu);
if (ret == 0)
--
2.25.1
When a DMA domain is attached to a device, it needs to allocate a domain
ID from its IOMMU. Currently, the domain ID information is stored in two
static arrays embedded in the domain structure. This can lead to memory
waste when the driver is running on a small platform.
This optimizes these static arrays by replacing them with an xarray and
consuming memory on demand.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.h | 27 +++++---
drivers/iommu/intel/iommu.c | 123 ++++++++++++++++++++----------------
drivers/iommu/intel/pasid.c | 2 +-
drivers/iommu/intel/svm.c | 2 +-
4 files changed, 90 insertions(+), 64 deletions(-)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 56e0d8cd2102..56c3d1a9e155 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -21,6 +21,7 @@
#include <linux/dmar.h>
#include <linux/ioasid.h>
#include <linux/bitfield.h>
+#include <linux/xarray.h>
#include <asm/cacheflush.h>
#include <asm/iommu.h>
@@ -524,17 +525,15 @@ struct context_entry {
*/
#define DOMAIN_FLAG_USE_FIRST_LEVEL BIT(1)
+struct iommu_domain_info {
+ struct intel_iommu *iommu;
+ unsigned int refcnt;
+ u16 did;
+};
+
struct dmar_domain {
int nid; /* node id */
-
- unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
- /* Refcount of devices per iommu */
-
-
- u16 iommu_did[DMAR_UNITS_SUPPORTED];
- /* Domain ids per IOMMU. Use u16 since
- * domain ids are 16 bit wide according
- * to VT-d spec, section 9.3 */
+ struct xarray iommu_array; /* Attached IOMMU array */
u8 has_iotlb_device: 1;
u8 iommu_coherency: 1; /* indicate coherency of iommu access */
@@ -640,6 +639,16 @@ static inline struct dmar_domain *to_dmar_domain(struct iommu_domain *dom)
return container_of(dom, struct dmar_domain, domain);
}
+/* Retrieve the domain ID which has allocated to the domain */
+static inline u16
+domain_id_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
+{
+ struct iommu_domain_info *info =
+ xa_load(&domain->iommu_array, iommu->seq_id);
+
+ return info->did;
+}
+
/*
* 0: readable
* 1: writable
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 781e060352e6..78b26fef685e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -254,10 +254,6 @@ static inline void context_clear_entry(struct context_entry *context)
static struct dmar_domain *si_domain;
static int hw_pass_through = 1;
-#define for_each_domain_iommu(idx, domain) \
- for (idx = 0; idx < g_num_of_iommus; idx++) \
- if (domain->iommu_refcnt[idx])
-
struct dmar_rmrr_unit {
struct list_head list; /* list of rmrr units */
struct acpi_dmar_header *hdr; /* ACPI header */
@@ -453,16 +449,16 @@ static inline bool iommu_paging_structure_coherency(struct intel_iommu *iommu)
static void domain_update_iommu_coherency(struct dmar_domain *domain)
{
+ struct iommu_domain_info *info;
struct dmar_drhd_unit *drhd;
struct intel_iommu *iommu;
bool found = false;
- int i;
+ unsigned long i;
domain->iommu_coherency = true;
-
- for_each_domain_iommu(i, domain) {
+ xa_for_each(&domain->iommu_array, i, info) {
found = true;
- if (!iommu_paging_structure_coherency(g_iommus[i])) {
+ if (!iommu_paging_structure_coherency(info->iommu)) {
domain->iommu_coherency = false;
break;
}
@@ -1495,7 +1491,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
unsigned int aligned_pages = __roundup_pow_of_two(pages);
unsigned int mask = ilog2(aligned_pages);
uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
- u16 did = domain->iommu_did[iommu->seq_id];
+ u16 did = domain_id_iommu(domain, iommu);
BUG_ON(pages == 0);
@@ -1565,11 +1561,12 @@ static inline void __mapping_notify_one(struct intel_iommu *iommu,
static void intel_flush_iotlb_all(struct iommu_domain *domain)
{
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
- int idx;
+ struct iommu_domain_info *info;
+ unsigned long idx;
- for_each_domain_iommu(idx, dmar_domain) {
- struct intel_iommu *iommu = g_iommus[idx];
- u16 did = dmar_domain->iommu_did[iommu->seq_id];
+ xa_for_each(&dmar_domain->iommu_array, idx, info) {
+ struct intel_iommu *iommu = info->iommu;
+ u16 did = domain_id_iommu(dmar_domain, iommu);
if (domain_use_first_level(dmar_domain))
qi_flush_piotlb(iommu, did, PASID_RID2PASID, 0, -1, 0);
@@ -1745,6 +1742,7 @@ static struct dmar_domain *alloc_domain(unsigned int type)
domain->has_iotlb_device = false;
INIT_LIST_HEAD(&domain->devices);
spin_lock_init(&domain->lock);
+ xa_init(&domain->iommu_array);
return domain;
}
@@ -1752,45 +1750,64 @@ static struct dmar_domain *alloc_domain(unsigned int type)
static int domain_attach_iommu(struct dmar_domain *domain,
struct intel_iommu *iommu)
{
+ struct iommu_domain_info *info, *curr;
unsigned long ndomains;
- int num, ret = 0;
+ int num, ret = -ENOSPC;
- spin_lock(&iommu->lock);
- domain->iommu_refcnt[iommu->seq_id] += 1;
- if (domain->iommu_refcnt[iommu->seq_id] == 1) {
- ndomains = cap_ndoms(iommu->cap);
- num = find_first_zero_bit(iommu->domain_ids, ndomains);
-
- if (num >= ndomains) {
- pr_err("%s: No free domain ids\n", iommu->name);
- domain->iommu_refcnt[iommu->seq_id] -= 1;
- ret = -ENOSPC;
- goto out_unlock;
- }
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
- set_bit(num, iommu->domain_ids);
- domain->iommu_did[iommu->seq_id] = num;
- domain->nid = iommu->node;
- domain_update_iommu_cap(domain);
+ spin_lock(&iommu->lock);
+ curr = xa_load(&domain->iommu_array, iommu->seq_id);
+ if (curr) {
+ curr->refcnt++;
+ kfree(info);
+ goto success;
}
-out_unlock:
+ ndomains = cap_ndoms(iommu->cap);
+ num = find_first_zero_bit(iommu->domain_ids, ndomains);
+ if (num >= ndomains) {
+ pr_err("%s: No free domain ids\n", iommu->name);
+ goto err_unlock;
+ }
+
+ set_bit(num, iommu->domain_ids);
+ info->refcnt = 1;
+ info->did = num;
+ info->iommu = iommu;
+ domain->nid = iommu->node;
+ ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
+ info, GFP_ATOMIC));
+ if (ret)
+ goto err_clear;
+ domain_update_iommu_cap(domain);
+
+success:
+ spin_unlock(&iommu->lock);
+ return 0;
+
+err_clear:
+ clear_bit(info->did, iommu->domain_ids);
+err_unlock:
spin_unlock(&iommu->lock);
+ kfree(info);
return ret;
}
static void domain_detach_iommu(struct dmar_domain *domain,
struct intel_iommu *iommu)
{
- int num;
+ struct iommu_domain_info *info;
spin_lock(&iommu->lock);
- domain->iommu_refcnt[iommu->seq_id] -= 1;
- if (domain->iommu_refcnt[iommu->seq_id] == 0) {
- num = domain->iommu_did[iommu->seq_id];
- clear_bit(num, iommu->domain_ids);
+ info = xa_load(&domain->iommu_array, iommu->seq_id);
+ if (--info->refcnt == 0) {
+ clear_bit(info->did, iommu->domain_ids);
+ xa_erase(&domain->iommu_array, iommu->seq_id);
domain_update_iommu_cap(domain);
- domain->iommu_did[iommu->seq_id] = 0;
+ kfree(info);
}
spin_unlock(&iommu->lock);
}
@@ -1880,7 +1897,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
{
struct device_domain_info *info =
iommu_support_dev_iotlb(domain, iommu, bus, devfn);
- u16 did = domain->iommu_did[iommu->seq_id];
+ u16 did = domain_id_iommu(domain, iommu);
int translation = CONTEXT_TT_MULTI_LEVEL;
struct context_entry *context;
int ret;
@@ -2130,8 +2147,9 @@ static void switch_to_super_page(struct dmar_domain *domain,
unsigned long end_pfn, int level)
{
unsigned long lvl_pages = lvl_to_nr_pages(level);
+ struct iommu_domain_info *info;
struct dma_pte *pte = NULL;
- int i;
+ unsigned long i;
while (start_pfn <= end_pfn) {
if (!pte)
@@ -2142,8 +2160,8 @@ static void switch_to_super_page(struct dmar_domain *domain,
start_pfn + lvl_pages - 1,
level + 1);
- for_each_domain_iommu(i, domain)
- iommu_flush_iotlb_psi(g_iommus[i], domain,
+ xa_for_each(&domain->iommu_array, i, info)
+ iommu_flush_iotlb_psi(info->iommu, domain,
start_pfn, lvl_pages,
0, 0);
}
@@ -2273,7 +2291,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
if (hw_pass_through && domain_type_is_si(info->domain))
did_old = FLPT_DEFAULT_DID;
else
- did_old = info->domain->iommu_did[iommu->seq_id];
+ did_old = domain_id_iommu(info->domain, iommu);
} else {
did_old = context_domain_id(context);
}
@@ -2331,7 +2349,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
flags |= PASID_FLAG_PAGE_SNOOP;
return intel_pasid_setup_first_level(iommu, dev, (pgd_t *)pgd, pasid,
- domain->iommu_did[iommu->seq_id],
+ domain_id_iommu(domain, iommu),
flags);
}
@@ -4369,15 +4387,16 @@ static void intel_iommu_tlb_sync(struct iommu_domain *domain,
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
unsigned long iova_pfn = IOVA_PFN(gather->start);
size_t size = gather->end - gather->start;
+ struct iommu_domain_info *info;
unsigned long start_pfn;
unsigned long nrpages;
- int iommu_id;
+ unsigned long i;
nrpages = aligned_nrpages(gather->start, size);
start_pfn = mm_to_dma_pfn(iova_pfn);
- for_each_domain_iommu(iommu_id, dmar_domain)
- iommu_flush_iotlb_psi(g_iommus[iommu_id], dmar_domain,
+ xa_for_each(&dmar_domain->iommu_array, i, info)
+ iommu_flush_iotlb_psi(info->iommu, dmar_domain,
start_pfn, nrpages,
list_empty(&gather->freelist), 0);
@@ -4622,7 +4641,7 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev)
context[0].lo = ctx_lo;
wmb();
iommu->flush.flush_context(iommu,
- domain->iommu_did[iommu->seq_id],
+ domain_id_iommu(domain, iommu),
PCI_DEVID(info->bus, info->devfn),
DMA_CCMD_MASK_NOBIT,
DMA_CCMD_DEVICE_INVL);
@@ -4759,13 +4778,11 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
unsigned long pages = aligned_nrpages(iova, size);
unsigned long pfn = iova >> VTD_PAGE_SHIFT;
- struct intel_iommu *iommu;
- int iommu_id;
+ struct iommu_domain_info *info;
+ unsigned long i;
- for_each_domain_iommu(iommu_id, dmar_domain) {
- iommu = g_iommus[iommu_id];
- __mapping_notify_one(iommu, dmar_domain, pfn, pages);
- }
+ xa_for_each(&dmar_domain->iommu_array, i, info)
+ __mapping_notify_one(info->iommu, dmar_domain, pfn, pages);
}
const struct iommu_ops intel_iommu_ops = {
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c30de2339de5..006026dc18d4 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -626,7 +626,7 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
}
pgd_val = virt_to_phys(pgd);
- did = domain->iommu_did[iommu->seq_id];
+ did = domain_id_iommu(domain, iommu);
spin_lock(&iommu->lock);
pte = intel_pasid_get_entry(dev, pasid);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 64072e628bbd..8bcfb93dda56 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -541,7 +541,7 @@ static void intel_svm_drain_prq(struct device *dev, u32 pasid)
domain = info->domain;
pdev = to_pci_dev(dev);
sid = PCI_DEVID(info->bus, info->devfn);
- did = domain->iommu_did[iommu->seq_id];
+ did = domain_id_iommu(domain, iommu);
qdep = pci_ats_queue_depth(pdev);
/*
--
2.25.1
In the IOMMU hot-add path, there's a need to check whether an IOMMU
has been probed. Instead of checking the IOMMU pointer in the global
list, it's better to allocate a flag bit in iommu->flags for this
purpose.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.h | 1 +
drivers/iommu/intel/iommu.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 56c3d1a9e155..105a1e7c60d9 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -479,6 +479,7 @@ enum {
#define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
#define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
#define VTD_FLAG_SVM_CAPABLE (1 << 2)
+#define VTD_FLAG_IOMMU_PROBED (1 << 3)
extern int intel_iommu_sm;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 78b26fef685e..f6d7055cffd7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1695,6 +1695,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
}
g_iommus[iommu->seq_id] = NULL;
+ iommu->flags &= ~VTD_FLAG_IOMMU_PROBED;
/* free context mapping */
free_context_table(iommu);
@@ -2951,6 +2952,7 @@ static int __init init_dmars(void)
}
g_iommus[iommu->seq_id] = iommu;
+ iommu->flags |= VTD_FLAG_IOMMU_PROBED;
intel_iommu_init_qi(iommu);
@@ -3460,7 +3462,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
int sp, ret;
struct intel_iommu *iommu = dmaru->iommu;
- if (g_iommus[iommu->seq_id])
+ if (iommu->flags & VTD_FLAG_IOMMU_PROBED)
return 0;
ret = intel_cap_audit(CAP_AUDIT_HOTPLUG_DMAR, iommu);
@@ -3487,6 +3489,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
iommu_disable_translation(iommu);
g_iommus[iommu->seq_id] = iommu;
+ iommu->flags |= VTD_FLAG_IOMMU_PROBED;
ret = iommu_init_domains(iommu);
if (ret == 0)
ret = iommu_alloc_root_entry(iommu);
--
2.25.1
On 2022/6/25 20:51, Lu Baolu wrote:
> Hi folks,
>
> This is a follow-up series of changes proposed by this patch:
>
> https://lore.kernel.org/linux-iommu/[email protected]/
>
> It removes several static arrays of size DMAR_UNITS_SUPPORTED and sets
> the DMAR_UNITS_SUPPORTED to 1024.
>
> Please help review and suggest.
This series is also available on github:
https://github.com/LuBaolu/intel-iommu/commits/vtd-next-for-v5.20
Best regards,
baolu
>
> Best regards,
> baolu
>
> Lu Baolu (6):
> iommu/vt-d: Remove unused domain_get_iommu()
> iommu/vt-d: Use IDA interface to manage iommu sequence id
> iommu/vt-d: Refactor iommu information of each domain
> iommu/vt-d: Add VTD_FLAG_IOMMU_PROBED flag
> iommu/vt-d: Remove global g_iommus array
> iommu/vt-d: Make DMAR_UNITS_SUPPORTED default 1024
>
> include/linux/dmar.h | 6 +-
> drivers/iommu/intel/iommu.h | 29 ++++--
> drivers/iommu/intel/dmar.c | 33 ++-----
> drivers/iommu/intel/iommu.c | 188 ++++++++++++++----------------------
> drivers/iommu/intel/pasid.c | 2 +-
> drivers/iommu/intel/svm.c | 2 +-
> 6 files changed, 103 insertions(+), 157 deletions(-)
>
> From: Lu Baolu <[email protected]>
> Sent: Saturday, June 25, 2022 8:52 PM
>
> In the IOMMU hot-add path, there's a need to check whether an IOMMU
> has been probed. Instead of checking the IOMMU pointer in the global
> list, it's better to allocate a flag bit in iommu->flags for this
> purpose.
Sorry I didn't get the point of original check. This is the hotplug path
hence the caller of this function should already figure out it's a new
iommu before reaching this point?
> From: Lu Baolu <[email protected]>
> Sent: Saturday, June 25, 2022 8:52 PM
>
> +struct iommu_domain_info {
> + struct intel_iommu *iommu;
> + unsigned int refcnt;
> + u16 did;
> +};
> +
> struct dmar_domain {
> int nid; /* node id */
> -
> - unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
> - /* Refcount of devices per iommu */
> -
> -
> - u16 iommu_did[DMAR_UNITS_SUPPORTED];
> - /* Domain ids per IOMMU. Use u16
> since
> - * domain ids are 16 bit wide
> according
> - * to VT-d spec, section 9.3 */
It'd make sense to keep the comments when moving above fields.
> + spin_lock(&iommu->lock);
> + curr = xa_load(&domain->iommu_array, iommu->seq_id);
> + if (curr) {
> + curr->refcnt++;
> + kfree(info);
> + goto success;
Not a fan of adding a label for success. Just putting two lines (unlock+
return) here is clearer.
> + ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> + info, GFP_ATOMIC));
check xa_err in separate line.
>
> static void domain_detach_iommu(struct dmar_domain *domain,
> struct intel_iommu *iommu)
> {
> - int num;
> + struct iommu_domain_info *info;
>
> spin_lock(&iommu->lock);
> - domain->iommu_refcnt[iommu->seq_id] -= 1;
> - if (domain->iommu_refcnt[iommu->seq_id] == 0) {
> - num = domain->iommu_did[iommu->seq_id];
> - clear_bit(num, iommu->domain_ids);
> + info = xa_load(&domain->iommu_array, iommu->seq_id);
> + if (--info->refcnt == 0) {
> + clear_bit(info->did, iommu->domain_ids);
> + xa_erase(&domain->iommu_array, iommu->seq_id);
> domain_update_iommu_cap(domain);
> - domain->iommu_did[iommu->seq_id] = 0;
> + kfree(info);
domain->nid may still point to the node of the removed iommu.
> From: Lu Baolu <[email protected]>
> Sent: Saturday, June 25, 2022 8:52 PM
>
> The g_iommus is not used anywhere. Remove it to avoid dead code.
>
> Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> From: Lu Baolu <[email protected]>
> Sent: Saturday, June 25, 2022 8:52 PM
>
> If the available hardware exceeds DMAR_UNITS_SUPPORTED (previously set
> to MAX_IO_APICS, or 128), it causes these messages: "DMAR: Failed to
> allocate seq_id", "DMAR: Parse DMAR table failure.", and "x2apic: IRQ
> remapping doesn't support X2APIC mode x2apic disabled"; and the system
> fails to boot properly.
>
> To support up to 64 sockets with 10 DMAR units each (640), make the
> value of DMAR_UNITS_SUPPORTED default 1024.
>
> Signed-off-by: Steve Wahl<[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
On 6/30/22 4:28 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Saturday, June 25, 2022 8:52 PM
>>
>> +struct iommu_domain_info {
>> + struct intel_iommu *iommu;
>> + unsigned int refcnt;
>> + u16 did;
>> +};
>> +
>> struct dmar_domain {
>> int nid; /* node id */
>> -
>> - unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
>> - /* Refcount of devices per iommu */
>> -
>> -
>> - u16 iommu_did[DMAR_UNITS_SUPPORTED];
>> - /* Domain ids per IOMMU. Use u16
>> since
>> - * domain ids are 16 bit wide
>> according
>> - * to VT-d spec, section 9.3 */
>
> It'd make sense to keep the comments when moving above fields.
Sure. Updated.
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 56c3d1a9e155..fae45bbb0c7f 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -527,8 +527,10 @@ struct context_entry {
struct iommu_domain_info {
struct intel_iommu *iommu;
- unsigned int refcnt;
- u16 did;
+ unsigned int refcnt; /* Refcount of devices per iommu */
+ u16 did; /* Domain ids per IOMMU. Use u16
since
+ * domain ids are 16 bit wide
according
+ * to VT-d spec, section 9.3 */
};
>
>> + spin_lock(&iommu->lock);
>> + curr = xa_load(&domain->iommu_array, iommu->seq_id);
>> + if (curr) {
>> + curr->refcnt++;
>> + kfree(info);
>> + goto success;
>
> Not a fan of adding a label for success. Just putting two lines (unlock+
> return) here is clearer.
Updated.
>
>> + ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> + info, GFP_ATOMIC));
>
> check xa_err in separate line.
Sure. Updated as below:
@@ -1778,13 +1780,14 @@ static int domain_attach_iommu(struct
dmar_domain *domain,
info->did = num;
info->iommu = iommu;
domain->nid = iommu->node;
- ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
- info, GFP_ATOMIC));
- if (ret)
+ curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
+ NULL, info, GFP_ATOMIC);
+ if (curr) {
+ ret = xa_err(curr) ? : -EBUSY;
goto err_clear;
+ }
domain_update_iommu_cap(domain);
>
>>
>> static void domain_detach_iommu(struct dmar_domain *domain,
>> struct intel_iommu *iommu)
>> {
>> - int num;
>> + struct iommu_domain_info *info;
>>
>> spin_lock(&iommu->lock);
>> - domain->iommu_refcnt[iommu->seq_id] -= 1;
>> - if (domain->iommu_refcnt[iommu->seq_id] == 0) {
>> - num = domain->iommu_did[iommu->seq_id];
>> - clear_bit(num, iommu->domain_ids);
>> + info = xa_load(&domain->iommu_array, iommu->seq_id);
>> + if (--info->refcnt == 0) {
>> + clear_bit(info->did, iommu->domain_ids);
>> + xa_erase(&domain->iommu_array, iommu->seq_id);
>> domain_update_iommu_cap(domain);
>> - domain->iommu_did[iommu->seq_id] = 0;
>> + kfree(info);
>
> domain->nid may still point to the node of the removed iommu.
Good catch! I should assign it to NUMA_NO_NODE, so that it could be
updated in the next domain_update_iommu_cap(). Updated as below:
@@ -1806,6 +1809,7 @@ static void domain_detach_iommu(struct dmar_domain
*domain,
if (--info->refcnt == 0) {
clear_bit(info->did, iommu->domain_ids);
xa_erase(&domain->iommu_array, iommu->seq_id);
+ domain->nid = NUMA_NO_NODE;
domain_update_iommu_cap(domain);
kfree(info);
}
Best regards,
baolu
On 6/30/22 4:29 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Saturday, June 25, 2022 8:52 PM
>>
>> In the IOMMU hot-add path, there's a need to check whether an IOMMU
>> has been probed. Instead of checking the IOMMU pointer in the global
>> list, it's better to allocate a flag bit in iommu->flags for this
>> purpose.
>
> Sorry I didn't get the point of original check. This is the hotplug path
> hence the caller of this function should already figure out it's a new
> iommu before reaching this point?
>
Either did I. It was added by below commit without any comments about
this check.
commit ffebeb46dd34736c90ffbca1ccb0bef8f4827c44
Author: Jiang Liu <[email protected]>
Date: Sun Nov 9 22:48:02 2014 +0800
iommu/vt-d: Enhance intel-iommu driver to support DMAR unit hotplug
Implement required callback functions for intel-iommu driver
to support DMAR unit hotplug.
Signed-off-by: Jiang Liu <[email protected]>
Reviewed-by: Yijing Wang <[email protected]>
Signed-off-by: Joerg Roedel <[email protected]>
I went through the whole hot-add process and found this check seemed to
be duplicate.
Hot-add process starts from dmar_device_hotplug(), it uses a rwlock to
synchronize the hot-add paths.
2386 down_write(&dmar_global_lock);
2387 if (insert)
2388 ret = dmar_hotplug_insert(tmp);
2389 else
2390 ret = dmar_hotplug_remove(tmp);
2391 up_write(&dmar_global_lock);
dmar_device_hotplug()
->dmar_hotplug_insert()
-->dmar_parse_one_drhd() /* the added intel_iommu is allocated here*/
-->dmar_hp_add_drhd() /* the intel_iommu is about to bring up */
--->intel_iommu_add()
The duplicate check here:
if (g_iommus[iommu->seq_id])
return 0;
All the iommu units are allocated and then initialized in the same
synchronized path. There is no need to check a duplicate initialization.
I would like to remove this check if no objection.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Friday, July 1, 2022 11:13 AM
>
> On 6/30/22 4:29 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <[email protected]>
> >> Sent: Saturday, June 25, 2022 8:52 PM
> >>
> >> In the IOMMU hot-add path, there's a need to check whether an IOMMU
> >> has been probed. Instead of checking the IOMMU pointer in the global
> >> list, it's better to allocate a flag bit in iommu->flags for this
> >> purpose.
> >
> > Sorry I didn't get the point of original check. This is the hotplug path
> > hence the caller of this function should already figure out it's a new
> > iommu before reaching this point?
> >
>
> Either did I. It was added by below commit without any comments about
> this check.
>
> commit ffebeb46dd34736c90ffbca1ccb0bef8f4827c44
> Author: Jiang Liu <[email protected]>
> Date: Sun Nov 9 22:48:02 2014 +0800
>
> iommu/vt-d: Enhance intel-iommu driver to support DMAR unit hotplug
>
> Implement required callback functions for intel-iommu driver
> to support DMAR unit hotplug.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Reviewed-by: Yijing Wang <[email protected]>
> Signed-off-by: Joerg Roedel <[email protected]>
>
> I went through the whole hot-add process and found this check seemed to
> be duplicate.
>
> Hot-add process starts from dmar_device_hotplug(), it uses a rwlock to
> synchronize the hot-add paths.
>
> 2386 down_write(&dmar_global_lock);
> 2387 if (insert)
> 2388 ret = dmar_hotplug_insert(tmp);
> 2389 else
> 2390 ret = dmar_hotplug_remove(tmp);
> 2391 up_write(&dmar_global_lock);
>
> dmar_device_hotplug()
> ->dmar_hotplug_insert()
> -->dmar_parse_one_drhd() /* the added intel_iommu is allocated here*/
> -->dmar_hp_add_drhd() /* the intel_iommu is about to bring up */
> --->intel_iommu_add()
>
> The duplicate check here:
>
> if (g_iommus[iommu->seq_id])
> return 0;
>
> All the iommu units are allocated and then initialized in the same
> synchronized path. There is no need to check a duplicate initialization.
>
> I would like to remove this check if no objection.
>
This matches my impression.
On Sat, Jun 25, 2022 at 08:51:58PM +0800, Lu Baolu wrote:
> Hi folks,
>
> This is a follow-up series of changes proposed by this patch:
>
> https://lore.kernel.org/linux-iommu/[email protected]/
>
> It removes several static arrays of size DMAR_UNITS_SUPPORTED and sets
> the DMAR_UNITS_SUPPORTED to 1024.
>
After Kevin Tian's comments, for the whole series:
Reviewed-by: Steve Wahl <[email protected]>
--> Steve
--
Steve Wahl, Hewlett Packard Enterprise