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

[Michal Orzel <michal.orzel@xxxxxxx>]

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).

>>
>> 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