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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 6/29/23 17:47, Julien Grall wrote:
> 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()?

It was switched originally in the downstream I cherry-picked the patch 
("xen/arm: Move is_protected flag to struct device") [1] from, where the 
rationale for the change wasn't explicitly written. Improving readability was 
the only rationale I could think of.

Anyway, I will drop this change for the next revision of this series. Hmm. The 
comment may still want to be expanded though... But I feel improving the 
comment alone should not be part of this series.

[1] 
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/59753aac77528a584d3950936b853ebf264b68e7#9e9e9f5f577b2b9fc19d92dcefe7580c7c3af744