Re: [PATCH] iommu/vt-d: Check for non-NULL domain on device release

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/1/16 23:22, Jason Gunthorpe wrote:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 6fb5f6fceea1..26e450259889 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3750,7 +3750,6 @@ static void domain_context_clear(struct device_domain_info *info)
>>   static void dmar_remove_one_dev_info(struct device *dev)
>>   {
>>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>> -	struct dmar_domain *domain = info->domain;
>>   	struct intel_iommu *iommu = info->iommu;
>>   	unsigned long flags;
>>   
>> @@ -3763,11 +3762,14 @@ static void dmar_remove_one_dev_info(struct device *dev)
>>   		domain_context_clear(info);
>>   	}
>>   
>> -	spin_lock_irqsave(&domain->lock, flags);
>> +	if (!info->domain)
>> +		return;
>> +
>> +	spin_lock_irqsave(&info->domain->lock, flags);
>>   	list_del(&info->link);
>> -	spin_unlock_irqrestore(&domain->lock, flags);
>> +	spin_unlock_irqrestore(&info->domain->lock, flags);
>>   
>> -	domain_detach_iommu(domain, iommu);
>> +	domain_detach_iommu(info->domain, iommu);
>>   	info->domain = NULL;
>>   }
> This function is almost a copy of device_block_translation(), with
> some bugs added :\
> 
> Lu can they be made the same? It seems to me BLOCKED could always
> clear the domain context, even in scalable mode?

Yes. A part of dmar_remove_one_dev_info() is duplicated with
device_block_translation().

> 
> Anyhow, my suggestion is more like:

I like this suggestion.

Currently, the device_release callback for an iommu driver does the
following:

a) Silent IOMMU DMA translation. This is done by detaching the existing
    domain from the IOMMU and bringing the IOMMU into a blocking state
    (some drivers might be in identity state);
b) Releases the resources allocated in the probe callback and restores
    the device to its previous state before the probe.

 From my understanding of your suggestion, we should move a) out of the
release callback and make it a special domain, which could be a blocking
domain or identity domain, depending on the iommu hardware.

In the end, the release_device callback of an iommu driver should focus
on the opposite operation of device_probe. This makes the concept
clearer.

Best regards,
baolu