| Message ID | 20210722233642.22515-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Generic SMMU Bindings | expand |
On 23.07.2021 01:36, Stefano Stabellini wrote: > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) > if ( !ops ) > return -EINVAL; > > + /* > + * Some Device Trees may expose both legacy SMMU and generic > + * IOMMU bindings together. If both are present, the device > + * can be already added. > + */ > if ( dev_iommu_fwspec_get(dev) ) > - return -EEXIST; > + return 0; Since the xen: prefix in the subject made me go look (I wouldn't have if it had been e.g. dt: ), I may as well ask: Since previously there was concern about bogus duplicate entries, does this concern go away no altogether? It's one thing for there to be a legacy and a generic binding, but another if you found two legacy or two generic ones, I would think. And what if legacy and generic representation differ in some way? Shouldn't you limit processing to just one of the two categories, such that no legitimate "already present" case could be encountered here in the first place? Jan
Hi Stefano, On 23/07/2021 00:36, Stefano Stabellini wrote: > If both legacy IOMMU bindings and generic bindings are present, > iommu_add_dt_device can be called twice. Do not return error in that > case, that way there is no need to check for -EEXIST at the call sites. > Remove the one existing -EEXIT check, now unneeded. The commit message implies that we already support both legacy and generic bindings. However, this is not yet implemented. So how about: " iommu_add_dt_device() will returns -EEXIST if the device was already registered. At the moment, this can only happen if the device was already assigned to a domain (either dom0 at boot or via XEN_DOMCTL_assign_device). In a follow-up patch, we will convert the SMMU driver to use the FW spec. When the legacy bindings are used, all the devices will be registered at probe. Therefore, iommu_add_dt_device() will always returns -EEXIST. Currently, one caller (XEN_DOMCTL_assign_device) will check the return and ignore -EEXIST. All the other will fail because it was technically a programming error. However, there is no harm to call iommu_add_dt_device() twice, so we can simply return 0. With that in place the caller doesn't need to check -EEXIST anymore, so remove the check. " > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > Changes in v5: > - new patch > --- > xen/drivers/passthrough/device_tree.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index 999b831d90..32526ecabb 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) > if ( !ops ) > return -EINVAL; > > + /* > + * Some Device Trees may expose both legacy SMMU and generic > + * IOMMU bindings together. If both are present, the device > + * can be already added. Wouldn't this also happen when there is just generic bindings? If so, shouldn't this patch be first in the series to avoid breaking bisection? > + */ My point on the previous version is this is not the only reasons why dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT, there is only two) or we want to write a generic message. > if ( dev_iommu_fwspec_get(dev) ) > - return -EEXIST; > + return 0; > > /* > * According to the Documentation/devicetree/bindings/iommu/iommu.txt > @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > * already added to the IOMMU (positive result). Such happens after > * re-creating guest domain. > */ This comment on top is now stale. > - if ( ret < 0 && ret != -EEXIST ) > + if ( ret < 0 ) > { > printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n", > dt_node_full_name(dev)); > Cheers,
Hi Jan, On 23/07/2021 07:31, Jan Beulich wrote: > On 23.07.2021 01:36, Stefano Stabellini wrote: >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >> if ( !ops ) >> return -EINVAL; >> >> + /* >> + * Some Device Trees may expose both legacy SMMU and generic >> + * IOMMU bindings together. If both are present, the device >> + * can be already added. >> + */ >> if ( dev_iommu_fwspec_get(dev) ) >> - return -EEXIST; >> + return 0; > > Since the xen: prefix in the subject made me go look (I wouldn't have > if it had been e.g. dt: ), I may as well ask: Since previously there > was concern about bogus duplicate entries, does this concern go away > no altogether? The check wasn't originally added because of legacy vs generic binding. It was added because in some circumstances iommu_add_dt_device() could genuinely be called twice (for instance if the device is re-assigned). This was returning -EEXIST rather than 0 so the caller can decide whether it is normal that the device is already added. Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 (this patch should really be first), dev_iommu_fwspec_get() will return a non-NULL pointer as the legacy devices are added when the IOMMU is probed. > It's one thing for there to be a legacy and a generic > binding, but another if you found two legacy or two generic ones, I > would think. I am not quite too sure what you mean by "two legacy" and "two generic". Can you clarify it? > > And what if legacy and generic representation differ in some way? That would be a firmware table issue. It is not Xen business to check whether the two representation agree. > Shouldn't you limit processing to just one of the two categories, > such that no legitimate "already present" case could be encountered > here in the first place? There are legitimate "already present" case. This can happen when a device is re-assigned. Arguably the caller could check if the device was already added, however it would involve more code in each caller. So it is much easier to add in iommu_add_dt_device(). Cheers,
On 23.07.2021 11:28, Julien Grall wrote: > Hi Jan, > > On 23/07/2021 07:31, Jan Beulich wrote: >> On 23.07.2021 01:36, Stefano Stabellini wrote: >>> --- a/xen/drivers/passthrough/device_tree.c >>> +++ b/xen/drivers/passthrough/device_tree.c >>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>> if ( !ops ) >>> return -EINVAL; >>> >>> + /* >>> + * Some Device Trees may expose both legacy SMMU and generic >>> + * IOMMU bindings together. If both are present, the device >>> + * can be already added. >>> + */ >>> if ( dev_iommu_fwspec_get(dev) ) >>> - return -EEXIST; >>> + return 0; >> >> Since the xen: prefix in the subject made me go look (I wouldn't have >> if it had been e.g. dt: ), I may as well ask: Since previously there >> was concern about bogus duplicate entries, does this concern go away >> no altogether? > > The check wasn't originally added because of legacy vs generic binding. > > It was added because in some circumstances iommu_add_dt_device() could > genuinely be called twice (for instance if the device is re-assigned). > This was returning -EEXIST rather than 0 so the caller can decide > whether it is normal that the device is already added. Okay. If that distinction is of no interest anymore, then I can see this wanting dropping. > Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 > (this patch should really be first), dev_iommu_fwspec_get() will return > a non-NULL pointer as the legacy devices are added when the IOMMU is probed. > >> It's one thing for there to be a legacy and a generic >> binding, but another if you found two legacy or two generic ones, I >> would think. > > I am not quite too sure what you mean by "two legacy" and "two generic". > Can you clarify it? Well, I'm having trouble describing it in different terms. I mean two entries of the same kind (both legacy or both generic) referring to the same device, thus leading to the function recognizing the 2nd time round that the device is already there. Jan
On Fri, 23 Jul 2021, Julien Grall wrote: > Hi Stefano, > > On 23/07/2021 00:36, Stefano Stabellini wrote: > > If both legacy IOMMU bindings and generic bindings are present, > > iommu_add_dt_device can be called twice. Do not return error in that > > case, that way there is no need to check for -EEXIST at the call sites. > > Remove the one existing -EEXIT check, now unneeded. > > The commit message implies that we already support both legacy and generic > bindings. However, this is not yet implemented. > > So how about: > > " > iommu_add_dt_device() will returns -EEXIST if the device was already > registered. > > At the moment, this can only happen if the device was already assigned to a > domain (either dom0 at boot or via XEN_DOMCTL_assign_device). > > In a follow-up patch, we will convert the SMMU driver to use the FW spec. When > the legacy bindings are used, all the devices will be registered at probe. > Therefore, iommu_add_dt_device() will always returns -EEXIST. > > Currently, one caller (XEN_DOMCTL_assign_device) will check the return and > ignore -EEXIST. All the other will fail because it was technically a > programming error. > > However, there is no harm to call iommu_add_dt_device() twice, so we can > simply return 0. > > With that in place the caller doesn't need to check -EEXIST anymore, so remove > the check. > " This is a lot better, thank you! > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > --- > > Changes in v5: > > - new patch > > --- > > xen/drivers/passthrough/device_tree.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/xen/drivers/passthrough/device_tree.c > > b/xen/drivers/passthrough/device_tree.c > > index 999b831d90..32526ecabb 100644 > > --- a/xen/drivers/passthrough/device_tree.c > > +++ b/xen/drivers/passthrough/device_tree.c > > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) > > if ( !ops ) > > return -EINVAL; > > + /* > > + * Some Device Trees may expose both legacy SMMU and generic > > + * IOMMU bindings together. If both are present, the device > > + * can be already added. > > Wouldn't this also happen when there is just generic bindings? If so, > shouldn't this patch be first in the series to avoid breaking bisection? No, both need to be present; if there is just the generic bindings we don't need this change. I can still move it to the beginning of the series anyway if you prefer. > > + */ > > My point on the previous version is this is not the only reasons why > dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT, > there is only two) or we want to write a generic message. I see. Maybe: * In some circumstances iommu_add_dt_device() can genuinly be called * twice. As there is no harm in it just return success early. > > if ( dev_iommu_fwspec_get(dev) ) > > - return -EEXIST; > > + return 0; > > /* > > * According to the Documentation/devicetree/bindings/iommu/iommu.txt > > @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct > > domain *d, > > * already added to the IOMMU (positive result). Such happens > > after > > * re-creating guest domain. > > */ > > This comment on top is now stale. I missed it somehow; yes definitely it should be removed. I can do it in the next version of this patch. > > - if ( ret < 0 && ret != -EEXIST ) > > + if ( ret < 0 ) > > { > > printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n", > > dt_node_full_name(dev));
Hi Jan, On 23/07/2021 14:02, Jan Beulich wrote: > On 23.07.2021 11:28, Julien Grall wrote: >> Hi Jan, >> >> On 23/07/2021 07:31, Jan Beulich wrote: >>> On 23.07.2021 01:36, Stefano Stabellini wrote: >>>> --- a/xen/drivers/passthrough/device_tree.c >>>> +++ b/xen/drivers/passthrough/device_tree.c >>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>>> if ( !ops ) >>>> return -EINVAL; >>>> >>>> + /* >>>> + * Some Device Trees may expose both legacy SMMU and generic >>>> + * IOMMU bindings together. If both are present, the device >>>> + * can be already added. >>>> + */ >>>> if ( dev_iommu_fwspec_get(dev) ) >>>> - return -EEXIST; >>>> + return 0; >>> >>> Since the xen: prefix in the subject made me go look (I wouldn't have >>> if it had been e.g. dt: ), I may as well ask: Since previously there >>> was concern about bogus duplicate entries, does this concern go away >>> no altogether? >> >> The check wasn't originally added because of legacy vs generic binding. >> >> It was added because in some circumstances iommu_add_dt_device() could >> genuinely be called twice (for instance if the device is re-assigned). >> This was returning -EEXIST rather than 0 so the caller can decide >> whether it is normal that the device is already added. > > Okay. If that distinction is of no interest anymore, then I can see > this wanting dropping. > >> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 >> (this patch should really be first), dev_iommu_fwspec_get() will return >> a non-NULL pointer as the legacy devices are added when the IOMMU is probed. >> >>> It's one thing for there to be a legacy and a generic >>> binding, but another if you found two legacy or two generic ones, I >>> would think. >> >> I am not quite too sure what you mean by "two legacy" and "two generic". >> Can you clarify it? > > Well, I'm having trouble describing it in different terms. I mean > two entries of the same kind (both legacy or both generic) referring > to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there. I think you are misunderstanding the purpose of this function. It is called when we discover a new device rather than discovering a new entry in the IOMMU. The function will then sort out what to do for the device. The legacy binding is somewhat specific because it bypass the function as the discovering is done per IOMMU rather than per device. Cheers,
Hi Stefano, On 23/07/2021 21:16, Stefano Stabellini wrote: > On Fri, 23 Jul 2021, Julien Grall wrote: >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> --- >>> Changes in v5: >>> - new patch >>> --- >>> xen/drivers/passthrough/device_tree.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/drivers/passthrough/device_tree.c >>> b/xen/drivers/passthrough/device_tree.c >>> index 999b831d90..32526ecabb 100644 >>> --- a/xen/drivers/passthrough/device_tree.c >>> +++ b/xen/drivers/passthrough/device_tree.c >>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>> if ( !ops ) >>> return -EINVAL; >>> + /* >>> + * Some Device Trees may expose both legacy SMMU and generic >>> + * IOMMU bindings together. If both are present, the device >>> + * can be already added. >> >> Wouldn't this also happen when there is just generic bindings? If so, >> shouldn't this patch be first in the series to avoid breaking bisection? > > No, both need to be present; if there is just the generic bindings we > don't need this change. I can still move it to the beginning of the > series anyway if you prefer. Sorry but I am having some trouble to understand why this is not a problem for just the legacy binding. If I look at patch #1, the dev->iommu_fspec will be allocated in register_smmu_master(). If I am not mistaken, this is called when the SMMU is initialized. So the call to iommu_add_dt_device() in handle_device() should return -EEXIST (dev_iommu_fwspec_get() will return a non-NULL pointer). What did I miss? > > >>> + */ >> >> My point on the previous version is this is not the only reasons why >> dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT, >> there is only two) or we want to write a generic message. > > I see. Maybe: > > * In some circumstances iommu_add_dt_device() can genuinly be called > * twice. As there is no harm in it just return success early. Sound good to me. > > >>> if ( dev_iommu_fwspec_get(dev) ) >>> - return -EEXIST; >>> + return 0; >>> /* >>> * According to the Documentation/devicetree/bindings/iommu/iommu.txt >>> @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct >>> domain *d, >>> * already added to the IOMMU (positive result). Such happens >>> after >>> * re-creating guest domain. >>> */ >> >> This comment on top is now stale. > > I missed it somehow; yes definitely it should be removed. I can do it in > the next version of this patch. > > >>> - if ( ret < 0 && ret != -EEXIST ) >>> + if ( ret < 0 ) >>> { >>> printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n", >>> dt_node_full_name(dev)); Cheers,
On Mon, 26 Jul 2021, Julien Grall wrote: > Hi Stefano, > > On 23/07/2021 21:16, Stefano Stabellini wrote: > > On Fri, 23 Jul 2021, Julien Grall wrote: > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > --- > > > > Changes in v5: > > > > - new patch > > > > --- > > > > xen/drivers/passthrough/device_tree.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/xen/drivers/passthrough/device_tree.c > > > > b/xen/drivers/passthrough/device_tree.c > > > > index 999b831d90..32526ecabb 100644 > > > > --- a/xen/drivers/passthrough/device_tree.c > > > > +++ b/xen/drivers/passthrough/device_tree.c > > > > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) > > > > if ( !ops ) > > > > return -EINVAL; > > > > + /* > > > > + * Some Device Trees may expose both legacy SMMU and generic > > > > + * IOMMU bindings together. If both are present, the device > > > > + * can be already added. > > > > > > Wouldn't this also happen when there is just generic bindings? If so, > > > shouldn't this patch be first in the series to avoid breaking bisection? > > > > No, both need to be present; if there is just the generic bindings we > > don't need this change. I can still move it to the beginning of the > > series anyway if you prefer. > > Sorry but I am having some trouble to understand why this is not a problem for > just the legacy binding. > > If I look at patch #1, the dev->iommu_fspec will be allocated in > register_smmu_master(). If I am not mistaken, this is called when the SMMU is > initialized. > > So the call to iommu_add_dt_device() in handle_device() should return -EEXIST > (dev_iommu_fwspec_get() will return a non-NULL pointer). > > What did I miss? I checked. It is true that we need this check with the legacy bindings, even when alone. Like you said, dev->iommu_fspec is allocated early by register_smmu_master. Then, handle_device, or handle_passthrough_prop for dom0less guests, calls iommu_add_dt_device a second time. On the other hand with only the generic bindings register_smmu_master doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only once by handle_device or handle_passthrough_prop. The comment I proposed is not correct. What about this one? /* * In case of legacy SMMU bindings, register_smmu_master might have * already initialized struct iommu_fwspec for this device. */
Hi Stefano, On 30/07/2021 22:57, Stefano Stabellini wrote: > On Mon, 26 Jul 2021, Julien Grall wrote: >> Hi Stefano, >> >> On 23/07/2021 21:16, Stefano Stabellini wrote: >>> On Fri, 23 Jul 2021, Julien Grall wrote: >>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>>>> --- >>>>> Changes in v5: >>>>> - new patch >>>>> --- >>>>> xen/drivers/passthrough/device_tree.c | 9 +++++++-- >>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/drivers/passthrough/device_tree.c >>>>> b/xen/drivers/passthrough/device_tree.c >>>>> index 999b831d90..32526ecabb 100644 >>>>> --- a/xen/drivers/passthrough/device_tree.c >>>>> +++ b/xen/drivers/passthrough/device_tree.c >>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>>>> if ( !ops ) >>>>> return -EINVAL; >>>>> + /* >>>>> + * Some Device Trees may expose both legacy SMMU and generic >>>>> + * IOMMU bindings together. If both are present, the device >>>>> + * can be already added. >>>> >>>> Wouldn't this also happen when there is just generic bindings? If so, >>>> shouldn't this patch be first in the series to avoid breaking bisection? >>> >>> No, both need to be present; if there is just the generic bindings we >>> don't need this change. I can still move it to the beginning of the >>> series anyway if you prefer. >> >> Sorry but I am having some trouble to understand why this is not a problem for >> just the legacy binding. >> >> If I look at patch #1, the dev->iommu_fspec will be allocated in >> register_smmu_master(). If I am not mistaken, this is called when the SMMU is >> initialized. >> >> So the call to iommu_add_dt_device() in handle_device() should return -EEXIST >> (dev_iommu_fwspec_get() will return a non-NULL pointer). >> >> What did I miss? > > I checked. It is true that we need this check with the legacy bindings, > even when alone. > > Like you said, dev->iommu_fspec is allocated early by > register_smmu_master. Then, handle_device, or handle_passthrough_prop > for dom0less guests, calls iommu_add_dt_device a second time. > > On the other hand with only the generic bindings register_smmu_master > doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only > once by handle_device or handle_passthrough_prop. > > > The comment I proposed is not correct. What about this one? > > /* > * In case of legacy SMMU bindings, register_smmu_master might have > * already initialized struct iommu_fwspec for this device. > */ As I may have mentionned in a previous version of the series, this check is not specific to the SMMU. We also need it for other cases (e.g. when the device is (re-)assigned to a guest). So I think a specialized comment is unsuitable here. Instead, we should provide a generic comment. Something like: /* * The device may already have been registered. As there is no harm in * it just return success early. */ Cheers,
On Mon, 2 Aug 2021, Julien Grall wrote: > Hi Stefano, > > On 30/07/2021 22:57, Stefano Stabellini wrote: > > On Mon, 26 Jul 2021, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 23/07/2021 21:16, Stefano Stabellini wrote: > > > > On Fri, 23 Jul 2021, Julien Grall wrote: > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > > > --- > > > > > > Changes in v5: > > > > > > - new patch > > > > > > --- > > > > > > xen/drivers/passthrough/device_tree.c | 9 +++++++-- > > > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/xen/drivers/passthrough/device_tree.c > > > > > > b/xen/drivers/passthrough/device_tree.c > > > > > > index 999b831d90..32526ecabb 100644 > > > > > > --- a/xen/drivers/passthrough/device_tree.c > > > > > > +++ b/xen/drivers/passthrough/device_tree.c > > > > > > @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node > > > > > > *np) > > > > > > if ( !ops ) > > > > > > return -EINVAL; > > > > > > + /* > > > > > > + * Some Device Trees may expose both legacy SMMU and generic > > > > > > + * IOMMU bindings together. If both are present, the device > > > > > > + * can be already added. > > > > > > > > > > Wouldn't this also happen when there is just generic bindings? If so, > > > > > shouldn't this patch be first in the series to avoid breaking > > > > > bisection? > > > > > > > > No, both need to be present; if there is just the generic bindings we > > > > don't need this change. I can still move it to the beginning of the > > > > series anyway if you prefer. > > > > > > Sorry but I am having some trouble to understand why this is not a problem > > > for > > > just the legacy binding. > > > > > > If I look at patch #1, the dev->iommu_fspec will be allocated in > > > register_smmu_master(). If I am not mistaken, this is called when the SMMU > > > is > > > initialized. > > > > > > So the call to iommu_add_dt_device() in handle_device() should return > > > -EEXIST > > > (dev_iommu_fwspec_get() will return a non-NULL pointer). > > > > > > What did I miss? > > > > I checked. It is true that we need this check with the legacy bindings, > > even when alone. > > > > Like you said, dev->iommu_fspec is allocated early by > > register_smmu_master. Then, handle_device, or handle_passthrough_prop > > for dom0less guests, calls iommu_add_dt_device a second time. > > > > On the other hand with only the generic bindings register_smmu_master > > doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only > > once by handle_device or handle_passthrough_prop. > > > > > The comment I proposed is not correct. What about this one? > > > > /* > > * In case of legacy SMMU bindings, register_smmu_master might have > > * already initialized struct iommu_fwspec for this device. > > */ > As I may have mentionned in a previous version of the series, this check is > not specific to the SMMU. We also need it for other cases (e.g. when the > device is (re-)assigned to a guest). > > So I think a specialized comment is unsuitable here. Instead, we should > provide a generic comment. Something like: > > /* > * The device may already have been registered. As there is no harm in > * it just return success early. > */ Thanks, I'll use it in the next version
On 26.07.2021 17:45, Julien Grall wrote: > On 23/07/2021 14:02, Jan Beulich wrote: >> On 23.07.2021 11:28, Julien Grall wrote: >>> On 23/07/2021 07:31, Jan Beulich wrote: >>>> On 23.07.2021 01:36, Stefano Stabellini wrote: >>>>> --- a/xen/drivers/passthrough/device_tree.c >>>>> +++ b/xen/drivers/passthrough/device_tree.c >>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>>>> if ( !ops ) >>>>> return -EINVAL; >>>>> >>>>> + /* >>>>> + * Some Device Trees may expose both legacy SMMU and generic >>>>> + * IOMMU bindings together. If both are present, the device >>>>> + * can be already added. >>>>> + */ >>>>> if ( dev_iommu_fwspec_get(dev) ) >>>>> - return -EEXIST; >>>>> + return 0; >>>> >>>> Since the xen: prefix in the subject made me go look (I wouldn't have >>>> if it had been e.g. dt: ), I may as well ask: Since previously there >>>> was concern about bogus duplicate entries, does this concern go away >>>> no altogether? >>> >>> The check wasn't originally added because of legacy vs generic binding. >>> >>> It was added because in some circumstances iommu_add_dt_device() could >>> genuinely be called twice (for instance if the device is re-assigned). >>> This was returning -EEXIST rather than 0 so the caller can decide >>> whether it is normal that the device is already added. >> >> Okay. If that distinction is of no interest anymore, then I can see >> this wanting dropping. >> >>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 >>> (this patch should really be first), dev_iommu_fwspec_get() will return >>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed. >>> >>>> It's one thing for there to be a legacy and a generic >>>> binding, but another if you found two legacy or two generic ones, I >>>> would think. >>> >>> I am not quite too sure what you mean by "two legacy" and "two generic". >>> Can you clarify it? >> >> Well, I'm having trouble describing it in different terms. I mean >> two entries of the same kind (both legacy or both generic) referring >> to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there. > > I think you are misunderstanding the purpose of this function. It is > called when we discover a new device rather than discovering a new entry > in the IOMMU. The function will then sort out what to do for the device. I'm struggling with assigning meaning to "discovering a new entry in the IOMMU". Otoh to "discover a new device" means the device wasn't (supposed to be) known before, which to me means -EEXIST is appropriate. > The legacy binding is somewhat specific because it bypass the function > as the discovering is done per IOMMU rather than per device. Well, I then guess I'm lacking too much context here. Jan
Hi Jan, On 03/08/2021 07:57, Jan Beulich wrote: > On 26.07.2021 17:45, Julien Grall wrote: >> On 23/07/2021 14:02, Jan Beulich wrote: >>> On 23.07.2021 11:28, Julien Grall wrote: >>>> On 23/07/2021 07:31, Jan Beulich wrote: >>>>> On 23.07.2021 01:36, Stefano Stabellini wrote: >>>>>> --- a/xen/drivers/passthrough/device_tree.c >>>>>> +++ b/xen/drivers/passthrough/device_tree.c >>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) >>>>>> if ( !ops ) >>>>>> return -EINVAL; >>>>>> >>>>>> + /* >>>>>> + * Some Device Trees may expose both legacy SMMU and generic >>>>>> + * IOMMU bindings together. If both are present, the device >>>>>> + * can be already added. >>>>>> + */ >>>>>> if ( dev_iommu_fwspec_get(dev) ) >>>>>> - return -EEXIST; >>>>>> + return 0; >>>>> >>>>> Since the xen: prefix in the subject made me go look (I wouldn't have >>>>> if it had been e.g. dt: ), I may as well ask: Since previously there >>>>> was concern about bogus duplicate entries, does this concern go away >>>>> no altogether? >>>> >>>> The check wasn't originally added because of legacy vs generic binding. >>>> >>>> It was added because in some circumstances iommu_add_dt_device() could >>>> genuinely be called twice (for instance if the device is re-assigned). >>>> This was returning -EEXIST rather than 0 so the caller can decide >>>> whether it is normal that the device is already added. >>> >>> Okay. If that distinction is of no interest anymore, then I can see >>> this wanting dropping. >>> >>>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1 >>>> (this patch should really be first), dev_iommu_fwspec_get() will return >>>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed. >>>> >>>>> It's one thing for there to be a legacy and a generic >>>>> binding, but another if you found two legacy or two generic ones, I >>>>> would think. >>>> >>>> I am not quite too sure what you mean by "two legacy" and "two generic". >>>> Can you clarify it? >>> >>> Well, I'm having trouble describing it in different terms. I mean >>> two entries of the same kind (both legacy or both generic) referring >>> to the same device, thus leading to the function recognizing the 2nd > time round that the device is already there. >> >> I think you are misunderstanding the purpose of this function. It is >> called when we discover a new device rather than discovering a new entry >> in the IOMMU. The function will then sort out what to do for the device. > > I'm struggling with assigning meaning to "discovering a new entry in the > IOMMU". I meant in the IOMMU firmware table, sorry. IOW, when a new IOMMU is added we walk its configuration to figure out which device is attached to it. > Otoh to "discover a new device" means the device wasn't (supposed > to be) known before, which to me means -EEXIST is appropriate. Right. The problem is after patch #1 all callers would need to cope with -EEXIST because the legacy binding register the device up front. That's why I think returning 0 here is better. Cheers,
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 999b831d90..32526ecabb 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np) if ( !ops ) return -EINVAL; + /* + * Some Device Trees may expose both legacy SMMU and generic + * IOMMU bindings together. If both are present, the device + * can be already added. + */ if ( dev_iommu_fwspec_get(dev) ) - return -EEXIST; + return 0; /* * According to the Documentation/devicetree/bindings/iommu/iommu.txt @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, * already added to the IOMMU (positive result). Such happens after * re-creating guest domain. */ - if ( ret < 0 && ret != -EEXIST ) + if ( ret < 0 ) { printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n", dt_node_full_name(dev));
If both legacy IOMMU bindings and generic bindings are present, iommu_add_dt_device can be called twice. Do not return error in that case, that way there is no need to check for -EEXIST at the call sites. Remove the one existing -EEXIT check, now unneeded. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- Changes in v5: - new patch --- xen/drivers/passthrough/device_tree.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)