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

[Baolu Lu]

On 2022/5/5 16:43, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > 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