[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 30/07/2021 22:57, Stefano Stabellini wrote:
On Mon, 26 Jul 2021, Julien Grall wrote:
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
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?

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?

I checked. It is true that we need this check with the legacy bindings,
even when alone.

Like you said, dev->iommu_fspec is allocated early by
register_smmu_master. Then, handle_device, or handle_passthrough_prop
for dom0less guests, calls iommu_add_dt_device a second time.

On the other hand with only the generic bindings register_smmu_master
doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only
once by handle_device or handle_passthrough_prop. >

The comment I proposed is not correct. What about this one?

     /*
      * In case of legacy SMMU bindings, register_smmu_master might have
      * already initialized struct iommu_fwspec for this device.
      */
As I may have mentionned in a previous version of the series, this check is not specific to the SMMU. We also need it for other cases (e.g. when the device is (re-)assigned to a guest).

So I think a specialized comment is unsuitable here. Instead, we should provide a generic comment. Something like:

/*
 * The device may already have been registered. As there is no harm in
 * it just return success early.
 */

Cheers,

--
Julien Grall



 


Rackspace

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