Re: [Xen-devel] [PATCH 4/5] iommu: introduce iommu_groups

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 08, 2019 at 02:24:02PM +0100, Paul Durrant wrote:
> Some devices may share a single PCIe initiator id, e.g. if they are actually
> legacy PCI devices behind a bridge, and hence DMA from such devices will
> be subject to the same address translation in the IOMMU. Hence these devices
> should be treated as a unit for the purposes of assignment. There are also
> other reasons why multiple devices should be treated as a unit, e.g. those
> subject to a shared RMRR or those downstream of a bridge that does not
> support ACS.
> 
> This patch introduces a new struct iommu_group to act as a container for
> devices that should be treated as a unit, and builds a list of them as
> PCI devices are scanned. The iommu_ops already implement a method,
> get_device_group_id(), that is seemingly intended to return the initiator
> id for a given SBDF so use this as the mechanism for group assignment in
> the first instance. Assignment based on shared RMRR or lack of ACS will be
> dealt with in subsequent patches, as will modifications to the device
> assignment code.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/iommu.c | 76 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   |  3 ++
>  xen/include/xen/iommu.h         |  7 ++++
>  xen/include/xen/pci.h           |  3 ++
>  4 files changed, 89 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index d3a6199b77..11319fbaae 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
>      }
>  }
>  
> +#ifdef CONFIG_HAS_PCI
> +
> +struct iommu_group {
> +    unsigned int id;
> +    unsigned int index;

I'm not sure I see the point of the index field, isn't it enough to
just use the ID field?

The ID is already used as a unique key in the code below for the radix
tree operations.

> +    struct list_head devs_list;
> +};
> +
> +static struct radix_tree_root iommu_groups;
> +
> +void __init iommu_groups_init(void)
> +{
> +    radix_tree_init(&iommu_groups);
> +}
> +
> +static struct iommu_group *alloc_iommu_group(unsigned int id)
> +{
> +    struct iommu_group *grp;
> +    static unsigned int index;
> +
> +    grp = xzalloc(struct iommu_group);

Can be moved with the declaration above.

> +    if ( !grp )
> +        return NULL;
> +
> +    grp->id = id;
> +    grp->index = index++;

AFAICT none of this is subject to races because it's always protected
by the pcidevs lock?

> +    INIT_LIST_HEAD(&grp->devs_list);
> +
> +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> +    {
> +        xfree(grp);
> +        grp = NULL;

Do you need to decrease index here, or is it fine to burn numbers on
failure?

> +    }
> +
> +    return grp;
> +}
> +
> +static struct iommu_group *get_iommu_group(unsigned int id)
> +{
> +    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
> +
> +    if ( !grp )
> +        grp = alloc_iommu_group(id);
> +
> +    return grp;
> +}
> +
> +int iommu_group_assign(struct pci_dev *pdev)
> +{
> +    const struct iommu_ops *ops;
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    ops = iommu_get_ops();

This initialization can be done at declaration time.

> +    if ( !ops || !ops->get_device_group_id )
> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> +    grp = get_iommu_group(id);
> +
> +    if ( ! grp )
             ^ extra space
> +        return -ENOMEM;
> +
> +    if ( iommu_verbose )
> +        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
> +               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +               PCI_FUNC(pdev->devfn), grp->index);

Wouldn't it be more helpful to print the group ID rather than the Xen
internal index?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel