RE: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Monday, March 25, 2024 10:17 AM
> 
> +enum cache_tag_type {
> +	CACHE_TAG_TYPE_IOTLB,
> +	CACHE_TAG_TYPE_DEVTLB,
> +	CACHE_TAG_TYPE_PARENT_IOTLB,
> +	CACHE_TAG_TYPE_PARENT_DEVTLB,
> +};

'_TYPE_' can be removed to make it shorter

> +
> +/* Checks if an existing cache tag can be reused for a new association. */
> +static bool cache_tag_reusable(struct cache_tag *tag, u16 domain_id,
> +			       struct intel_iommu *iommu, struct device *dev,
> +			       ioasid_t pasid, enum cache_tag_type type)

cache_tage_match()

> +{
> +	if (tag->type != type)
> +		return false;
> +
> +	if (tag->domain_id != domain_id || tag->pasid != pasid)
> +		return false;
> +
> +	if (type == CACHE_TAG_TYPE_IOTLB)
> +		return tag->iommu == iommu;
> +
> +	if (type == CACHE_TAG_TYPE_DEVTLB)
> +		return tag->dev == dev;
> +
> +	return false;

why do you disallow PARENT_TYPE from reusing? It's not uncommon
to have two devices attached to a same nested domain hence with
the same parent domain. Disallowing tag reuse implies unnecessarily
duplicated cache flushes...

> +}
> +
> +/* Assign a cache tag with specified type to domain. */
> +static int cache_tag_assign(struct dmar_domain *domain, u16 did,
> +			    struct device *dev, ioasid_t pasid,
> +			    enum cache_tag_type type)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +	struct cache_tag *tag, *temp;
> +	unsigned long flags;
> +
> +	tag = kzalloc(sizeof(*tag), GFP_KERNEL);
> +	if (!tag)
> +		return -ENOMEM;
> +
> +	tag->type = type;
> +	tag->iommu = iommu;
> +	tag->dev = dev;

should we set tag->dev only for DEVTLB type? It's a bit confusing to set
it for IOTLB type which doesn't care about device. Actually doing so
is instead misleading as the 1st device creating the tag may have been
detached but then it will still show up in the trace when the last device
detach destroying the tag.

> +static int __cache_tag_assign_parent_domain(struct dmar_domain
> *domain, u16 did,
> +					    struct device *dev, ioasid_t pasid)

this pair is similar to the earlier one except the difference on type.

what about keeping just one pair which accepts a 'parent' argument to
decide the type internally?


> +/*
> + * Assigns cache tags to a domain when it's associated with a device's
> + * PASID using a specific domain ID.

s/Assigns/Assign/

> +
> +	did = domain_id_iommu(domain, iommu);
> +	ret = cache_tag_assign_domain(domain, did, dev,
> IOMMU_NO_PASID);

there are many occurrences of this pattern. What about passing in
a 'iommu' parameter and getting 'did' inside the helper? for svm
it can be specialized internally too.

> @@ -4607,10 +4623,11 @@ static void
> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  	 */
>  	if (domain->type == IOMMU_DOMAIN_SVA) {
>  		intel_svm_remove_dev_pasid(dev, pasid);
> +		cache_tag_unassign_domain(dmar_domain,
> +					  FLPT_DEFAULT_DID, dev, pasid);

is it correct to destroy the tag before teardown completes, e.g. iotlb still
needs to be flushed in intel_pasid_tear_down_entry()?