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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: 15 July 2019 16:35
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>;
> 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 v2 3/4] iommu: introduce iommu_groups
> 
> On Mon, Jul 15, 2019 at 01:37:09PM +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>
> 
> LGTM, just two comments below.
> 
> > ---
> > 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 <wl@xxxxxxx>
> >
> > v2:
> >  - Move code into new drivers/passthrough/groups.c
> >  - Drop the group index.
> >  - Handle failure to get group id.
> >  - Drop the group devs list.
> > ---
> >  xen/drivers/passthrough/Makefile    |  1 +
> >  xen/drivers/passthrough/groups.c    | 91 
> > +++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/x86/iommu.c |  8 +++-
> >  xen/include/xen/iommu.h             |  7 +++
> >  xen/include/xen/pci.h               |  2 +
> >  5 files changed, 108 insertions(+), 1 deletion(-)
> >  create mode 100644 xen/drivers/passthrough/groups.c
> >
> > diff --git a/xen/drivers/passthrough/Makefile 
> > b/xen/drivers/passthrough/Makefile
> > index d50ab188c8..8a77110179 100644
> > --- a/xen/drivers/passthrough/Makefile
> > +++ b/xen/drivers/passthrough/Makefile
> > @@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
> >  subdir-$(CONFIG_ARM) += arm
> >
> >  obj-y += iommu.o
> > +obj-$(CONFIG_HAS_PCI) += groups.o
> >  obj-$(CONFIG_HAS_PCI) += pci.o
> >  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> >
> > diff --git a/xen/drivers/passthrough/groups.c 
> > b/xen/drivers/passthrough/groups.c
> > new file mode 100644
> > index 0000000000..1a2f461c87
> > --- /dev/null
> > +++ b/xen/drivers/passthrough/groups.c
> > @@ -0,0 +1,91 @@
> > +/*
> > + * Copyright (c) 2019 Citrix Systems Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/iommu.h>
> > +#include <xen/radix-tree.h>
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +};
> > +
> > +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 = xzalloc(struct iommu_group);
> > +
> > +    if ( !grp )
> > +        return NULL;
> > +
> > +    grp->id = id;
> > +
> > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > +    {
> > +        xfree(grp);
> > +        grp = NULL;
> > +    }
> > +
> > +    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, void *arg)
> 
> I'm not sure I see the point of the arg parameter, AFAICT it's
> completely unused.

It needs to be there because it needs to conform to the all device iterator 
function callback prototype. It is indeed unused though.

> 
> > +{
> > +    const struct iommu_ops *ops = iommu_get_ops();
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    if ( !ops->get_device_group_id )
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> 
> I think I would prefer id to be of signed type here, then when you
> pass it to get_iommu_group it's already made unsigned.

That's a good catch. It's tested for < 0 below so it does need to be signed at 
this point.

  Paul

> 
> Thanks, Roger.

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