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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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?

>> +        return -EEXIST;
>> +
>>       /*
>>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>>        * from Linux.
> 
> Cheers,
> 
> -- 
> Julien Grall