Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 15, 2016 at 07:48:17PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:
> > On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> > > 
> > > >  /*
> > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > + * aliases are only supported on the same bus, therefore the search
> > > 
> > > I'm trying to reconcile this statement that "DMA aliases are only
> > > supported on the same bus" (which was there even before this patch)
> > > with the fact that pci_for_each_dma_alias() does not have that
> > > limitation.
> > 
> > Doesn't it? You can still only set a DMA alias on the same bus with
> > pci_add_dma_alias(), can't you?
> 
> I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
> pci_add_dma_alias() only add aliases on the same bus.  I was thinking
> about a scenario like this:
> 
>   00:00.0 PCIe-to-PCI bridge to [bus 01]
>   01:01.0 conventional PCI device
> 
> where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
> takes ownership of DMA transactions from 01:01.0 and assigns a
> Requester ID of 01:00.0 (secondary bus number, device 0, function 0).
> 
> > > >   * space is quite small (especially since we're really only looking at pcie
> > > >   * device, and therefore only expect multiple slots on the root complex or
> > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > > >                       continue;
> > > >  
> > > >               /* We alias them or they alias us */
> > > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > > -
> > > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > >                       group = get_pci_alias_group(tmp, devfns);
> > > 
> > > We basically have this:
> > > 
> > >   for_each_pci_dev(tmp) {
> > >     if ()
> > >       group = get_pci_alias_group();
> > >       ...
> > >   }
> > 
> > Strictly, that's:
> > 
> >  for_each_pci_dev(tmp) {
> >    if (pdev is an alias of tmp || tmp is an alias of pdev)
> >      group = get_pci_alias_group();
> >    ...
> >  }
> 
> OK.  
> 
> > > I'm trying to figure out why we don't do something like the following
> > > instead:
> > > 
> > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > >   {
> > >     struct iommu_group *group;
> > > 
> > >     group = get_pci_alias_group();
> > >     if (group)
> > >       return group;
> > > 
> > >     return 0;
> > >   }
> > > 
> > >   pci_for_each_dma_alias(pdev, callback, ...);
> > 
> > And this would be equivalent to
> > 
> >  for_each_pci_dev(tmp) {
> >    if (tmp is an alias of pdev)
> >      group = get_pci_alias_group();
> >    ...
> >  }
> > 
> > The "is an alias of" property is not commutative. Perhaps it should be.
> > But that's hard because in some cases the alias doesn't even *exist* as
> > a real PCI device. It's just that you appear to get DMA transactions
> > from a given source-id.
> 
> Right.  In my example above, 01:00.0 is not a PCI device; it's only a
> Requester ID that is fabricated by the bridge when it forwards DMA
> transactions upstream.
> 
> I think I'm confused because I don't really understand IOMMU groups.
> 
> Let me explain what I think they are and you can correct me when I go
> wrong.  The iommu_group_alloc() comment says "The IOMMU group
> represents the minimum granularity of the IOMMU."  So I suppose the
> IOMMU cannot distinguish between devices in a group.  All the devices
> in the group use the same set of DMA mappings.  Granting device A DMA
> access to a buffer grants the same access to all other members of A's
> IOMMU group.
> 
> That would mean my question was fundamentally backwards.  In
> get_pci_alias_group(A), we're not trying to figure out what all the
> aliases of A are, which is what pci_for_each_dma_alias() does.
> 
> Instead, we're trying to figure out which IOMMU group A belongs to.
> But I still don't quite understand how aliases fit into this.  Let's
> go back to my example and assume we've already put 00:00.0 and 01:01.0
> in IOMMU groups:
> 
>   00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
>   01:01.0 conventional PCI device            # in IOMMU group G1
> 
> I assume these devices are in different IOMMU groups because if the
> bridge generated its own DMA, it would use Requester ID 00:00.0, which
> is distinct from the 01:00.0 it would use when forwarding DMAs from
> its secondary side.
> 
> What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0 should
> both end up in IOMMU group G1 because the IOMMU will see only
> Requester ID 01:00.0, so it can't distinguish them.
> 
> When we add 01:02.0, the ops->add_device() ... ops->device_group()
> path calls pci_device_group(01:02.0):
> 
>   pci_device_group(01:02.0)
>     pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
>       get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
>         return 0           # 01:02.0 group not set yet
>       get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
>         return 1           # 00:00.0 is in G0
> 
> It seems like we'll assign 01:02.0 to group G0, when I think it should
> be in G1.  Where did I go wrong?

Ping?