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

[Julien Grall <julien@xxxxxxx>]

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