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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxx>

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel