> -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx] > Sent: Thursday, February 25, 2016 3:39 PM > To: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@xxxxxxxxx>; linux- > pci@xxxxxxxxxxxxxxx; Alex Williamson <alex.williamson@xxxxxxxxxx>; Joerg > Roedel <jroedel@xxxxxxx>; David Woodhouse <dwmw2@xxxxxxxxxxxxx>; > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases > > On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote: > > From: Jacek Lawrynowicz <jacek.lawrynowicz@xxxxxxxxx> > > > > <Insert changelog here> > > (Sorry, I should have copied this changelog in the patch; I copied > this manually from your v3 posting): > > > This patch solves IOMMU support issues with PCIe non-transparent bridges > > that use Requester ID look-up tables (LUT), e.g. PEX8733. Before exiting > > the bridge, packet's RID is rewritten according to LUT programmed by > > a driver. Modified packets are then passed to a destination bus and > > processed upstream. The problem is that such packets seem to come from > > non-existent nodes that are hidden behind NTB and are not discoverable > > by a destination node, so IOMMU discards them. Adding DMA alias for a > > given LUT entry allows IOMMU to create a proper mapping that enables > > inter-node communication. > > A specific example here would help me understand. Here's how I > understand this (correct me if I'm wrong): We're talking about a DMA > packet being forwarded upstream from an NTB. The NTB uses the LUT to > rewrite the RID in the DMA packet. The new RID from the LUT is > unknown to the IOMMU, so it discards the DMA packet. Yes, this is exactly the problem. > > The current DMA alias implementation supports only single alias, so it's > > not possible to connect more than two nodes when IOMMU is enabled. This > > implementation enables all possible aliases on a given bus (256) that > > are stored in a bitset. Alias devfn is directly translated to a bit > > number. The bitset is not allocated for devices that have no need for > > DMA aliases. > > I think "two nodes" is referring to two PCIe devices on the other side > of the NTB. You want DMA packets from those devices to have different > RIDs so the IOMMU can distinguish them. Right. > The LUT entries basically create aliases of the NTB (one alias for > each device beyond the NTB). Your quirk uses pci_add_dma_alias(), and > the aliases are all on the same bus as the NTB itself. > > The quirk adds PCI_DEVFN(0x10, 0x0), PCI_DEVFN(0x11, 0x0), and > PCI_DEVFN(0x12, 0x0). Shouldn't there be some connection between this > and the LUT programming? I assume the LUT is programmed to correspond > to those aliases. Does this mean you're limited to three devices > beyond the NTB? Yes, there is an indirect connection between LUT table and devfns used in the quirk. Dev part is an offset in the LUT table and function is taken from the device behind the NTB. So the driver can only change the dev part by using different LUT offsets. We don't plan to modify this quirk. The number of PCIe devices beyond single x200 card NTB will not change. Two are used by x200 CPU (host bridge & root port) and one is used by x200 DMA engine. I'm not sure introducing some dependencies to make sure the offsets are set correctly is really worth it. So regarding the improvements in the patch description, you want me to update and repost it? BTW I posted x200 DMA driver (the client for this change) on DMA list: https://lkml.org/lkml/2016/2/9/287 I'm working on integrating review comments and hope to get it included in 4.6. Regards, Jacek > > --- > > drivers/iommu/iommu.c | 17 ++++++++++------- > > drivers/pci/pci.c | 11 +++++++++-- > > drivers/pci/probe.c | 1 + > > drivers/pci/search.c | 14 +++++++++----- > > include/linux/pci.h | 4 +--- > > 5 files changed, 30 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index 0e3b009..a214e19 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -659,9 +659,15 @@ static struct iommu_group > *get_pci_function_alias_group(struct pci_dev *pdev, > > return NULL; > > } > > > > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn) > > +{ > > + return dev->dma_alias_mask && > > + test_bit(devfn, dev->dma_alias_mask); > > +} > > + > > /* > > - * 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. > > > * 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 (<pdev and tmp are DMA aliases>) > 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, ...); > > Is the existing code some sort of optimization, e.g., checking > PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using > pci_for_each_dma_alias()? > > It seems like this won't work for some very unlikely but theoretically > possible topologies, e.g., > > PCIe Root Complex/IOMMU > PCIe switch A > PCIe to conventional PCI bridge > PCI to PCIe Root Complex > PCIe NTB > > Here, I think the IOMMU will only see RIDs from PCIe switch A, but the > current code only looks at DMA aliases that are on the same bus as the > PCIe NTB. Wouldn't using pci_for_each_dma_alias() handle this > correctly? > > > if (group) { > > pci_dev_put(tmp);
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
