RE: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
-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-ker...@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.
+ }
+ } 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
Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
On Wed, 2013-06-26 at 06:24 +, 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-ker...@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
Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu implementation.
On Thu, Jun 20, 2013 at 09:31:28PM +0530, Varun Sethi wrote: This patch provides the PAMU driver (fsl_pamu.c) and the corresponding IOMMU API implementation (fsl_pamu_domain.c). The PAMU hardware driver (fsl_pamu.c) has been derived from the work done by Ashish Kalra and Timur Tabi. AlexW, can you please have a look at the group-code again and ack the patch if it looks right to you? Thanks, Joerg ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
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).
+
