Re: [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices

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

On 2022/5/5 16:43, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, May 5, 2022 9:07 AM
>>
>> As domain->force_snooping only impacts the devices attached with the
>> domain, there's no need to check against all IOMMU units. At the same
>> time, for a brand new domain (hasn't been attached to any device), the
>> force_snooping field could be set, but the attach_dev callback will
>> return failure if it wants to attach to a device which IOMMU has no
>> snoop control capability.
> 
> The description about brand new domain is not very clear. I think the
> point here is that force_snooping could be set on a domain no matter
> whether it has been attached or not and once set it is an immutable
> flag. If no device attached the operation always succeeds then this
> empty domain can be only attached to a device of which the IOMMU
> supports snoop control.

Exactly. Will update this description.

> 
>>   static bool intel_iommu_enforce_cache_coherency(struct iommu_domain
>> *domain)
>>   {
>>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>>
>> -	if (!domain_update_iommu_snooping(NULL))
>> +	if (dmar_domain->force_snooping)
>> +		return true;
>> +
>> +	if (!domain_support_force_snooping(dmar_domain))
>>   		return false;
>> +
> 
> Who guarantees that domain->devices won't change between
> above condition check and following set operation?

Good catch. Should lift the lock up here.

> 
>> +	domain_set_force_snooping(dmar_domain);
>>   	dmar_domain->force_snooping = true;
>> +
>>   	return true;
>>   }
>>
> 
> Thanks
> Kevin

Best regards,
baolu