[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN][PATCH v10 11/20] xen/iommu: Introduce iommu_remove_dt_device()
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@xxxxxxx> > >>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx> > >>> > >>> 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@xxxxxxxxxxxxxxxxxxxx > >> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |