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

Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices



Hi,

Sorry for the late answer.

On 07/06/2023 14:41, Stewart Hildebrand wrote:
On 6/7/23 03:27, Julien Grall wrote:
Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

Improve readability of check for devices already registered with the SMMU with
legacy mmu-masters DT bindings by using is_protected.

I am unconvinced with this change because...


There are 2 device tree bindings for registering a device with the SMMU:
* mmu-masters (legacy, SMMUv1/2 only)
* iommus

A device tree may include both mmu-masters and iommus properties (although it is
unnecessary to do so). When a device appears in the mmu-masters list,
np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
function iommu_add_dt_device() is subsequently invoked for devices that have an
iommus specification.

The check as it was before this patch:

    if ( dev_iommu_fwspec_get(dev) )
        return 0;

and the new check:

    if ( dt_device_is_protected(np) )
        return 0;

are guarding against the same corner case: when a device has both mmu-masters
and iommus specifications in the device tree. The is_protected naming is more
descriptive.

If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
an error condition, so return an error in this case.

Expand the comment to further clarify the corner case.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
---
v3->v4:
* new patch: this change was split from ("xen/arm: Move is_protected flag to struct 
device")
---
   xen/drivers/passthrough/device_tree.c | 11 ++++++++---
   1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c 
b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50cce..d9b63da7260a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
           return -EINVAL;

       /*
-     * The device may already have been registered. As there is no harm in
-     * it just return success early.
+     * Devices that appear in the legacy mmu-masters list may have already been
+     * registered with the SMMU. In case a device has both a mmu-masters entry
+     * and iommus property, there is no need to register it again. In this case
+     * simply return success early.
        */
-    if ( dev_iommu_fwspec_get(dev) )
+    if ( dt_device_is_protected(np) )
... we now have two checks and it implies that it would be normal for
dt_device_is_protected() to be false and ...

           return 0;

+    if ( dev_iommu_fwspec_get(dev) )

... this returning a non-zero value. Is there any other benefits with
adding the two checks?

No, I can't think of any good rationale for the 2nd check. After splitting this change 
from the other patch ("xen/arm: Move is_protected flag to struct device"), I 
simply wanted to evaluate it on its own.

If the others agree with the double check, then I think this should gain
an ASSERT_UNREACHABLE() because AFAIU this is a programming error.

Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), 
good point. But I don't think the 2nd check is justified.

If the 2nd check is dropped (keeping only the is_protected check), then do you 
think the change is justified?

To be honest not with the current justification. I don't view the new check better than the other in term of readability.

Is this the only reason you want to switch to dt_device_is_protected()?

Cheers,

--
Julien Grall



 


Rackspace

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