Re: [RFC PATCH v2 1/2] pci: Create PCIe requester ID interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/23/2013 06:35 PM, Bjorn Helgaas wrote:
On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
This provides interfaces for drivers to discover the visible PCIe
requester ID for a device, for things like IOMMU setup, and iterate

IDs (plural)

a single device does not have multiple requester id's;
can have multiple tag-id's (that are ignored in this situation, but
can be used by switches for ordering purposes), but there's only 1/fcn
(except for those quirker pdevs!).

over the device chain from requestee to requester, including DMA
quirks at each step.

"requestee" doesn't make sense to me.  The "-ee" suffix added to a verb
normally makes a noun that refers to the object of the action.  So
"requestee" sounds like it means something like "target" or "responder,"
but that's not what you mean here.

sorry, I didn't follow the requester/requestee terminology either...

Suggested-by: Bjorn Helgaas<bhelgaas@xxxxxxxxxx>
Signed-off-by: Alex Williamson<alex.williamson@xxxxxxxxxx>
---
  drivers/pci/search.c |  198 ++++++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci.h  |    7 ++
  2 files changed, 205 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index d0627fa..4759c02 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -18,6 +18,204 @@ DECLARE_RWSEM(pci_bus_sem);
  EXPORT_SYMBOL_GPL(pci_bus_sem);

  /*
+ * pci_has_pcie_requester_id - Does @dev have a PCIe requester ID
+ * @dev: device to test
+ */
+static bool pci_has_pcie_requester_id(struct pci_dev *dev)
+{
+	/*
+	 * XXX There's no indicator of the bus type, conventional PCI vs
+	 * PCI-X vs PCI-e, but we assume that a caller looking for a PCIe
+	 * requester ID is a native PCIe based system (such as VT-d or
+	 * AMD-Vi).  It's common that PCIe root complex devices do not
could the above comment be x86-iommu-neutral?
by definition of PCIe, all devices have a requester id (min. to accept cfg cycles);
req'd if source of read/write requests, read completions.

+	 * include a PCIe capability, but we can assume they are PCIe
+	 * devices based on their topology.
+	 */
+	if (pci_is_pcie(dev) || pci_is_root_bus(dev->bus))
+		return true;
+
+	/*
+	 * PCI-X devices have a requester ID, but the bridge may still take
+	 * ownership of transactions and create a requester ID.  We therefore
+	 * assume that the PCI-X requester ID is not the same one used on PCIe.
+	 */
+
+#ifdef CONFIG_PCI_QUIRKS
+	/*
+	 * Quirk for PCIe-to-PCI bridges which do not expose a PCIe capability.
+	 * If the device is a bridge, look to the next device upstream of it.
+	 * If that device is PCIe and not a PCIe-to-PCI bridge, then by
+	 * deduction, the device must be PCIe and therefore has a requester ID.
+	 */
+	if (dev->subordinate) {
+		struct pci_dev *parent = dev->bus->self;
+
+		if (pci_is_pcie(parent)&&
+		    pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+			return true;
+	}
+#endif
+
+	return false;
+}
+
+/*
+ * pci_has_visible_pcie_requester_id - Can @bridge see @dev's requester ID?
+ * @dev: requester device
+ * @bridge: upstream bridge (or NULL for root bus)
+ */
+static bool pci_has_visible_pcie_requester_id(struct pci_dev *dev,
+					      struct pci_dev *bridge)
+{
+	/*
+	 * The entire path must be tested, if any step does not have a
+	 * requester ID, the chain is broken.  This allows us to support
+	 * topologies with PCIe requester ID gaps, ex: PCIe-PCI-PCIe
+	 */
+	while (dev != bridge) {
+		if (!pci_has_pcie_requester_id(dev))
+			return false;
+
+		if (pci_is_root_bus(dev->bus))
+			return !bridge; /* false if we don't hit @bridge */
+
+		dev = dev->bus->self;
+	}
+
+	return true;
+}
+
+/*
+ * Legacy PCI bridges within a root complex (ex. Intel 82801) report
+ * a different requester ID than a standard PCIe-to-PCI bridge.  Instead
First, I'm assuming you mean that devices behind a Legacy PCI bridge within a root complex
get assigned IDs different than std PCIe-to-PCI bridges (as quoted below).

+ * of using (subordinate<<  8 | 0) the use (bus<<  8 | devfn), like a

s/the/they/

well, the PPBs should inject their (secondary << 8 | 0), not subordinate.
From the PCI Express to PCI/PCI-X Bridge spec, v1.0:
The Tag is reassigned by the bridge according to the rules outlined in the following sections. If the
bridge generates a new Requester ID for a transaction forwarded from the secondary interface to the
primary interface, the bridge assigns the PCI Express Requester ID using its secondary interface’s
                                                                             ^^^^^^^^^
Bus Number and sets both the Device Number and Function Number fields to zero.
^^^^^^^^^^

As for the 82801, looks like they took this part of the PCIe spec to heart:
(PCIe v3 spec, section 2.2.6.2 Transaction Descriptor - Transaction ID Field):
Exception: The assignment of Bus and Device Numbers to the Devices within a Root Complex,
and the Device Numbers to the Downstream Ports within a Switch, may be done in an
implementation specific way.
Obviously, you're missing the 'implementation-specific way' compiler... ;-)

aw: which 'bus' do you mean above in  '(bus<<  8 | devfn)' ?


Did you learn about this empirically?  Intel spec?  I wonder if there's
some way to derive this from the PCIe specs.

+ * standard PCIe endpoint.  This function detects them.
+ *
+ * XXX Is this Intel vendor ID specific?
+ */
+static bool pci_bridge_uses_endpoint_requester(struct pci_dev *bridge)
+{
+	if (!pci_is_pcie(bridge)&&  pci_is_root_bus(bridge->bus))
+		return true;
+
+	return false;
+}
+
+#define PCI_REQUESTER_ID(dev)	(((dev)->bus->number<<  8) | (dev)->devfn)
+#define PCI_BRIDGE_REQUESTER_ID(dev)	((dev)->subordinate->number<<  8)
+
+/*
+ * pci_get_visible_pcie_requester - Get requester and requester ID for
+ *                                  @requestee below @bridge
+ * @requestee: requester device
+ * @bridge: upstream bridge (or NULL for root bus)
+ * @requester_id: location to store requester ID or NULL
+ */
+struct pci_dev *pci_get_visible_pcie_requester(struct pci_dev *requestee,
+					       struct pci_dev *bridge,
+					       u16 *requester_id)

I'm not sure it makes sense to return a struct pci_dev here because
there's no requirement that a requester ID correspond to an actual
pci_dev.

well, I would expect the only callers would be for subsys (iommu's)
searching to find requester-id for a pdev, b/c if a pdev doesn't exist,
then the device (and requester-id) doesn't exist... :-/

+{
+	struct pci_dev *requester = requestee;
+
+	while (requester != bridge) {
+		requester = pci_get_dma_source(requester);
+		pci_dev_put(requester); /* XXX skip ref cnt */
+
+		if (pci_has_visible_pcie_requester_id(requester, bridge))

If we acquire the "requester" pointer via a ref-counting interface,
it's illegal to use the pointer after dropping the reference, isn't it?
Maybe that's what you mean by the "XXX" comment.

+			break;
+
+		if (pci_is_root_bus(requester->bus))
+			return NULL; /* @bridge not parent to @requestee */
+
+		requester = requester->bus->self;
+	}
+
+	if (requester_id) {
+		if (requester->bus != requestee->bus&&
+		    !pci_bridge_uses_endpoint_requester(requester))
+			*requester_id = PCI_BRIDGE_REQUESTER_ID(requester);
+		else
+			*requester_id = PCI_REQUESTER_ID(requester);
+	}
+
+	return requester;
+}
+
+static int pci_do_requester_callback(struct pci_dev **dev,
+				     int (*fn)(struct pci_dev *,
+					       u16 id, void *),
+				     void *data)
+{
+	struct pci_dev *dma_dev;
+	int ret;
+
+	ret = fn(*dev, PCI_REQUESTER_ID(*dev), data);
+	if (ret)
+		return ret;
+
+	dma_dev = pci_get_dma_source(*dev);
+	pci_dev_put(dma_dev); /* XXX skip ref cnt */
+	if (dma_dev == *dev)

Same ref count question as above.

+		return 0;
+
+	ret = fn(dma_dev, PCI_REQUESTER_ID(dma_dev), data);
+	if (ret)
+		return ret;
+
+	*dev = dma_dev;
+	return 0;
+}
+
+/*
+ * pcie_for_each_requester - Call callback @fn on each devices and DMA source
+ *                           from @requestee to the PCIe requester ID visible
+ *                           to @bridge.

Transactions from a device may appear with one of several requester IDs,
but there's not necessarily an actual pci_dev for each ID, so I think the
ditto above; have to have a pdev for each id....

caller reads better if it's "...for_each_requester_id()"

The "Call X on each devices and DMA source from Y to the requester ID"
part doesn't quite make a sentence.

+ * @requestee: Starting device
+ * @bridge: upstream bridge (or NULL for root bus)

You should say something about the significance of @bridge.  I think the
point is to call @fn for every possible requester ID @bridge could see for
transactions from @requestee.  This is a way to learn the requester IDs an
IOMMU at @bridge needs to be prepared for.

+ * @fn: callback function
+ * @data: data to pass to callback
+ */
+int pcie_for_each_requester(struct pci_dev *requestee, struct pci_dev *bridge,
+			    int (*fn)(struct pci_dev *, u16 id, void *),
+			    void *data)
+{
+	struct pci_dev *requester;
+	struct pci_dev *dev = requestee;
+	int ret = 0;
+
+	requester = pci_get_visible_pcie_requester(requestee, bridge, NULL);
+	if (!requester)
+		return -EINVAL;
+
+	do {
+		ret = pci_do_requester_callback(&dev, fn, data);
+		if (ret)
+			return ret;
+
+		if (dev == requester)
+			return 0;
+
+		/*
+		 * We always consider root bus devices to have a visible
+		 * requester ID, therefore this should never be true.
+		 */
+		BUG_ON(pci_is_root_bus(dev->bus));

What are we going to do if somebody hits this BUG_ON()?  If it's impossible
to hit, we should just remove it.  If it's possible to hit it in some weird
topology you didn't consider, we should see IOMMU faults for any requester
ID we neglected to map, and that fault would be a better debugging hint
than a BUG_ON() here.

according to spec, all pdev's have a requester-id, even RC ones, albeit
"implementation specific"...

+
+		dev = dev->bus->self;
+
+	} while (dev != requester);
+
+	/*
+	 * If we've made it here, @requester is a bridge upstream from
+	 * @requestee.
+	 */
+	if (pci_bridge_uses_endpoint_requester(requester))
+		return pci_do_requester_callback(&requester, fn, data);
+
+	return fn(requester, PCI_BRIDGE_REQUESTER_ID(requester), data);
+}
+
+/*
   * find the upstream PCIe-to-PCI bridge of a PCI device
   * if the device is PCIE, return NULL
   * if the device isn't connected to a PCIe bridge (that is its parent is a
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3a24e4f..94e81d1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1873,6 +1873,13 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
  }
  #endif

+struct pci_dev *pci_get_visible_pcie_requester(struct pci_dev *requestee,
+					       struct pci_dev *bridge,
+					       u16 *requester_id);

The structure of this interface implies that there is only one visible
requester ID, but the whole point of this patch is that a transaction from
@requestee may appear with one of several requester IDs.  So which one will
this return?

Are there devices that use multiple requester id's?
I know we have ones that use the wrong id.
If we want to handle the multiple requester-id's per pdev,
we could pass in a ptr to an initial requester-id; if null, give me first;
if !null, give me next one;  would also need a flag returned to indicate
there is more than 1.
null-rtn when pass in !null ref, means the end.
 ... sledge-hammer + nail ???

+int pcie_for_each_requester(struct pci_dev *requestee, struct pci_dev *bridge,
+			    int (*fn)(struct pci_dev *, u16 id, void *),
+			    void *data);
+
  /**
   * pci_find_upstream_pcie_bridge - find upstream PCIe-to-PCI bridge of a device
   * @pdev: the PCI device


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux