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:
smime.p7s
Description: S/MIME cryptographic signature
