Re: [PATCH v4 3/7] iommu/arm: Add iommu_dt_xlate()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 6/29/23 18:29, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> Move code for processing DT IOMMU specifier to a separate helper.
>> This helper will be re-used for adding PCI devices by the subsequent
>> patches as we will need exact the same actions for processing
>> DT PCI-IOMMU specifier.
>>
>> While at it introduce NO_IOMMU to avoid magic "1".
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v3->v4:
>> * make dt_phandle_args *iommu_spec const
>> * move !ops->add_device check to helper
>>
>> v2->v3:
>> * no change
>>
>> v1->v2:
>> * no change
>>
>> downstream->v1:
>> * trivial rebase
>> * s/dt_iommu_xlate/iommu_dt_xlate/
>>
>> (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
>>   the downstream branch poc/pci-passthrough from
>>   https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
>> ---
>>   xen/drivers/passthrough/device_tree.c | 47 ++++++++++++++++++---------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c 
>> b/xen/drivers/passthrough/device_tree.c
>> index c60e78eaf556..ff9e66ebf92a 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d)
>>       return 0;
>>   }
>>
>> +/* This correlation must not be altered */
> 
> Please expand why.

I don't have any insight regarding the rationale for why the comment was added. 
I don't believe the comment is adding any value, so I'll remove it.

>> +#define NO_IOMMU    1
> 
> Given that the value is returned, should not this be moved to an header
> and used by the callers?

Moving it to a header: yes. I'll move it to xen/include/xen/iommu.h, as that is 
where the iommu_add_dt_device() prototype also lives.

Used by callers: the callers currently are checking for error by comparing ( rc 
< 0 ). I think that's okay to leave as is since the comment in iommu.h by the 
prototype describe the possible return value for iommu_add_dt_device() as:
 * Return values:
 *  0 : device is protected by an IOMMU
 * <0 : device is not protected by an IOMMU, but must be (error condition)
 * >0 : device doesn't need to be protected by an IOMMU
 *      (IOMMU is not enabled/present or device is not connected to it).

>> +
>> +static int iommu_dt_xlate(struct device *dev,
>> +                          const struct dt_phandle_args *iommu_spec)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    int rc;
>> +
>> +    if ( !ops->dt_xlate )
>> +        return -EINVAL;
>> +
>> +    if ( !dt_device_is_available(iommu_spec->np) )
>> +        return NO_IOMMU;
>> +
>> +    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    /*
>> +     * Provide DT IOMMU specifier which describes the IOMMU master
>> +     * interfaces of that device (device IDs, etc) to the driver.
>> +     * The driver is responsible to decide how to interpret them.
>> +     */
>> +    return ops->dt_xlate(dev, iommu_spec);
>> +}
>> +
>>   int iommu_add_dt_device(struct dt_device_node *np)
>>   {
>>       const struct iommu_ops *ops = iommu_get_ops();
>>       struct dt_phandle_args iommu_spec;
>>       struct device *dev = dt_to_dev(np);
>> -    int rc = 1, index = 0;
>> +    int rc = NO_IOMMU, index = 0;
>>
>>       if ( !iommu_enabled )
>> -        return 1;
>> +        return NO_IOMMU;
>>
>>       if ( !ops )
>>           return -EINVAL;
>> @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>            * The driver which supports generic IOMMU DT bindings must have
>>            * these callback implemented.
> 
> The 's' was missing to callback. But now, I think you want to replace
> 'these' with 'this' as you move the second check.

I will fix it

>>            */
>> -        if ( !ops->add_device || !ops->dt_xlate )
>> +        if ( !ops->add_device )
>>               return -EINVAL;
>>
>> -        if ( !dt_device_is_available(iommu_spec.np) )
>> -            break;
>> -
>> -        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
>> -        if ( rc )
>> -            break;
>> -
>> -        /*
>> -         * Provide DT IOMMU specifier which describes the IOMMU master
>> -         * interfaces of that device (device IDs, etc) to the driver.
>> -         * The driver is responsible to decide how to interpret them.
>> -         */
>> -        rc = ops->dt_xlate(dev, &iommu_spec);
>> +        rc = iommu_dt_xlate(dev, &iommu_spec);
>>           if ( rc )
>>               break;
>>
> 
> Cheers,
> 
> -- 
> Julien Grall