Re: [XEN][PATCH v10 11/20] xen/iommu: Introduce iommu_remove_dt_device()

[Michal Orzel <michal.orzel@amd.com>]

On 25/08/2023 10:02, 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@amd.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

You don't seem to handle Julien remarks for this patch made in v9.
I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch
if you haven't yet addressed someone's remarks.

> 
> ---
> Changes from v7:
>     Add check if IOMMU is enabled.
>     Fix indentation of fail.
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++
>  xen/include/xen/iommu.h               |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1202eac625..3fad65fb69 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -128,6 +128,50 @@ 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 ( !iommu_enabled )
> +        return 1;
J:
The caller doesn't seem to check if the error code is > 0. So can we 
instead return a -ERRNO?

If you want to continue to return a value > 0 then I think it should be 
documented in a comment like we did for iommu_add_dt_device().

> +
> +    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.
> +     */
J:
I have questioned this message in v7 and I still question it. I guess 
you copied the comment on top of add_device(), this was add there 
because we have a different way to add legacy device.

But here there are no such requirement. In fact, you are not adding the 
the callback to all the IOMMU drivers... Yet all of them support the 
generic IOMMU DT bindings.

> +    if ( !ops->remove_device )
> +    {
> +        rc = -EOPNOTSUPP;
> +        goto fail;
> +    }
> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.
J:
I read this as this will not return an error if the device is protected. 
However, AFAICT, the implement in the SMMU driver provided in this 
series will return an error. So I would suggest to replace this sentence 
with:

de-register the device from the IOMMU driver.

> +     * The driver is responsible for removing is_protected flag.
J:
Can you add an assert in the 'if ( !rc )' block to confirm that 
is_protected was effectively removed. Something like:

ASSERT(!dt_device_is_protected(dev));

This would help to confirm the driver is respecting what you expect.

> +     */
> +    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 110693c59f..a8e9bc9a2d 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np);
>  
>  int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
> +int iommu_remove_dt_device(struct dt_device_node *np);
>  
>  #endif /* HAS_DEVICE_TREE */
>  

~Michal