Re: [PATCH v3 2/6] iommu/arm: Add iommu_dt_xlate()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/22/23 10:48, Michal Orzel wrote:
> Hi Stewart,
> 
> On 18/05/2023 23:06, 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> # rename
>> ---
>> 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 | 42 +++++++++++++++++----------
>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c 
>> b/xen/drivers/passthrough/device_tree.c
>> index b5bd13393b56..1b50f4670944 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -127,15 +127,39 @@ int iommu_release_dt_devices(struct domain *d)
>>      return 0;
>>  }
>>
>> +/* This correlation must not be altered */
>> +#define NO_IOMMU    1
>> +
>> +static int iommu_dt_xlate(struct device *dev,
>> +                          struct dt_phandle_args *iommu_spec)
> I think iommu_spec can be const.

Yes, good catch

>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    int rc;
>> +
>> +    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);
> Wouldn't it be better to move the check (!ops->dt_xlate) from 
> iommu_add_dt_device to this helper?
> After all it is the only function that calls dt_xlate so for me it would be a 
> natural placement.
> Looking at the next patch it will also reduce the similar check in 
> iommu_add_dt_pci_sideband_ids.

Yes, I will move it