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

[Michal Orzel <michal.orzel@xxxxxxx>]

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)

~Michal