[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 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



 


Rackspace

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