lkml.org 
[lkml]   [2022]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain
From
On 6/30/22 4:28 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> 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

\
 
 \ /
  Last update: 2022-07-01 04:41    [W:0.053 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site