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_tyshche...@epam.com>
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_tyshche...@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
---
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
