On Wed, 2013-06-26 at 06:24 +0000, Sethi Varun-B16395 wrote:
>
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Tuesday, June 25, 2013 10:27 AM
> > To: Sethi Varun-B16395
> > Cc: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> > d...@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > b...@kernel.crashing.org; ga...@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:
> >
> > > +#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.
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.
> > > + }
> > > + } 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?
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
