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

[Julien Grall <julien@xxxxxxx>]

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?
If the others agree with the double check, then I think this should gain 
an ASSERT_UNREACHABLE() because AFAIU this is a programming error.
> +        return -EEXIST;
> +
>       /*
>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>        * from Linux.
Cheers,

--
Julien Grall