Re: [PATCH v3 3/6] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/24/23 03:49, Michal Orzel wrote:
> Hi Stewart,
> 
> On 18/05/2023 23:06, Stewart Hildebrand wrote:
>>
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> The main purpose of this patch is to add a way to register PCI device
>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
>> before assigning that device to a domain.
>>
>> This behaves in almost the same way as existing iommu_add_dt_device API,
>> the difference is in devices to handle and DT bindings to use.
>>
>> The function of_map_id to translate an ID through a downstream mapping
>> (which is also suitable for mapping Requester ID) was borrowed from Linux
>> (v5.10-rc6) and updated according to the Xen code base.
>>
>> XXX: I don't port pci_for_each_dma_alias from Linux which is a part
>> of PCI-IOMMU bindings infrastucture as I don't have a good understanding
>> for how it is expected to work in Xen environment.
>> Also it is not completely clear whether we need to distinguish between
>> different PCI types here (DEV_TYPE_PCI, DEV_TYPE_PCI_HOST_BRIDGE, etc).
>> For example, how we should behave here if the host bridge doesn't have
>> a stream ID (so not described in iommu-map property) just simple
>> fail or bypasses translation?
>>
>> [1] 
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v2->v3:
>> * new patch title (was: iommu/arm: Introduce iommu_add_dt_pci_device API)
>> * renamed function
>>   from: iommu_add_dt_pci_device
>>   to: iommu_add_dt_pci_sideband_ids
>> * removed stale ops->add_device check
>> * iommu.h: add empty stub iommu_add_dt_pci_sideband_ids for !HAS_DEVICE_TREE
>> * iommu.h: add iommu_add_pci_sideband_ids helper
>> * iommu.h: don't wrap prototype in #ifdef CONFIG_HAS_PCI
>> * s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/
>>
>> v1->v2:
>> * remove extra devfn parameter since pdev fully describes the device
>> * remove ops->add_device() call from iommu_add_dt_pci_device(). Instead, 
>> rely on
>>   the existing iommu call in iommu_add_device().
>> * move the ops->add_device and ops->dt_xlate checks earlier
>>
>> downstream->v1:
>> * rebase
>> * add const qualifier to struct dt_device_node *np arg in dt_map_id()
>> * add const qualifier to struct dt_device_node *np declaration in 
>> iommu_add_pci_device()
>> * use stdint.h types instead of u8/u32/etc...
>> * rename functions:
>>   s/dt_iommu_xlate/iommu_dt_xlate/
>>   s/dt_map_id/iommu_dt_pci_map_id/
>>   s/iommu_add_pci_device/iommu_add_dt_pci_device/
>> * add device_is_protected check in iommu_add_dt_pci_device
>> * wrap prototypes in CONFIG_HAS_PCI
>>
>> (cherry picked from commit 734e3bf6ee77e7947667ab8fa96c25b349c2e1da 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 | 140 ++++++++++++++++++++++++++
>>  xen/include/xen/device_tree.h         |  25 +++++
>>  xen/include/xen/iommu.h               |  17 +++-
>>  3 files changed, 181 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c 
>> b/xen/drivers/passthrough/device_tree.c
>> index 1b50f4670944..d568166e19ec 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -151,6 +151,146 @@ static int iommu_dt_xlate(struct device *dev,
>>      return ops->dt_xlate(dev, iommu_spec);
>>  }
>>
>> +#ifdef CONFIG_HAS_PCI
>> +int iommu_dt_pci_map_id(const struct dt_device_node *np, uint32_t id,
>> +                        const char *map_name, const char *map_mask_name,
>> +                        struct dt_device_node **target, uint32_t *id_out)
>> +{
>> +    uint32_t map_mask, masked_id, map_len;
>> +    const __be32 *map = NULL;
>> +
>> +    if ( !np || !map_name || (!target && !id_out) )
>> +        return -EINVAL;
>> +
>> +    map = dt_get_property(np, map_name, &map_len);
>> +    if ( !map )
>> +    {
>> +        if ( target )
>> +            return -ENODEV;
> empty line here

OK

>> +        /* Otherwise, no map implies no translation */
>> +        *id_out = id;
>> +        return 0;
>> +    }
>> +
>> +    if ( !map_len || map_len % (4 * sizeof(*map)) )
> could you enclose the second expression in parantheses?

Yes

>> +    {
>> +        printk(XENLOG_ERR "%pOF: Error: Bad %s length: %d\n", np,
> %pOF is a Linux special printk format to print full name of the node.
> We do not have this in Xen (see printk-formats.txt). If you want to achieve 
> the same, use np->full_name.
> This applies to all the uses below.

OK

> Also, use %u for map_len as it is unsigned.

OK

>> +            map_name, map_len);
> incorrect alignment

OK

>> +        return -EINVAL;
>> +    }
>> +
>> +    /* The default is to select all bits. */
>> +    map_mask = 0xffffffff;
>> +
>> +    /*
>> +     * Can be overridden by "{iommu,msi}-map-mask" property.
>> +     * If of_property_read_u32() fails, the default is used.
> s/of_property_read_u32/dt_property_read_u32

OK

>> +     */
>> +    if ( map_mask_name )
>> +        dt_property_read_u32(np, map_mask_name, &map_mask);
>> +
>> +    masked_id = map_mask & id;
>> +    for ( ; (int)map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
>> +    {
>> +        struct dt_device_node *phandle_node;
>> +        uint32_t id_base = be32_to_cpup(map + 0);
>> +        uint32_t phandle = be32_to_cpup(map + 1);
>> +        uint32_t out_base = be32_to_cpup(map + 2);
>> +        uint32_t id_len = be32_to_cpup(map + 3);
>> +
>> +        if ( id_base & ~map_mask )
>> +        {
>> +            printk(XENLOG_ERR "%pOF: Invalid %s translation - %s-mask 
>> (0x%x) ignores id-base (0x%x)\n",
> we tend to use PRIx32 to print uint32_t values.

OK

>> +                   np, map_name, map_name, map_mask, id_base);
>> +            return -EFAULT;
>> +        }
>> +
>> +        if ( masked_id < id_base || masked_id >= id_base + id_len )
> could you enclose the expressions in parantheses?

Yes, I will follow MISRA C:2012 Rule 12.1

>> +            continue;
>> +
>> +        phandle_node = dt_find_node_by_phandle(phandle);
>> +        if ( !phandle_node )
>> +            return -ENODEV;
>> +
>> +        if ( target )
>> +        {
>> +            if ( !*target )
>> +                *target = phandle_node;
>> +
>> +            if ( *target != phandle_node )
>> +                continue;
>> +        }
>> +
>> +        if ( id_out )
>> +            *id_out = masked_id - id_base + out_base;
>> +
>> +        printk(XENLOG_DEBUG "%pOF: %s, using mask %08x, id-base: %08x, 
>> out-base: %08x, length: %08x, id: %08x -> %08x\n",
> %08"PRIx32"

OK

>> +               np, map_name, map_mask, id_base, out_base, id_len, id,
>> +               masked_id - id_base + out_base);
>> +        return 0;
>> +    }
>> +
>> +    printk(XENLOG_ERR "%pOF: no %s translation for id 0x%x on %pOF\n",
>> +           np, map_name, id, target && *target ? *target : NULL);
>> +
>> +    /*
>> +     * NOTE: Linux bypasses translation without returning an error here,
>> +     * but should we behave in the same way on Xen? Restrict for now.
>> +     */
>> +    return -EFAULT;
>> +}
>> +
>> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    struct dt_phandle_args iommu_spec = { .args_count = 1 };
>> +    struct device *dev = pci_to_dev(pdev);
>> +    const struct dt_device_node *np;
>> +    int rc = NO_IOMMU;
>> +
>> +    if ( !iommu_enabled )
>> +        return NO_IOMMU;
>> +
>> +    if ( !ops )
>> +        return -EINVAL;
>> +
>> +    if ( device_is_protected(dev) )
>> +        return 0;
>> +
>> +    if ( dev_iommu_fwspec_get(dev) )
>> +        return -EEXIST;
>> +
>> +    np = pci_find_host_bridge_node(pdev);
>> +    if ( !np )
>> +        return -ENODEV;
>> +
>> +    /*
>> +     * The driver which supports generic PCI-IOMMU DT bindings must have
>> +     * these callback implemented.
>> +     */
>> +    if ( !ops->dt_xlate )
>> +        return -EINVAL;
> See my comment in previous patch. This could be moved to iommu_dt_xlate().

OK

>> +
>> +    /*
>> +     * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
>> +     * from Linux.
>> +     */
>> +    rc = iommu_dt_pci_map_id(np, PCI_BDF(pdev->bus, pdev->devfn), 
>> "iommu-map",
>> +                             "iommu-map-mask", &iommu_spec.np, 
>> iommu_spec.args);
>> +    if ( rc )
>> +        return rc == -ENODEV ? NO_IOMMU : rc;
>> +
>> +    rc = iommu_dt_xlate(dev, &iommu_spec);
>> +    if ( rc < 0 )
>> +    {
>> +        iommu_fwspec_free(dev);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return rc;
>> +}
>> +#endif /* CONFIG_HAS_PCI */
>> +
>>  int iommu_add_dt_device(struct dt_device_node *np)
>>  {
>>      const struct iommu_ops *ops = iommu_get_ops();
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index c1e4751a581f..dc40fdfb9231 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -852,6 +852,31 @@ int dt_count_phandle_with_args(const struct 
>> dt_device_node *np,
>>   */
>>  int dt_get_pci_domain_nr(struct dt_device_node *node);
>>
>> +#ifdef CONFIG_HAS_PCI
>> +/**
>> + * iommu_dt_pci_map_id - Translate an ID through a downstream mapping.
>> + * @np: root complex device node.
>> + * @id: device ID to map.
>> + * @map_name: property name of the map to use.
>> + * @map_mask_name: optional property name of the mask to use.
>> + * @target: optional pointer to a target device node.
>> + * @id_out: optional pointer to receive the translated ID.
>> + *
>> + * Given a device ID, look up the appropriate implementation-defined
>> + * platform ID and/or the target device which receives transactions on that
>> + * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
>> + * @id_out may be NULL if only the other is required. If @target points to
>> + * a non-NULL device node pointer, only entries targeting that node will be
>> + * matched; if it points to a NULL value, it will receive the device node of
>> + * the first matching target phandle, with a reference held.
>> + *
>> + * Return: 0 on success or a standard error code on failure.
>> + */
>> +int iommu_dt_pci_map_id(const struct dt_device_node *np, uint32_t id,
>> +                        const char *map_name, const char *map_mask_name,
>> +                        struct dt_device_node **target, uint32_t *id_out);
> Why is the iommu function prototype in device_tree.h and not in iommu.h where 
> all rest of the iommu
> dt related prototypes are placed?

The function was borrowed from Linux's of_map_id, and it can be used to parse 
both "iommu-map" and "msi-map" properties.  So it is not necessarily iommu 
specific. I'm considering renaming it back to dt_map_id in v4.

> Furthermore, do you need to expose globally this function?
> Looking at this series it could be static as it is only used by 
> iommu_add_dt_pci_sideband_ids().
> Will there be any use of it later on?

Not in this series, but in the future MSI series it will be used with the 
"msi-map" binding. See [1]

[1] 
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/a8af686c327c2f2bdde321027abfda0b9ba4469c#15fe36652149e3439a60b50d9672982ca3de755e_290_301