Re: [XEN][PATCH v10 11/20] xen/iommu: Introduce iommu_remove_dt_device()

[Vikram Garhwal <vikram.garhwal@amd.com>]

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