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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]

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?

> >   * 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();
   ...
 }

> The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> dma_alias_devfn here in the IOMMU code.  
>
> 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.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]