On Wed, 2013-06-26 at 06:24 +0000, Sethi Varun-B16395 wrote:

quoted

-----Original Message-----
From: Alex Williamson [mailto:alex.williamson@redhat.com]
Sent: Tuesday, June 25, 2013 10:27 AM
To: Sethi Varun-B16395
Cc: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
benh@kernel.crashing.org; galak@kernel.crashing.org; Yoder Stuart-B08248;
Wood Scott-B07421; Timur Tabi
Subject: Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu
implementation.

On Thu, 2013-06-20 at 21:31 +0530, Varun Sethi wrote:

quoted

+#define REQ_ACS_FLAGS	(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |

PCI_ACS_UF)

quoted

+
+static struct iommu_group *get_device_iommu_group(struct device *dev)
+{
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		group = iommu_group_alloc();
+
+	return group;
+}
+

[snip]

quoted

+

This really gets parent or peer, right?

quoted

+static struct iommu_group *get_peer_pci_device_group(struct pci_dev
+*pdev) {
+	struct iommu_group *group = NULL;
+
+	/* check if this is the first device on the bus*/
+	if (pdev->bus_list.next == pdev->bus_list.prev) {

It's a list_head, use list functions.  The list implementation should be
treated as opaque.

if (list_is_singular(&pdev->bus_list))

quoted

+		struct pci_bus *bus = pdev->bus->parent;
+		/* Traverese the parent bus list to get
+		 * pdev & dev for the sibling device.
+		 */
+		while (bus) {
+			if (!list_empty(&bus->devices)) {
+				pdev = container_of(bus->devices.next,
+					            struct pci_dev, bus_list);

pdev = list_first_entry(&bus->devices, struct pci_dev, bus_list);

quoted

+				group = iommu_group_get(&pdev->dev);
+				break;
+			} else
+				bus = bus->parent;

Is this ever reached?  Don't you always have bus->self?

[Sethi Varun-B16395] Not sure I understand. Trying to get the group
information from the parent bus, if there are no sibling devices on
the current bus.

I assume there's always a bridge on a bus, but maybe that bridge
(parent->self) is not in the list of parent->devices?  Is that the case?
If not, then there's always a device on the bus, the bridge that created
it.

quoted

quoted

+		}
+	} else {
+		/*
+		 * Get the pdev & dev for the sibling device
+		 */
+		pdev = container_of(pdev->bus_list.prev,
+				    struct pci_dev, bus_list);

How do you know if you're at the head or tail of the list?

struct pci_dev *tmp;
list_for_each_entry(tmp, &pdev->bus_list, bus_list) {
	if (tmp == pdev)
		continue;

	group = iommu_group_get(&tmp->dev);
	break;
}

quoted

+		group = iommu_group_get(&pdev->dev);
+	}
+
+	return group;
+}
+
+static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
+{
+	struct iommu_group *group = NULL;
+	struct pci_dev *bridge, *dma_pdev = NULL;
+	struct pci_controller *pci_ctl;
+	bool pci_endpt_partioning;
+
+	pci_ctl = pci_bus_to_host(pdev->bus);
+	pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
+	/* We can partition PCIe devices so assign device group to the

device */

quoted

+	if (pci_endpt_partioning) {
+		bridge = pci_find_upstream_pcie_bridge(pdev);
+		if (bridge) {
+			if (pci_is_pcie(bridge))
+				dma_pdev = pci_get_domain_bus_and_slot(
+						pci_domain_nr(pdev->bus),
+						bridge->subordinate->number, 0);
+			if (!dma_pdev)
+				dma_pdev = pci_dev_get(bridge);
+		} else
+			dma_pdev = pci_dev_get(pdev);
+
+		/* Account for quirked devices */
+		swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
+
+		/*
+		 * If it's a multifunction device that does not support our
+		 * required ACS flags, add to the same group as function 0.
+		 */

See c14d2690 in Joerg's next tree, using function 0 was a poor
assumption.

[Sethi Varun-B16395] ok.

quoted

quoted

+		if (dma_pdev->multifunction &&
+		    !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS))
+			swap_pci_ref(&dma_pdev,
+				     pci_get_slot(dma_pdev->bus,
+						  PCI_DEVFN(PCI_SLOT(dma_pdev-
devfn),
+						  0)));
+
+		group = get_device_iommu_group(&pdev->dev);
+		pci_dev_put(pdev);

What was the point of all the above if we use pdev here instead of
dma_pdev?  Wrong device and broken reference counting.

[Sethi Varun-B16395] Will fix this

  This also isn't

quoted

testing ACS all the way up to the root complex or controller.

[Sethi Varun-B16395] In our case the IOMMU can differentiate
transactions based on the LIODN. The PCIe controller can generate a
unique LIODN based on the bus,device,function number. I believe this
would even be true for devices connected to a PCIe bridge (and not on
the root bus). So, do we still need to check for ACS up to the root
node?

ACS is the PCI capability that tells us whether a device allows
peer-to-peer or requires all DMA to be forwarded upstream.  At any step
along the way from an endpoint to the IOMMU the transaction can be
re-routed.  We therefore need to test the ACS property not only of the
endpoint, but every device between the endpoint and the IOMMU to form
the IOMMU groups.  An IOMMU cannot isolate a device if a DMA access is
re-routed by a link in the topology that prevents the IOMMU from even
being part of the transaction.  Thanks,

Alex