Re: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain

2022-06-30 Thread Lu Baolu 

On 6/30/22 4:28 PM, Tian, Kevin wrote:

From: Lu Baolu 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain

2022-06-30 Thread Tian, Kevin 
> From: Lu Baolu 
> 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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain

2022-06-25 Thread Lu Baolu 
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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -524,17 +525,15 @@ struct context_entry {
  */
 #define DOMAIN_FLAG_USE_FIRST_LEVELBIT(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 @@ s