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

[Lu Baolu]

On 6/30/22 4:28 PM, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > 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