[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi Stefano,

On 23/07/2021 21:16, Stefano Stabellini wrote:
On Fri, 23 Jul 2021, Julien Grall wrote:
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
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?

No, both need to be present; if there is just the generic bindings we
don't need this change. I can still move it to the beginning of the
series anyway if you prefer.

Sorry but I am having some trouble to understand why this is not a problem for just the legacy binding.

If I look at patch #1, the dev->iommu_fspec will be allocated in register_smmu_master(). If I am not mistaken, this is called when the SMMU is initialized.

So the call to iommu_add_dt_device() in handle_device() should return -EEXIST (dev_iommu_fwspec_get() will return a non-NULL pointer).

What did I miss?

+     */

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.

I see. Maybe:

   * In some circumstances iommu_add_dt_device() can genuinly be called
   * twice. As there is no harm in it just return success early.

Sound good to me.

       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
            * re-creating guest domain.

This comment on top is now stale.

I missed it somehow; yes definitely it should be removed. I can do it in
the next version of this patch.

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


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.