Re: [XEN][PATCH v7 11/19] xen/iommu: Introduce iommu_remove_dt_device()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/06/2023 01:48, Vikram Garhwal wrote:
> Remove master device from the IOMMU. This will be helpful when removing the
> overlay nodes using dynamic programming during run time.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>   xen/drivers/passthrough/device_tree.c | 41 +++++++++++++++++++++++++++
>   xen/include/xen/iommu.h               |  2 ++
>   2 files changed, 43 insertions(+)
>
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 8cc413f867..301a5bcd97 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -126,6 +126,47 @@ 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;
> +
iommu_add_dt_device() checks if the IOMMU is enabled. I think you should 
do the same here as well and return 0 if it is disabled.
> +    if ( !ops )
> +        return -EOPNOTSUPP;
> +
> +    spin_lock(&dtdevs_lock);
> +
> +    if ( iommu_dt_device_is_assigned_locked(np) )
> +    {
> +        rc = -EBUSY;
> +        goto fail;
> +    }
> +
> +    /*
> +     * The driver which supports generic IOMMU DT bindings must have this
> +     * callback implemented.
> +     */
It is not clear to me why you want to mandate remove_device when using 
the generic IOMMU DT bindings.
But if this is really necessary, then I think the comment should be 
placed on top of the callback definition rather than in the caller.
> +    if ( !ops->remove_device )
> +    {
> +        rc = -EOPNOTSUPP;
> +        goto fail;
> +    }
> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.
> +     * The driver is responsible for removing is_protected flag.
> +     */
> +    rc = ops->remove_device(0, dev);
> +
> +    if ( !rc )
> +        iommu_fwspec_free(dev);
> +
> +fail:
> +    spin_unlock(&dtdevs_lock);
> +    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 405db59971..0d7924821b 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -229,6 +229,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