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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 09:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; George 
> Dunlap
> <George.Dunlap@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian 
> Jackson
> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Julien Grall 
> <julien.grall@xxxxxxx>; Jan
> Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 4/5] iommu: introduce iommu_groups
> 
> 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 index is just supposed to just be an index to refer to the group. Linux has 
similar, and this is what ends up in sysfs, but I guess there's not much point 
having this in Xen as yet... It can always be added later if it proves 
desirable.

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

Yes, that's the meaningful number.

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

Sure.

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

Yes, it's under 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?
> 

I'll get rid of the index.

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

Sure.

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

Yes, once the index is gone I'll do that instead.

  Paul

> 
> Thanks, Roger.

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