RE: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommuimplementation.

[Sethi Varun-B16395]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: Tuesday, June 25, 2013 10:27 AM
> To: Sethi Varun-B16395
> Cc: joro@xxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linuxppc-
> dev@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> benh@xxxxxxxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxxxxxxx; 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:
> 
> > +#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?
> 
[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.

> > +		}
> > +	} 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.
[Sethi Varun-B16395] ok.

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

-Varun
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—