Re: [PATCH v3] xen/passthrough: Support a single iommu_domain per xen domain per SMMU

[Julien Grall <julien.grall@linaro.org>]

Hello Robert,

On 24/03/2015 16:42, Robbie VanVossen wrote:
> If multiple devices are being passed through to the same domain and they
> share a single SMMU, then they only require a single iommu_domain.
>
> In arm_smmu_assign_dev, before a new iommu_domain is created, the
> xen_domain->contexts is checked for any iommu_domains that are already
> assigned to device that uses the same SMMU as the current device. If one
> is found, attach the device to that iommu_domain. If a new one isn't
> found, create a new iommu_domain just like before.
>
> The arm_smmu_deassign_dev function assumes that there is a single
> device per iommu_domain. This meant that when the first device was
> deassigned, the iommu_domain was freed and when another device was
> deassigned a crash occured in xen.
>
> To fix this, a reference counter was added to the iommu_domain struct.
> When an arm_smmu_xen_device references an iommu_domain, the
> iommu_domains ref is incremented. When that reference is removed, the
> iommu_domains ref is decremented. The iommu_domain will only be freed
> when the ref is 0.
>
> Signed-off-by: Robbie VanVossen <robert.vanvossen@dornerworks.com>
>
> ---
> Changed since v2:
>    * Fixed coding style
>    * Removed an unnecessary error message
>    * Added some helper functions to clean up the workflow in arm_smmu_assign_dev a bit
> Changed since v1:
>    * Fixed coding style for comments
>    * Move increment/decrement outside of attach/detach functions
>    * Expanded xen_domain->lock to protect more of the assign/deassign
>      functions
>    * Removed iommu_domain add/remove_device functions
> ---
>   xen/drivers/passthrough/arm/smmu.c |   99 ++++++++++++++++++++++++++----------
>   1 file changed, 71 insertions(+), 28 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index a7a7da9..be0ecdc 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -223,6 +223,7 @@ struct iommu_domain
>   	/* Runtime SMMU configuration for this iommu_domain */
>   	struct arm_smmu_domain		*priv;
>
> +	atomic_t ref;
>   	/* Used to link iommu_domain contexts for a same domain.
>   	 * There is at least one per-SMMU to used by the domain.
>   	 * */
> @@ -2564,12 +2565,45 @@ static void arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn,
>       arm_smmu_iotlb_flush_all(d);
>   }
>
> +static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
> +								struct device *dev)

The indentation is wrong here.

[..]

> -	spin_lock(&xen_domain->lock);
> -	/* Chain the new context to the domain */
> -	list_add(&domain->list, &xen_domain->contexts);
> -	spin_unlock(&xen_domain->lock);
> +		/* Chain the new context to the domain */
> +		list_add(&domain->list, &xen_domain->contexts);
> +		

This line contains trailing white-space.

Other than this 2 nits:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall