Re: [PATCH v2 09/17] iommufd: Add enforced_cache_coherency to iommufd_hw_pagetable_alloc()

[Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>]

On 2023/3/8 8:35, Jason Gunthorpe wrote:
> Logically the HWPT should have the coherency set properly for the device
> that it is being created for when it is created.
>
> This was happening implicitly if the immediate_attach was set because
> iommufd_hw_pagetable_attach() does it as the first thing.
>
> Do it unconditionally so !immediate_attach works properly.
>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
>   drivers/iommu/iommufd/device.c          | 20 +++++-------------
>   drivers/iommu/iommufd/hw_pagetable.c    | 27 +++++++++++++++++++++++++
>   drivers/iommu/iommufd/iommufd_private.h |  1 +
>   3 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index d6d5c2fcc33889..ddde14d6d1352c 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -293,21 +293,11 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>   	if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt)
>   		return -EINVAL;
>   
> -	/*
> -	 * Try to upgrade the domain we have, it is an iommu driver bug to
> -	 * report IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail
> -	 * enforce_cache_coherency when there are no devices attached to the
> -	 * domain.
> -	 */
> -	if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) {
> -		if (hwpt->domain->ops->enforce_cache_coherency)
> -			hwpt->enforce_cache_coherency =
> -				hwpt->domain->ops->enforce_cache_coherency(
> -					hwpt->domain);
> -		if (!hwpt->enforce_cache_coherency) {
> -			WARN_ON(list_empty(&idev->igroup->device_list));
> -			return -EINVAL;
> -		}
> +	/* Try to upgrade the domain we have */
> +	if (idev->enforce_cache_coherency) {
> +		rc = iommufd_hw_pagetable_enforce_cc(hwpt);
> +		if (rc)
> +			return rc;

As "HWPT should have the coherency set properly for the device that it
is being created for when it is created", is it an incompatible case if

	idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency

?

If so, why not,

	if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency)
		return -EINVAL;

?

Best regards,
baolu

>   	}
>   
>   	rc = iopt_table_enforce_dev_resv_regions(&hwpt->ioas->iopt, idev->dev,
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 566eba0cd9b917..2584f9038b29a2 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -25,6 +25,20 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)
>   	refcount_dec(&hwpt->ioas->obj.users);
>   }
>   
> +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
> +{
> +	if (hwpt->enforce_cache_coherency)
> +		return 0;
> +
> +	if (hwpt->domain->ops->enforce_cache_coherency)
> +		hwpt->enforce_cache_coherency =
> +			hwpt->domain->ops->enforce_cache_coherency(
> +				hwpt->domain);
> +	if (!hwpt->enforce_cache_coherency)
> +		return -EINVAL;
> +	return 0;
> +}
> +
>   /**
>    * iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device
>    * @ictx: iommufd context
> @@ -60,6 +74,19 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   		goto out_abort;
>   	}
>   
> +	/*
> +	 * Set the coherency mode before we do iopt_table_add_domain() as some
> +	 * iommus have a per-PTE bit that controls it and need to decide before
> +	 * doing any maps. It is an iommu driver bug to report
> +	 * IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail enforce_cache_coherency on
> +	 * a new domain.
> +	 */
> +	if (idev->enforce_cache_coherency) {
> +		rc = iommufd_hw_pagetable_enforce_cc(hwpt);
> +		if (WARN_ON(rc))
> +			goto out_abort;
> +	}
> +
>   	mutex_lock(&idev->igroup->lock);
>   
>   	/*
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 8bf053f4d4a9ce..471a3fdff1e0b6 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -254,6 +254,7 @@ struct iommufd_hw_pagetable {
>   struct iommufd_hw_pagetable *
>   iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   			   struct iommufd_device *idev, bool immediate_attach);
> +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt);
>   int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>   				struct iommufd_device *idev);
>   struct iommufd_hw_pagetable *