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: 16 July 2019 09:57
> 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 05:41:27PM +0200, Paul Durrant wrote:
> > > -----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.
> 
> Oh right, sorry for the noise.
> 
> > >
> > > > +{
> > > > +    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.
> 
> With that you can add my:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks :-)

  Paul

> 
> Thanks, Roger.

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