Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 23/07/2021 00:36, Stefano Stabellini wrote:
> If both legacy IOMMU bindings and generic bindings are present,
> iommu_add_dt_device can be called twice. Do not return error in that
> case, that way there is no need to check for -EEXIST at the call sites.
> Remove the one existing -EEXIT check, now unneeded.

The commit message implies that we already support both legacy and 
generic bindings. However, this is not yet implemented.

So how about:

"
iommu_add_dt_device() will returns -EEXIST if the device was already 
registered.

At the moment, this can only happen if the device was already assigned 
to a domain (either dom0 at boot or via XEN_DOMCTL_assign_device).

In a follow-up patch, we will convert the SMMU driver to use the FW 
spec. When the legacy bindings are used, all the devices will be 
registered at probe. Therefore, iommu_add_dt_device() will always 
returns -EEXIST.

Currently, one caller (XEN_DOMCTL_assign_device) will check the return 
and ignore -EEXIST. All the other will fail because it was technically a 
programming error.

However, there is no harm to call iommu_add_dt_device() twice, so we can 
simply return 0.

With that in place the caller doesn't need to check -EEXIST anymore, so 
remove the check.
"

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v5:
> - new patch
> ---
>   xen/drivers/passthrough/device_tree.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 999b831d90..32526ecabb 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>       if ( !ops )
>           return -EINVAL;
>   
> +    /*
> +     * Some Device Trees may expose both legacy SMMU and generic
> +     * IOMMU bindings together. If both are present, the device
> +     * can be already added.

Wouldn't this also happen when there is just generic bindings? If so, 
shouldn't this patch be first in the series to avoid breaking bisection?

> +     */

My point on the previous version is this is not the only reasons why 
dev_iommu_fwspec_get(). So either we want to write all the reasons 
(AFAICT, there is only two) or we want to write a generic message.

>       if ( dev_iommu_fwspec_get(dev) )
> -        return -EEXIST;
> +        return 0;
>   
>       /*
>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
> @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>            * already added to the IOMMU (positive result). Such happens after
>            * re-creating guest domain.
>            */

This comment on top is now stale.

> -        if ( ret < 0 && ret != -EEXIST )
> +        if ( ret < 0 )
>           {
>               printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
>                      dt_node_full_name(dev));
> 

Cheers,

-- 
Julien Grall