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:

+#define REQ_ACS_FLAGS	(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
+
+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]

+

This really gets parent or peer, right?

+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))

+		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);

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

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

+		}
+	} 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;
}

+		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 */
+	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.

+		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.  This also isn't
testing ACS all the way up to the root complex or controller.

+		/*
+		 * PCIe controller is not a paritionable entity
+		 * free the controller device iommu_group.
+		 */
+		if (pci_ctl->parent->iommu_group)
+			iommu_group_remove_device(pci_ctl->parent);
+	} else {
+		/*
+		 * All devices connected to the controller will share the
+		 * PCI controllers device group. If this is the first
+		 * device to be probed for the pci controller, copy the
+		 * device group information from the PCI controller device
+		 * node and remove the PCI controller iommu group.
+		 * For subsequent devices, the iommu group information can
+		 * be obtained from sibling devices (i.e. from the bus_devices
+		 * link list).
+		 */
+		if (pci_ctl->parent->iommu_group) {
+			group = get_device_iommu_group(pci_ctl->parent);
+			iommu_group_remove_device(pci_ctl->parent);
+		} else
+			group = get_peer_pci_device_group(pdev);
+	}
+
+	return group;
+}
+
+static int fsl_pamu_add_device(struct device *dev)
+{
+	struct iommu_group *group = NULL;
+	struct pci_dev *pdev;
+	const u32 *prop;
+	int ret, len;
+
+	/*
+	 * For platform devices we allocate a separate group for
+	 * each of the devices.
+	 */
+	if (dev->bus == &pci_bus_type) {
+		pdev = to_pci_dev(dev);
+		/* Don't create device groups for virtual PCI bridges */
+		if (pdev->subordinate)
+			return 0;
+
+		group = get_pci_device_group(pdev);
+
+	} else {
+		prop = of_get_property(dev->of_node, "fsl,liodn", &len);
+		if (prop)
+			group = get_device_iommu_group(dev);
+	}
+
+	if (!group || IS_ERR(group))
+		return PTR_ERR(group);
+
+	ret = iommu_group_add_device(group, dev);
+
+	iommu_group_put(group);
+	return ret;
+}
+
+static void fsl_pamu_remove_device(struct device *dev)
+{
+	iommu_group_remove_device(dev);
+}