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

[Michal Orzel <michal.orzel@xxxxxxx>]

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

> +        /* 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?

> +    {
> +        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.
Also, use %u for map_len as it is unsigned.

> +            map_name, map_len);
incorrect alignment

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

> +     */
> +    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.

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

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

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

> +
> +    /*
> +     * 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?

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?

~Michal