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.garh...@amd.com>
> >>>> Acked-by: Jan Beulich <jbeul...@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-boun...@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
