Re: [XEN][RFC PATCH v2 07/12] xen/smmu: Add remove_device callback for smmu_iommu ops

[Julien Grall <julien@xxxxxxx>]

Hi,

On 09/11/2021 08:02, Vikram Garhwal wrote:
> Add remove_device callback for removing the device entry from smmu-master using
>   following steps:
>      1. Find if SMMU master exists for the device node.
>      2. Remove the SMMU master
The commit title and message doesn't match the code.

> Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> ---
>   xen/drivers/passthrough/device_tree.c | 30 ++++++++++++++++++++++++++++++
>   xen/include/xen/iommu.h               |  2 ++
>   2 files changed, 32 insertions(+)
>
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0..9d9eed8 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,6 +127,36 @@ int iommu_release_dt_devices(struct domain *d)
>       return 0;
>   }
>   
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    struct device *dev = dt_to_dev(np);
> +    int rc;
> +
> +    if ( !ops )
> +        return -EOPNOTSUPP;
> +
> +    if ( iommu_dt_device_is_assigned(np) )
> +        return -EPERM;
EPERM means the caller doesn't have the permission to request it. 
However, dom0 will have the permission to remote the device. The problem 
is the device is currently assigned to a domain. So it would be better 
to return EBUSY.
Also, most of the function wants to be protected with dtdevs_lock to 
prevent concurrent access to add/remove/assign/deassign.
I would create a version of iommu_dt_device_is_assigned() (maybe called 
iommu_dt_device_is_assigned_locked()) that would do the same but with 
the caller hold the lock.
> +
> +    /*
> +     * The driver which supports generic IOMMU DT bindings must have
> +     * these callback implemented.
> +     */
> +    if ( !ops->remove_device )
> +        return -EOPNOTSUPP;
> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.
> +     */
> +    rc = ops->remove_device(0, dev);
> +
> +    if ( rc == 0 )
> +        iommu_fwspec_free(dev);
> +
> +    return rc;
> +}
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 6b2cdff..c4d5d12 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -215,6 +215,8 @@ int iommu_release_dt_devices(struct domain *d);
>    */
>   int iommu_add_dt_device(struct dt_device_node *np);
>   
> +int iommu_remove_dt_device(struct dt_device_node *np);
> +
>   int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>   
Cheers,

--
Julien Grall