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_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()?
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
