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

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

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, Roger.

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