Re: [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices
On 2022/5/5 16:43, Tian, Kevin wrote: From: Lu Baolu 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices
> From: Lu Baolu > 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. > 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? > + domain_set_force_snooping(dmar_domain); > dmar_domain->force_snooping = true; > + > return true; > } > Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu