| Message ID | 20230607030220.22698-2-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi Stewart, On 07/06/2023 04:02, Stewart Hildebrand wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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_tyshchenko@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@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? 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,
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@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_tyshchenko@epam.com> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@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? >> + return -EEXIST; >> + >> /* >> * According to the Documentation/devicetree/bindings/iommu/iommu.txt >> * from Linux. > > Cheers, > > -- > Julien Grall
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@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_tyshchenko@epam.com> >>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@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,
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@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_tyshchenko@epam.com> >>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@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
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) ) return 0; + if ( dev_iommu_fwspec_get(dev) ) + return -EEXIST; + /* * According to the Documentation/devicetree/bindings/iommu/iommu.txt * from Linux.