| Message ID | 20230825080222.14247-12-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 25/08/2023 10:02, Vikram Garhwal wrote: > Remove master device from the IOMMU. This will be helpful when removing the > overlay nodes using dynamic programming during run time. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > Acked-by: Jan Beulich <jbeulich@suse.com> You don't seem to handle Julien remarks for this patch made in v9. I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch if you haven't yet addressed someone's remarks. > > --- > Changes from v7: > Add check if IOMMU is enabled. > Fix indentation of fail. > --- > --- > xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ > xen/include/xen/iommu.h | 1 + > 2 files changed, 45 insertions(+) > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index 1202eac625..3fad65fb69 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) > return 0; > } > > +int iommu_remove_dt_device(struct dt_device_node *np) > +{ > + const struct iommu_ops *ops = iommu_get_ops(); > + struct device *dev = dt_to_dev(np); > + int rc; > + > + if ( !iommu_enabled ) > + return 1; J: The caller doesn't seem to check if the error code is > 0. So can we instead return a -ERRNO? If you want to continue to return a value > 0 then I think it should be documented in a comment like we did for iommu_add_dt_device(). > + > + if ( !ops ) > + return -EOPNOTSUPP; > + > + spin_lock(&dtdevs_lock); > + > + if ( iommu_dt_device_is_assigned_locked(np) ) > + { > + rc = -EBUSY; > + goto fail; > + } > + > + /* > + * The driver which supports generic IOMMU DT bindings must have this > + * callback implemented. > + */ J: I have questioned this message in v7 and I still question it. I guess you copied the comment on top of add_device(), this was add there because we have a different way to add legacy device. But here there are no such requirement. In fact, you are not adding the the callback to all the IOMMU drivers... Yet all of them support the generic IOMMU DT bindings. > + if ( !ops->remove_device ) > + { > + rc = -EOPNOTSUPP; > + goto fail; > + } > + > + /* > + * Remove master device from the IOMMU if latter is present and available. J: I read this as this will not return an error if the device is protected. However, AFAICT, the implement in the SMMU driver provided in this series will return an error. So I would suggest to replace this sentence with: de-register the device from the IOMMU driver. > + * The driver is responsible for removing is_protected flag. J: Can you add an assert in the 'if ( !rc )' block to confirm that is_protected was effectively removed. Something like: ASSERT(!dt_device_is_protected(dev)); This would help to confirm the driver is respecting what you expect. > + */ > + rc = ops->remove_device(0, dev); > + > + if ( !rc ) > + iommu_fwspec_free(dev); > + > + fail: > + spin_unlock(&dtdevs_lock); > + return rc; > +} > + > int iommu_add_dt_device(struct dt_device_node *np) > { > const struct iommu_ops *ops = iommu_get_ops(); > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 110693c59f..a8e9bc9a2d 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np); > > int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); > +int iommu_remove_dt_device(struct dt_device_node *np); > > #endif /* HAS_DEVICE_TREE */ > ~Michal
Hi Michal, On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote: > > > On 25/08/2023 10:02, Vikram Garhwal wrote: > > Remove master device from the IOMMU. This will be helpful when removing the > > overlay nodes using dynamic programming during run time. > > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> > > You don't seem to handle Julien remarks for this patch made in v9. > I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch > if you haven't yet addressed someone's remarks. This got skipped as I cannot find direct email from Julien. The only email reply on this patch is can find is from: xen-devel-bounces@lists.xenproject.org and this got messed up with other larger set of email xen-devel sends. Did you get direct email? > > > > > --- > > Changes from v7: > > Add check if IOMMU is enabled. > > Fix indentation of fail. > > --- > > --- > > xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ > > xen/include/xen/iommu.h | 1 + > > 2 files changed, 45 insertions(+) > > > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > > index 1202eac625..3fad65fb69 100644 > > --- a/xen/drivers/passthrough/device_tree.c > > +++ b/xen/drivers/passthrough/device_tree.c > > @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) > > return 0; > > } > > > > +int iommu_remove_dt_device(struct dt_device_node *np) > > +{ > > + const struct iommu_ops *ops = iommu_get_ops(); > > + struct device *dev = dt_to_dev(np); > > + int rc; > > + > > + if ( !iommu_enabled ) > > + return 1; > J: > The caller doesn't seem to check if the error code is > 0. So can we > instead return a -ERRNO? Will change the check in caller. I want to keep this as it as so it looks similar to iommu_add_dt_device(). > > If you want to continue to return a value > 0 then I think it should be > documented in a comment like we did for iommu_add_dt_device(). > Will add comment before iommu_remove_dt_device(). > > + > > + if ( !ops ) > > + return -EOPNOTSUPP; > > + > > + spin_lock(&dtdevs_lock); > > + > > + if ( iommu_dt_device_is_assigned_locked(np) ) > > + { > > + rc = -EBUSY; > > + goto fail; > > + } > > + > > + /* > > + * The driver which supports generic IOMMU DT bindings must have this > > + * callback implemented. > > + */ > J: > I have questioned this message in v7 and I still question it. I guess > you copied the comment on top of add_device(), this was add there > because we have a different way to add legacy device. > > But here there are no such requirement. In fact, you are not adding the > the callback to all the IOMMU drivers... Yet all of them support the > generic IOMMU DT bindings. Will change this. > > > + if ( !ops->remove_device ) > > + { > > + rc = -EOPNOTSUPP; > > + goto fail; > > + } > > + > > + /* > > + * Remove master device from the IOMMU if latter is present and available. > J: > I read this as this will not return an error if the device is protected. > However, AFAICT, the implement in the SMMU driver provided in this > series will return an error. So I would suggest to replace this sentence > with: > > de-register the device from the IOMMU driver. Will change the comment. > > > + * The driver is responsible for removing is_protected flag. > J: > Can you add an assert in the 'if ( !rc )' block to confirm that > is_protected was effectively removed. Something like: > > ASSERT(!dt_device_is_protected(dev)); Is ASSERT really required here. remove callback can return before setting is_protected as false. > > This would help to confirm the driver is respecting what you expect. > > > + */ > > + rc = ops->remove_device(0, dev); > > + > > + if ( !rc ) > > + iommu_fwspec_free(dev); > > + > > + fail: > > + spin_unlock(&dtdevs_lock); > > + return rc; > > +} > > + > > int iommu_add_dt_device(struct dt_device_node *np) > > { > > const struct iommu_ops *ops = iommu_get_ops(); > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > > index 110693c59f..a8e9bc9a2d 100644 > > --- a/xen/include/xen/iommu.h > > +++ b/xen/include/xen/iommu.h > > @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np); > > > > int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); > > +int iommu_remove_dt_device(struct dt_device_node *np); > > > > #endif /* HAS_DEVICE_TREE */ > > > > ~Michal
On Wed, 30 Aug 2023, Vikram Garhwal wrote: > Hi Michal, > On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote: > > > > > > On 25/08/2023 10:02, Vikram Garhwal wrote: > > > Remove master device from the IOMMU. This will be helpful when removing the > > > overlay nodes using dynamic programming during run time. > > > > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > > Acked-by: Jan Beulich <jbeulich@suse.com> > > > > You don't seem to handle Julien remarks for this patch made in v9. > > I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch > > if you haven't yet addressed someone's remarks. > This got skipped as I cannot find direct email from Julien. The only email reply > on this patch is can find is from: xen-devel-bounces@lists.xenproject.org and > this got messed up with other larger set of email xen-devel sends. > > Did you get direct email? > > > > > > > > --- > > > Changes from v7: > > > Add check if IOMMU is enabled. > > > Fix indentation of fail. > > > --- > > > --- > > > xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ > > > xen/include/xen/iommu.h | 1 + > > > 2 files changed, 45 insertions(+) > > > > > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > > > index 1202eac625..3fad65fb69 100644 > > > --- a/xen/drivers/passthrough/device_tree.c > > > +++ b/xen/drivers/passthrough/device_tree.c > > > @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) > > > return 0; > > > } > > > > > > +int iommu_remove_dt_device(struct dt_device_node *np) > > > +{ > > > + const struct iommu_ops *ops = iommu_get_ops(); > > > + struct device *dev = dt_to_dev(np); > > > + int rc; > > > + > > > + if ( !iommu_enabled ) > > > + return 1; > > J: > > The caller doesn't seem to check if the error code is > 0. So can we > > instead return a -ERRNO? > Will change the check in caller. I want to keep this as it as so it looks > similar to iommu_add_dt_device(). That is OK to me as long as the check on the return value is done > > If you want to continue to return a value > 0 then I think it should be > > documented in a comment like we did for iommu_add_dt_device(). > > > Will add comment before iommu_remove_dt_device(). > > > + > > > + if ( !ops ) > > > + return -EOPNOTSUPP; > > > + > > > + spin_lock(&dtdevs_lock); > > > + > > > + if ( iommu_dt_device_is_assigned_locked(np) ) > > > + { > > > + rc = -EBUSY; > > > + goto fail; > > > + } > > > + > > > + /* > > > + * The driver which supports generic IOMMU DT bindings must have this > > > + * callback implemented. > > > + */ > > J: > > I have questioned this message in v7 and I still question it. I guess > > you copied the comment on top of add_device(), this was add there > > because we have a different way to add legacy device. > > > > But here there are no such requirement. In fact, you are not adding the > > the callback to all the IOMMU drivers... Yet all of them support the > > generic IOMMU DT bindings. > Will change this. > > > > > + if ( !ops->remove_device ) > > > + { > > > + rc = -EOPNOTSUPP; > > > + goto fail; > > > + } > > > + > > > + /* > > > + * Remove master device from the IOMMU if latter is present and available. > > J: > > I read this as this will not return an error if the device is protected. > > However, AFAICT, the implement in the SMMU driver provided in this > > series will return an error. So I would suggest to replace this sentence > > with: > > > > de-register the device from the IOMMU driver. > Will change the comment. > > > > > + * The driver is responsible for removing is_protected flag. > > J: > > Can you add an assert in the 'if ( !rc )' block to confirm that > > is_protected was effectively removed. Something like: > > > > ASSERT(!dt_device_is_protected(dev)); > Is ASSERT really required here. remove callback can return before setting is_protected as false. But if ops->remove_device didn't actually set is_protected to false, then it should return an error (rc != 0). What Julien is suggesting is the following: rc = ops->remove_device(0, dev); if ( !rc ) { ASSERT(!dt_device_is_protected(dev)); iommu_fwspec_free(dev); } Every time remove_device returns rc == 0 then is_protected should be false, right? > > > > This would help to confirm the driver is respecting what you expect. > > > > > + */ > > > + rc = ops->remove_device(0, dev); > > > + > > > + if ( !rc ) > > > + iommu_fwspec_free(dev); > > > + > > > + fail: > > > + spin_unlock(&dtdevs_lock); > > > + return rc; > > > +} > > > + > > > int iommu_add_dt_device(struct dt_device_node *np) > > > { > > > const struct iommu_ops *ops = iommu_get_ops(); > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > > > index 110693c59f..a8e9bc9a2d 100644 > > > --- a/xen/include/xen/iommu.h > > > +++ b/xen/include/xen/iommu.h > > > @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np); > > > > > > int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, > > > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); > > > +int iommu_remove_dt_device(struct dt_device_node *np); > > > > > > #endif /* HAS_DEVICE_TREE */ > > > > > > > ~Michal >
On 30/08/2023 19:48, Vikram Garhwal wrote: > Hi Michal, > On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote: >> >> >> On 25/08/2023 10:02, Vikram Garhwal wrote: >>> Remove master device from the IOMMU. This will be helpful when removing the >>> overlay nodes using dynamic programming during run time. >>> >>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >>> Acked-by: Jan Beulich <jbeulich@suse.com> >> >> You don't seem to handle Julien remarks for this patch made in v9. >> I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch >> if you haven't yet addressed someone's remarks. > This got skipped as I cannot find direct email from Julien. The only email reply > on this patch is can find is from: xen-devel-bounces@lists.xenproject.org and > this got messed up with other larger set of email xen-devel sends. > > Did you get direct email? >> >>> >>> --- >>> Changes from v7: >>> Add check if IOMMU is enabled. >>> Fix indentation of fail. >>> --- >>> --- >>> xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ >>> xen/include/xen/iommu.h | 1 + >>> 2 files changed, 45 insertions(+) >>> >>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >>> index 1202eac625..3fad65fb69 100644 >>> --- a/xen/drivers/passthrough/device_tree.c >>> +++ b/xen/drivers/passthrough/device_tree.c >>> @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) >>> return 0; >>> } >>> >>> +int iommu_remove_dt_device(struct dt_device_node *np) >>> +{ >>> + const struct iommu_ops *ops = iommu_get_ops(); >>> + struct device *dev = dt_to_dev(np); >>> + int rc; >>> + >>> + if ( !iommu_enabled ) >>> + return 1; >> J: >> The caller doesn't seem to check if the error code is > 0. So can we >> instead return a -ERRNO? > Will change the check in caller. I want to keep this as it as so it looks > similar to iommu_add_dt_device(). >> >> If you want to continue to return a value > 0 then I think it should be >> documented in a comment like we did for iommu_add_dt_device(). >> > Will add comment before iommu_remove_dt_device(). >>> + >>> + if ( !ops ) >>> + return -EOPNOTSUPP; >>> + >>> + spin_lock(&dtdevs_lock); >>> + >>> + if ( iommu_dt_device_is_assigned_locked(np) ) >>> + { >>> + rc = -EBUSY; >>> + goto fail; >>> + } >>> + >>> + /* >>> + * The driver which supports generic IOMMU DT bindings must have this >>> + * callback implemented. >>> + */ >> J: >> I have questioned this message in v7 and I still question it. I guess >> you copied the comment on top of add_device(), this was add there >> because we have a different way to add legacy device. >> >> But here there are no such requirement. In fact, you are not adding the >> the callback to all the IOMMU drivers... Yet all of them support the >> generic IOMMU DT bindings. > Will change this. >> >>> + if ( !ops->remove_device ) >>> + { >>> + rc = -EOPNOTSUPP; >>> + goto fail; >>> + } >>> + >>> + /* >>> + * Remove master device from the IOMMU if latter is present and available. >> J: >> I read this as this will not return an error if the device is protected. >> However, AFAICT, the implement in the SMMU driver provided in this >> series will return an error. So I would suggest to replace this sentence >> with: >> >> de-register the device from the IOMMU driver. > Will change the comment. >> >>> + * The driver is responsible for removing is_protected flag. >> J: >> Can you add an assert in the 'if ( !rc )' block to confirm that >> is_protected was effectively removed. Something like: >> >> ASSERT(!dt_device_is_protected(dev)); > Is ASSERT really required here. remove callback can return before setting is_protected as false. I think Julien wanted to add extra check to make sure driver behaves as expected. That said, his suggestion is incorrect since the callback can return before clearing the flag. So, if ASSERT is required, this should be: ASSERT(rc || !dt_device_is_protected(dev)); so that we check for is_protected being false only on callback returning success (i.e. 0). >> >> This would help to confirm the driver is respecting what you expect. >> >>> + */ >>> + rc = ops->remove_device(0, dev); >>> + >>> + if ( !rc ) >>> + iommu_fwspec_free(dev); >>> + >>> + fail: >>> + spin_unlock(&dtdevs_lock); >>> + return rc; >>> +} >>> + >>> int iommu_add_dt_device(struct dt_device_node *np) >>> { >>> const struct iommu_ops *ops = iommu_get_ops(); >>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>> index 110693c59f..a8e9bc9a2d 100644 >>> --- a/xen/include/xen/iommu.h >>> +++ b/xen/include/xen/iommu.h >>> @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np); >>> >>> int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, >>> XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); >>> +int iommu_remove_dt_device(struct dt_device_node *np); >>> >>> #endif /* HAS_DEVICE_TREE */ >>> >> >> ~Michal
On 31/08/2023 09:23, Michal Orzel wrote: > > > On 30/08/2023 19:48, Vikram Garhwal wrote: >> Hi Michal, >> On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote: >>> >>> >>> On 25/08/2023 10:02, Vikram Garhwal wrote: >>>> Remove master device from the IOMMU. This will be helpful when removing the >>>> overlay nodes using dynamic programming during run time. >>>> >>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>> >>> You don't seem to handle Julien remarks for this patch made in v9. >>> I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch >>> if you haven't yet addressed someone's remarks. >> This got skipped as I cannot find direct email from Julien. The only email reply >> on this patch is can find is from: xen-devel-bounces@lists.xenproject.org and >> this got messed up with other larger set of email xen-devel sends. >> >> Did you get direct email? >>> >>>> >>>> --- >>>> Changes from v7: >>>> Add check if IOMMU is enabled. >>>> Fix indentation of fail. >>>> --- >>>> --- >>>> xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ >>>> xen/include/xen/iommu.h | 1 + >>>> 2 files changed, 45 insertions(+) >>>> >>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >>>> index 1202eac625..3fad65fb69 100644 >>>> --- a/xen/drivers/passthrough/device_tree.c >>>> +++ b/xen/drivers/passthrough/device_tree.c >>>> @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) >>>> return 0; >>>> } >>>> >>>> +int iommu_remove_dt_device(struct dt_device_node *np) >>>> +{ >>>> + const struct iommu_ops *ops = iommu_get_ops(); >>>> + struct device *dev = dt_to_dev(np); >>>> + int rc; >>>> + >>>> + if ( !iommu_enabled ) >>>> + return 1; >>> J: >>> The caller doesn't seem to check if the error code is > 0. So can we >>> instead return a -ERRNO? >> Will change the check in caller. I want to keep this as it as so it looks >> similar to iommu_add_dt_device(). >>> >>> If you want to continue to return a value > 0 then I think it should be >>> documented in a comment like we did for iommu_add_dt_device(). >>> >> Will add comment before iommu_remove_dt_device(). >>>> + >>>> + if ( !ops ) >>>> + return -EOPNOTSUPP; >>>> + >>>> + spin_lock(&dtdevs_lock); >>>> + >>>> + if ( iommu_dt_device_is_assigned_locked(np) ) >>>> + { >>>> + rc = -EBUSY; >>>> + goto fail; >>>> + } >>>> + >>>> + /* >>>> + * The driver which supports generic IOMMU DT bindings must have this >>>> + * callback implemented. >>>> + */ >>> J: >>> I have questioned this message in v7 and I still question it. I guess >>> you copied the comment on top of add_device(), this was add there >>> because we have a different way to add legacy device. >>> >>> But here there are no such requirement. In fact, you are not adding the >>> the callback to all the IOMMU drivers... Yet all of them support the >>> generic IOMMU DT bindings. >> Will change this. >>> >>>> + if ( !ops->remove_device ) >>>> + { >>>> + rc = -EOPNOTSUPP; >>>> + goto fail; >>>> + } >>>> + >>>> + /* >>>> + * Remove master device from the IOMMU if latter is present and available. >>> J: >>> I read this as this will not return an error if the device is protected. >>> However, AFAICT, the implement in the SMMU driver provided in this >>> series will return an error. So I would suggest to replace this sentence >>> with: >>> >>> de-register the device from the IOMMU driver. >> Will change the comment. >>> >>>> + * The driver is responsible for removing is_protected flag. >>> J: >>> Can you add an assert in the 'if ( !rc )' block to confirm that >>> is_protected was effectively removed. Something like: >>> >>> ASSERT(!dt_device_is_protected(dev)); >> Is ASSERT really required here. remove callback can return before setting is_protected as false. > I think Julien wanted to add extra check to make sure driver behaves as expected. > That said, his suggestion is incorrect since the callback can return before clearing the flag. > So, if ASSERT is required, this should be: > ASSERT(rc || !dt_device_is_protected(dev)); > so that we check for is_protected being false only on callback returning success (i.e. 0). I wrote this based on iommu_add_dt_device(), which does: if ( !rc ) rc = ops->add_device(0, dev); but looking at iommu_remove_dt_device(), where you have: rc = ops->remove_device(0, dev); if ( !rc ) iommu_fwspec_free(dev); you should do what Stefano suggested (i.e. just add ASSERT into ( !rc ) block) ~Michal
On Thu, Aug 31, 2023 at 09:32:48AM +0200, Michal Orzel wrote: > > > On 31/08/2023 09:23, Michal Orzel wrote: > > > > > > On 30/08/2023 19:48, Vikram Garhwal wrote: > >> Hi Michal, > >> On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote: > >>> > >>> > >>> On 25/08/2023 10:02, Vikram Garhwal wrote: > >>>> Remove master device from the IOMMU. This will be helpful when removing the > >>>> overlay nodes using dynamic programming during run time. > >>>> > >>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > >>>> Acked-by: Jan Beulich <jbeulich@suse.com> > >>> > >>> You don't seem to handle Julien remarks for this patch made in v9. > >>> I will forward them here to avoid answering to old version, but for the future, do not carry the exact same patch > >>> if you haven't yet addressed someone's remarks. > >> This got skipped as I cannot find direct email from Julien. The only email reply > >> on this patch is can find is from: xen-devel-bounces@lists.xenproject.org and > >> this got messed up with other larger set of email xen-devel sends. > >> > >> Did you get direct email? > >>> > >>>> > >>>> --- > >>>> Changes from v7: > >>>> Add check if IOMMU is enabled. > >>>> Fix indentation of fail. > >>>> --- > >>>> --- > >>>> xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++ > >>>> xen/include/xen/iommu.h | 1 + > >>>> 2 files changed, 45 insertions(+) > >>>> > >>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > >>>> index 1202eac625..3fad65fb69 100644 > >>>> --- a/xen/drivers/passthrough/device_tree.c > >>>> +++ b/xen/drivers/passthrough/device_tree.c > >>>> @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) > >>>> return 0; > >>>> } > >>>> > >>>> +int iommu_remove_dt_device(struct dt_device_node *np) > >>>> +{ > >>>> + const struct iommu_ops *ops = iommu_get_ops(); > >>>> + struct device *dev = dt_to_dev(np); > >>>> + int rc; > >>>> + > >>>> + if ( !iommu_enabled ) > >>>> + return 1; > >>> J: > >>> The caller doesn't seem to check if the error code is > 0. So can we > >>> instead return a -ERRNO? > >> Will change the check in caller. I want to keep this as it as so it looks > >> similar to iommu_add_dt_device(). > >>> > >>> If you want to continue to return a value > 0 then I think it should be > >>> documented in a comment like we did for iommu_add_dt_device(). > >>> > >> Will add comment before iommu_remove_dt_device(). > >>>> + > >>>> + if ( !ops ) > >>>> + return -EOPNOTSUPP; > >>>> + > >>>> + spin_lock(&dtdevs_lock); > >>>> + > >>>> + if ( iommu_dt_device_is_assigned_locked(np) ) > >>>> + { > >>>> + rc = -EBUSY; > >>>> + goto fail; > >>>> + } > >>>> + > >>>> + /* > >>>> + * The driver which supports generic IOMMU DT bindings must have this > >>>> + * callback implemented. > >>>> + */ > >>> J: > >>> I have questioned this message in v7 and I still question it. I guess > >>> you copied the comment on top of add_device(), this was add there > >>> because we have a different way to add legacy device. > >>> > >>> But here there are no such requirement. In fact, you are not adding the > >>> the callback to all the IOMMU drivers... Yet all of them support the > >>> generic IOMMU DT bindings. > >> Will change this. > >>> > >>>> + if ( !ops->remove_device ) > >>>> + { > >>>> + rc = -EOPNOTSUPP; > >>>> + goto fail; > >>>> + } > >>>> + > >>>> + /* > >>>> + * Remove master device from the IOMMU if latter is present and available. > >>> J: > >>> I read this as this will not return an error if the device is protected. > >>> However, AFAICT, the implement in the SMMU driver provided in this > >>> series will return an error. So I would suggest to replace this sentence > >>> with: > >>> > >>> de-register the device from the IOMMU driver. > >> Will change the comment. > >>> > >>>> + * The driver is responsible for removing is_protected flag. > >>> J: > >>> Can you add an assert in the 'if ( !rc )' block to confirm that > >>> is_protected was effectively removed. Something like: > >>> > >>> ASSERT(!dt_device_is_protected(dev)); > >> Is ASSERT really required here. remove callback can return before setting is_protected as false. > > I think Julien wanted to add extra check to make sure driver behaves as expected. > > That said, his suggestion is incorrect since the callback can return before clearing the flag. > > So, if ASSERT is required, this should be: > > ASSERT(rc || !dt_device_is_protected(dev)); > > so that we check for is_protected being false only on callback returning success (i.e. 0). > I wrote this based on iommu_add_dt_device(), which does: > if ( !rc ) > rc = ops->add_device(0, dev); > > but looking at iommu_remove_dt_device(), where you have: > rc = ops->remove_device(0, dev); > if ( !rc ) > iommu_fwspec_free(dev); > > you should do what Stefano suggested (i.e. just add ASSERT into ( !rc ) block) Added it in v11. > > ~Michal
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 1202eac625..3fad65fb69 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d) return 0; } +int iommu_remove_dt_device(struct dt_device_node *np) +{ + const struct iommu_ops *ops = iommu_get_ops(); + struct device *dev = dt_to_dev(np); + int rc; + + if ( !iommu_enabled ) + return 1; + + if ( !ops ) + return -EOPNOTSUPP; + + spin_lock(&dtdevs_lock); + + if ( iommu_dt_device_is_assigned_locked(np) ) + { + rc = -EBUSY; + goto fail; + } + + /* + * The driver which supports generic IOMMU DT bindings must have this + * callback implemented. + */ + if ( !ops->remove_device ) + { + rc = -EOPNOTSUPP; + goto fail; + } + + /* + * Remove master device from the IOMMU if latter is present and available. + * The driver is responsible for removing is_protected flag. + */ + rc = ops->remove_device(0, dev); + + if ( !rc ) + iommu_fwspec_free(dev); + + fail: + spin_unlock(&dtdevs_lock); + return rc; +} + int iommu_add_dt_device(struct dt_device_node *np) { const struct iommu_ops *ops = iommu_get_ops(); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 110693c59f..a8e9bc9a2d 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np); int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); +int iommu_remove_dt_device(struct dt_device_node *np); #endif /* HAS_DEVICE_TREE */