[RFC PATCH v2 0/2] pci/iommu: PCIe requester ID interface

[RFC PATCH v2 0/2] pci/iommu: PCIe requester ID interface

[Alex Williamson]

v2: second pass at refining the interface.  Rolled pci_requester_id
into pci_get_visible_pcie_requester.  Fixed some bus walk issues.

Another obscure issue that I forgot to mention that this should fix
is gaps in requester IDs.  If you had:

 ---pcie--->[pcie2pci]---pci--->[pci2pcie]---pcie--->[endpoint]

I think the current code would try to use the endpoint requester id and
fail.  This code will use the pcie2pci->secondary as the requester id.
This may not even be that obscure of a bug if a PCIe ASIC is soldered
to a pci2pcie bridge to make a compatible conventional PCI card.

The diffstat for pci improves a bit when we remove:
pci_find_upstream_pci_bridge:

 drivers/iommu/intel-iommu.c         |  164 ++++++++++-----------------
 drivers/iommu/intel_irq_remapping.c |    2 
 drivers/pci/search.c                |  215 +++++++++++++++++++++++++++++++-----
 include/linux/pci.h                 |   16 +-
 4 files changed, 260 insertions(+), 137 deletions(-)

Comments?  Thanks,

Alex

---

Alex Williamson (2):
      pci: Create PCIe requester ID interface
      iommu/intel: Make use of PCIe requester ID interface


 drivers/iommu/intel-iommu.c         |  164 +++++++++++------------------
 drivers/iommu/intel_irq_remapping.c |    2 
 drivers/pci/search.c                |  198 +++++++++++++++++++++++++++++++++++
 include/linux/pci.h                 |    7 +
 4 files changed, 270 insertions(+), 101 deletions(-)

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

[Alex Williamson]

This provides interfaces for drivers to discover the visible PCIe
requester ID for a device, for things like IOMMU setup, and iterate
over the device chain from requestee to requester, including DMA
quirks at each step.

Suggested-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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
+	 * 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
+ * of using (subordinate << 8 | 0) the use (bus << 8 | devfn), like a
+ * 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)
+{
+	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))
+			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)
+		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.
+ * @requestee: Starting device
+ * @bridge: upstream bridge (or NULL for root bus)
+ * @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));
+
+		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);
+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

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

[Bjorn Helgaas]

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)

> 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.

> Suggested-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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
> +	 * 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
> + * of using (subordinate << 8 | 0) the use (bus << 8 | devfn), like a

s/the/they/

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.

> +{
> +	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
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.

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

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

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

[Alex Williamson]

On Tue, 2013-07-23 at 16:35 -0600, 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)

How does a device can't have multiple requester IDs?  Reading below, I'm
not sure we're on the same page for the purpose of this patch.

> > 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.

Hmm, ok.  I figured a request-er makes a request on behalf of a
request-ee.  Suggestions?

> > Suggested-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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
> > +	 * 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
> > + * of using (subordinate << 8 | 0) the use (bus << 8 | devfn), like a
> 
> s/the/they/
> 
> Did you learn about this empirically?  Intel spec?  I wonder if there's
> some way to derive this from the PCIe specs.

It's in the current intel-iommu logic, pretty much anywhere it uses
pci_find_upstream_pcie_bridge() it follows that up with a pci_is_pcie()
check.  If it's PCIe, it uses a traditional PCIe-to-PCI bridge ID.  If
it's a legacy PCI bridge it uses the bridge ID itself.

static int
domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
                        int translation)
{
...
        /* dependent device mapping */
        tmp = pci_find_upstream_pcie_bridge(pdev);
        if (!tmp)
                return 0;
...
        if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
                return domain_context_mapping_one(domain,
                                        pci_domain_nr(tmp->subordinate),
                                        tmp->subordinate->number, 0,
                                        translation);
        else /* this is a legacy PCI bridge */
                return domain_context_mapping_one(domain,
                                                  pci_domain_nr(tmp->bus),
                                                  tmp->bus->number,
                                                  tmp->devfn,
                                                  translation);

The 82801 reference is from looking at when this would actually happen
on one of my own systems.


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

That's why this function is named get_.._requester instead of requester
ID.  I believe there still has to be a struct pci_dev that does the
request, but the requester ID for that device may not actually match.
So I return both.  In a PCIe-to-PCI bridge case, the pci_dev is the
bridge, but the requester ID is either the bridge bus|devfn or
subordinate|0 depending on the topology.  If we want to support "ghost
functions", we can return the real pci_dev and a ghost requester ID.

I think if we used just a requester ID, it ends up being extremely
difficult to pass that into anything else since we then have to search
again for where that requester ID is rooted.

> > +{
> > +	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.


Yes, I was just following your RFC lead and didn't want to deal with
reference counting until this approach had enough traction.


> > +			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
> caller reads better if it's "...for_each_requester_id()"

Wouldn't you expect to pass a requester ID into a function with that
name?  I'm pretty sure I had it named that at one point but thought the
parameters made more sense this way.  I'll see if I can think of a
better name.

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


Ok

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

I can add something.  @bridge is supposed to be for bridge-based IOMMUs.
Essentially it's just a stopping point.  There might be PCI topology
upstream of @bridge, but if you only want the requester ID seen by the
bridge, you don't care.

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

It's mostly for readability.  I've learned that we never what to look at
dev->bus->self without first testing pci_is_root_bus(dev->bus), so if I
was reading this code I'd stumble when I got here and spend too long
looking around for the assumption that makes it not needed.  I suppose I
could just make it a comment, but I thought why not make it official w/
a BUG.

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

I thought the point of this patch was to have an integrated interface
for finding the requester ID and doing something across all devices with
that requester ID and thereby remove pci_find_upstream_pcie_bridge(),
provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
devices that use the wrong requester ID.  In what case does a device
appear to have multiple requester IDs?  We have cases where devices use
the wrong requester ID, but AFAIK they only use a single wrong requester
ID.  Thanks,

Alex

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

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

[Andrew Cooks]

On Wed, Jul 24, 2013 at 7:21 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2013-07-23 at 16:35 -0600, 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)
>
> How does a device can't have multiple requester IDs?  Reading below, I'm
> not sure we're on the same page for the purpose of this patch.
>
>> > 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.
>
> Hmm, ok.  I figured a request-er makes a request on behalf of a
> request-ee.  Suggestions?
>
>> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
>> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > ---
>> >  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
>> > +    * 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
>> > + * of using (subordinate << 8 | 0) the use (bus << 8 | devfn), like a
>>
>> s/the/they/
>>
>> Did you learn about this empirically?  Intel spec?  I wonder if there's
>> some way to derive this from the PCIe specs.
>
> It's in the current intel-iommu logic, pretty much anywhere it uses
> pci_find_upstream_pcie_bridge() it follows that up with a pci_is_pcie()
> check.  If it's PCIe, it uses a traditional PCIe-to-PCI bridge ID.  If
> it's a legacy PCI bridge it uses the bridge ID itself.
>
> static int
> domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
>                         int translation)
> {
> ...
>         /* dependent device mapping */
>         tmp = pci_find_upstream_pcie_bridge(pdev);
>         if (!tmp)
>                 return 0;
> ...
>         if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
>                 return domain_context_mapping_one(domain,
>                                         pci_domain_nr(tmp->subordinate),
>                                         tmp->subordinate->number, 0,
>                                         translation);
>         else /* this is a legacy PCI bridge */
>                 return domain_context_mapping_one(domain,
>                                                   pci_domain_nr(tmp->bus),
>                                                   tmp->bus->number,
>                                                   tmp->devfn,
>                                                   translation);
>
> The 82801 reference is from looking at when this would actually happen
> on one of my own systems.
>
>
>> > + * 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.
>
> That's why this function is named get_.._requester instead of requester
> ID.  I believe there still has to be a struct pci_dev that does the
> request, but the requester ID for that device may not actually match.
> So I return both.  In a PCIe-to-PCI bridge case, the pci_dev is the
> bridge, but the requester ID is either the bridge bus|devfn or
> subordinate|0 depending on the topology.  If we want to support "ghost
> functions", we can return the real pci_dev and a ghost requester ID.
>
> I think if we used just a requester ID, it ends up being extremely
> difficult to pass that into anything else since we then have to search
> again for where that requester ID is rooted.
>
>> > +{
>> > +   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.
>
>
> Yes, I was just following your RFC lead and didn't want to deal with
> reference counting until this approach had enough traction.
>
>
>> > +                   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
>> caller reads better if it's "...for_each_requester_id()"
>
> Wouldn't you expect to pass a requester ID into a function with that
> name?  I'm pretty sure I had it named that at one point but thought the
> parameters made more sense this way.  I'll see if I can think of a
> better name.
>
>> The "Call X on each devices and DMA source from Y to the requester ID"
>> part doesn't quite make a sentence.
>
>
> Ok
>
>> > + * @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.
>
> I can add something.  @bridge is supposed to be for bridge-based IOMMUs.
> Essentially it's just a stopping point.  There might be PCI topology
> upstream of @bridge, but if you only want the requester ID seen by the
> bridge, you don't care.
>
>> > + * @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.
>
> It's mostly for readability.  I've learned that we never what to look at
> dev->bus->self without first testing pci_is_root_bus(dev->bus), so if I
> was reading this code I'd stumble when I got here and spend too long
> looking around for the assumption that makes it not needed.  I suppose I
> could just make it a comment, but I thought why not make it official w/
> a BUG.
>
>> > +
>> > +           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?
>
> I thought the point of this patch was to have an integrated interface
> for finding the requester ID and doing something across all devices with
> that requester ID and thereby remove pci_find_upstream_pcie_bridge(),
> provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> devices that use the wrong requester ID.  In what case does a device
> appear to have multiple requester IDs?  We have cases where devices use
> the wrong requester ID, but AFAIK they only use a single wrong requester
> ID.  Thanks,
>

The cases I've found where multiple requester IDs were used are all related
to Marvell Sata controllers.

Here's a table of affected devices with links to the
bug reports. In each case both functions 0 and 1 are used.

static const struct pci_dev_dma_multi_source_map {
       u16 vendor;
       u16 device;
} pci_dev_dma_multi_source_map[] = {
        /* Reported by Patrick Bregman
         * https://bugzilla.redhat.com/show_bug.cgi?id=863653 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9120},

       /* Reported by  Paweł Żak, Korneliusz Jarzębski, Daniel Mayer
        * https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
        * Justin Piszcz  https://lkml.org/lkml/2012/11/24/94 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9123},

       /* Reported by Robert Cicconetti
        * https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
        * Fernando https://bugzilla.redhat.com/show_bug.cgi?id=757166 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9128},

       /* Reported by Stijn Tintel
        * https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9130},

       /* Reported by Gaudenz Steinlin
        * https://lkml.org/lkml/2013/3/5/288 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9143},

       /* Reported by Andrew Cooks
        * https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
       { PCI_VENDOR_ID_MARVELL_EXT, 0x9172}
};


> Alex
>
>> > +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
>> >
>
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Alex Williamson]

On Wed, 2013-07-24 at 23:03 +0800, Andrew Cooks wrote:
> On Wed, Jul 24, 2013 at 7:21 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Tue, 2013-07-23 at 16:35 -0600, 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)
> >
> > How does a device can't have multiple requester IDs?  Reading below, I'm
> > not sure we're on the same page for the purpose of this patch.
> >
> >> > 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.
> >
> > Hmm, ok.  I figured a request-er makes a request on behalf of a
> > request-ee.  Suggestions?
> >
> >> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> >> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> > ---
> >> >  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
> >> > +    * 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
> >> > + * of using (subordinate << 8 | 0) the use (bus << 8 | devfn), like a
> >>
> >> s/the/they/
> >>
> >> Did you learn about this empirically?  Intel spec?  I wonder if there's
> >> some way to derive this from the PCIe specs.
> >
> > It's in the current intel-iommu logic, pretty much anywhere it uses
> > pci_find_upstream_pcie_bridge() it follows that up with a pci_is_pcie()
> > check.  If it's PCIe, it uses a traditional PCIe-to-PCI bridge ID.  If
> > it's a legacy PCI bridge it uses the bridge ID itself.
> >
> > static int
> > domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
> >                         int translation)
> > {
> > ...
> >         /* dependent device mapping */
> >         tmp = pci_find_upstream_pcie_bridge(pdev);
> >         if (!tmp)
> >                 return 0;
> > ...
> >         if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
> >                 return domain_context_mapping_one(domain,
> >                                         pci_domain_nr(tmp->subordinate),
> >                                         tmp->subordinate->number, 0,
> >                                         translation);
> >         else /* this is a legacy PCI bridge */
> >                 return domain_context_mapping_one(domain,
> >                                                   pci_domain_nr(tmp->bus),
> >                                                   tmp->bus->number,
> >                                                   tmp->devfn,
> >                                                   translation);
> >
> > The 82801 reference is from looking at when this would actually happen
> > on one of my own systems.
> >
> >
> >> > + * 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.
> >
> > That's why this function is named get_.._requester instead of requester
> > ID.  I believe there still has to be a struct pci_dev that does the
> > request, but the requester ID for that device may not actually match.
> > So I return both.  In a PCIe-to-PCI bridge case, the pci_dev is the
> > bridge, but the requester ID is either the bridge bus|devfn or
> > subordinate|0 depending on the topology.  If we want to support "ghost
> > functions", we can return the real pci_dev and a ghost requester ID.
> >
> > I think if we used just a requester ID, it ends up being extremely
> > difficult to pass that into anything else since we then have to search
> > again for where that requester ID is rooted.
> >
> >> > +{
> >> > +   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.
> >
> >
> > Yes, I was just following your RFC lead and didn't want to deal with
> > reference counting until this approach had enough traction.
> >
> >
> >> > +                   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
> >> caller reads better if it's "...for_each_requester_id()"
> >
> > Wouldn't you expect to pass a requester ID into a function with that
> > name?  I'm pretty sure I had it named that at one point but thought the
> > parameters made more sense this way.  I'll see if I can think of a
> > better name.
> >
> >> The "Call X on each devices and DMA source from Y to the requester ID"
> >> part doesn't quite make a sentence.
> >
> >
> > Ok
> >
> >> > + * @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.
> >
> > I can add something.  @bridge is supposed to be for bridge-based IOMMUs.
> > Essentially it's just a stopping point.  There might be PCI topology
> > upstream of @bridge, but if you only want the requester ID seen by the
> > bridge, you don't care.
> >
> >> > + * @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.
> >
> > It's mostly for readability.  I've learned that we never what to look at
> > dev->bus->self without first testing pci_is_root_bus(dev->bus), so if I
> > was reading this code I'd stumble when I got here and spend too long
> > looking around for the assumption that makes it not needed.  I suppose I
> > could just make it a comment, but I thought why not make it official w/
> > a BUG.
> >
> >> > +
> >> > +           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?
> >
> > I thought the point of this patch was to have an integrated interface
> > for finding the requester ID and doing something across all devices with
> > that requester ID and thereby remove pci_find_upstream_pcie_bridge(),
> > provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> > devices that use the wrong requester ID.  In what case does a device
> > appear to have multiple requester IDs?  We have cases where devices use
> > the wrong requester ID, but AFAIK they only use a single wrong requester
> > ID.  Thanks,
> >
> 
> The cases I've found where multiple requester IDs were used are all related
> to Marvell Sata controllers.
> 
> Here's a table of affected devices with links to the
> bug reports. In each case both functions 0 and 1 are used.

I'm confused.  There's only one requester ID for a given transaction.
Therefore on a per transaction level there's a 1:1 relationship between
a requester ID and a device.  The requester ID may be incorrect, but
there cannot be more than one per transaction.  Are you suggesting that
a given device (PCI function) will alternate using one requester ID for
some transactions and a different requester ID for others?  I'm thinking
of cases like this bz:

https://bugzilla.redhat.com/show_bug.cgi?id=757166#c16

In that case pci_get_visible_pcie_requester() (assuming no bridges are
involved) should return the device and the requester ID that it would
use if it worked correctly.  pcie_for_each_requester() should then call
the callback function using both the correct requester ID and the
quirked requester ID.  The reason I mentioned that this doesn't
currently handle ghost functions is because it uses the
pci_get_dma_source() quirk, which only handles actual devices.  We'd
need to add a pci_get_dma_alias() or something that would give us a list
of ghost requester IDs to handle these broken marvel devices.  Thanks,

Alex

> static const struct pci_dev_dma_multi_source_map {
>        u16 vendor;
>        u16 device;
> } pci_dev_dma_multi_source_map[] = {
>         /* Reported by Patrick Bregman
>          * https://bugzilla.redhat.com/show_bug.cgi?id=863653 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9120},
> 
>        /* Reported by  Paweł Żak, Korneliusz Jarzębski, Daniel Mayer
>         * https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
>         * Justin Piszcz  https://lkml.org/lkml/2012/11/24/94 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9123},
> 
>        /* Reported by Robert Cicconetti
>         * https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
>         * Fernando https://bugzilla.redhat.com/show_bug.cgi?id=757166 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9128},
> 
>        /* Reported by Stijn Tintel
>         * https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9130},
> 
>        /* Reported by Gaudenz Steinlin
>         * https://lkml.org/lkml/2013/3/5/288 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9143},
> 
>        /* Reported by Andrew Cooks
>         * https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
>        { PCI_VENDOR_ID_MARVELL_EXT, 0x9172}
> };
> 
> 
> > Alex
> >
> >> > +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
> >> >
> >
> >
> >



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Bjorn Helgaas]

On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 2013-07-23 at 16:35 -0600, 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)
>
> How does a device can't have multiple requester IDs?  Reading below, I'm
> not sure we're on the same page for the purpose of this patch.

>> "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.
>
> Hmm, ok.  I figured a request-er makes a request on behalf of a
> request-ee.  Suggestions?

I would expect a request-er to make a request *to* a request-ee, just
like a grant-or makes a grant to a grant-ee.  My suggestion is to use
"requester" consistently for only the originator of a DMA transaction.
 Any devices above it are by definition "bridges".  As the DMA
transaction propagates through the fabric, it may be tagged by bridges
with different requester IDs.

The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
not sure the intermediate pci_devs are.

>> > + * 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.
>
> That's why this function is named get_.._requester instead of requester
> ID.  I believe there still has to be a struct pci_dev that does the
> request, but the requester ID for that device may not actually match.
> So I return both.  In a PCIe-to-PCI bridge case, the pci_dev is the
> bridge, but the requester ID is either the bridge bus|devfn or
> subordinate|0 depending on the topology.  If we want to support "ghost
> functions", we can return the real pci_dev and a ghost requester ID.
>
> I think if we used just a requester ID, it ends up being extremely
> difficult to pass that into anything else since we then have to search
> again for where that requester ID is rooted.

Returning both a pci_dev and a requester ID makes it more complicated.
 At the hardware level, transactions contain only a requester ID, and
bridges can either drop it, pass it unchanged, or assign a new one.  I
think the code will be simpler if we just model that.

>> > + * 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
>> caller reads better if it's "...for_each_requester_id()"
>
> Wouldn't you expect to pass a requester ID into a function with that
> name?  I'm pretty sure I had it named that at one point but thought the
> parameters made more sense this way.  I'll see if I can think of a
> better name.

My thought was to pass a pci_dev (the originator of the DMA, which I
would call the "requester") and a callback.  The callback would accept
the requester pci_dev (always the same requester device) and a
requester ID.

This would call @fn for each possible requester ID for transactions
from the device.  IOMMU drivers should only need the requester ID to
manage mappings; they shouldn't need a pci_dev corresponding to any
intermediate bridges.

>> > +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?
>
> I thought the point of this patch was to have an integrated interface
> for finding the requester ID and doing something across all devices with
> that requester ID

Oh, here's the difference in our understanding.  "Doing something
across all devices with that requester ID" sounds like identifying the
set of devices that have to be handled as a group by the IOMMU.
That's certainly an issue, but I wasn't considering it at all.

I was only concerned with the question of a single device that
requires multiple IOMMU mappings because DMA requests might use any of
several source IDs.  This is mentioned in sec 3.6.1.1 of the VT-d
spec, i.e., "requests arriving with the source-id in the original
PCI-X transaction or the source-id provided by the bridge. ...
software must program multiple context entries, each corresponding to
the possible set of source-ids."

My thought was that the IOMMU driver would use
pcie_for_each_requester_id() and have the callback set up a mapping
for one requester ID each time it was called.

> and thereby remove pci_find_upstream_pcie_bridge(),
> provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> devices that use the wrong requester ID.  In what case does a device
> appear to have multiple requester IDs?  We have cases where devices use
> the wrong requester ID, but AFAIK they only use a single wrong requester
> ID.

I think the example in sec 3.6.1.1 answers this, doesn't it?

Bjorn

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

[Alex Williamson]

On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 2013-07-23 at 16:35 -0600, 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)
> >
> > How does a device can't have multiple requester IDs?  Reading below, I'm
> > not sure we're on the same page for the purpose of this patch.
> 
> >> "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.
> >
> > Hmm, ok.  I figured a request-er makes a request on behalf of a
> > request-ee.  Suggestions?
> 
> I would expect a request-er to make a request *to* a request-ee, just
> like a grant-or makes a grant to a grant-ee.  My suggestion is to use
> "requester" consistently for only the originator of a DMA transaction.
>  Any devices above it are by definition "bridges".

But there may not be a bridge, so are we then creating just a revised
pci_find_upstream_pcie_bridge() function?  I think we want to abstract
away caring whether there's a bridge or a well behaved device or a
broken device that requires quirks.

>   As the DMA
> transaction propagates through the fabric, it may be tagged by bridges
> with different requester IDs.
> 
> The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> not sure the intermediate pci_devs are.

A u16 requester ID doesn't mean much on it's own though, it's not
necessarily even unique.  A requester ID associated with the context of
a pci_dev is unique and gives us a reference point if we need to perform
another operation on that requester ID.

> >> > + * 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.
> >
> > That's why this function is named get_.._requester instead of requester
> > ID.  I believe there still has to be a struct pci_dev that does the
> > request, but the requester ID for that device may not actually match.
> > So I return both.  In a PCIe-to-PCI bridge case, the pci_dev is the
> > bridge, but the requester ID is either the bridge bus|devfn or
> > subordinate|0 depending on the topology.  If we want to support "ghost
> > functions", we can return the real pci_dev and a ghost requester ID.
> >
> > I think if we used just a requester ID, it ends up being extremely
> > difficult to pass that into anything else since we then have to search
> > again for where that requester ID is rooted.
> 
> Returning both a pci_dev and a requester ID makes it more complicated.
>  At the hardware level, transactions contain only a requester ID, and
> bridges can either drop it, pass it unchanged, or assign a new one.  I
> think the code will be simpler if we just model that.

I'm not convinced.  Patch 2/2 makes use of both the returned pci_dev and
the returned requester ID and it's a huge simplification overall.

> >> > + * 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
> >> caller reads better if it's "...for_each_requester_id()"
> >
> > Wouldn't you expect to pass a requester ID into a function with that
> > name?  I'm pretty sure I had it named that at one point but thought the
> > parameters made more sense this way.  I'll see if I can think of a
> > better name.
> 
> My thought was to pass a pci_dev (the originator of the DMA, which I
> would call the "requester") and a callback.  The callback would accept
> the requester pci_dev (always the same requester device) and a
> requester ID.
> 
> This would call @fn for each possible requester ID for transactions
> from the device.  IOMMU drivers should only need the requester ID to
> manage mappings; they shouldn't need a pci_dev corresponding to any
> intermediate bridges.

This implementation is almost the same with the only change being that
the pci_dev passed to the callback is the one most closely associated
with the requester ID.  For the IOMMU driver, it doesn't matter since
it's only using the requester ID, but why would the callback care about
the original requester?  If it needed to do something device specific,
it's going to care about the closest device to the requester ID.

> >> > +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?
> >
> > I thought the point of this patch was to have an integrated interface
> > for finding the requester ID and doing something across all devices with
> > that requester ID
> 
> Oh, here's the difference in our understanding.  "Doing something
> across all devices with that requester ID" sounds like identifying the
> set of devices that have to be handled as a group by the IOMMU.
> That's certainly an issue, but I wasn't considering it at all.
> 
> I was only concerned with the question of a single device that
> requires multiple IOMMU mappings because DMA requests might use any of
> several source IDs.  This is mentioned in sec 3.6.1.1 of the VT-d
> spec, i.e., "requests arriving with the source-id in the original
> PCI-X transaction or the source-id provided by the bridge. ...
> software must program multiple context entries, each corresponding to
> the possible set of source-ids."
> 
> My thought was that the IOMMU driver would use
> pcie_for_each_requester_id() and have the callback set up a mapping
> for one requester ID each time it was called.

Which is exactly what pcie_for_each_requester() does, but I try to solve
the problem of a single device that needs multiple context entries the
same as a bridge that masks multiple devices.

> > and thereby remove pci_find_upstream_pcie_bridge(),
> > provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> > devices that use the wrong requester ID.  In what case does a device
> > appear to have multiple requester IDs?  We have cases where devices use
> > the wrong requester ID, but AFAIK they only use a single wrong requester
> > ID.
> 
> I think the example in sec 3.6.1.1 answers this, doesn't it?

It does, in summary we have to be prepared that a transaction from a
device behind a PCIe-to-PCI/X bridge may use either the device requester
ID (bus|devfn) or the bridge (subordinate|0) requester ID.  This is why
pcie_for_each_requester() makes sense to me, we iterate across all
requesters that may use the same requester ID.  If we had a
pcie_for_each_requester_id() how would we handle that?  Do we call it
once for the (bus|devfn) requester ID of the device we're trying to map
and then call it again for the (subordinate|0) requester ID of the
bridge?  That's almost how pci_find_upstream_pcie_bridge() is used and
it ends up putting a lot of the logic into the driver.  How do we write
a pcie_for_each_requester_id() function that can uniquely identify a PCI
hierarchy to traverse in the presences of multiple domains?  I have a
hard time seeing how we get away from returning a {pci_dev, requester
ID} pair if we want to be descriptive.  Maybe we create a:

struct pci_requester_id {
	struct pci_dev *dev;
	u16 id;
}

and return that if this is a matter of semantics.  Thanks,

Alex

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

[Bjorn Helgaas]

On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
> On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> > On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > > On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
> > >> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> >   As the DMA
> > transaction propagates through the fabric, it may be tagged by bridges
> > with different requester IDs.
> > 
> > The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> > not sure the intermediate pci_devs are.
> 
> A u16 requester ID doesn't mean much on it's own though, it's not
> necessarily even unique.  A requester ID associated with the context of
> a pci_dev is unique and gives us a reference point if we need to perform
> another operation on that requester ID.

A u16 requester ID better mean something to an IOMMU -- it's all the
IOMMU can use to look up the correct mapping.  That's why we have to
give the iterator something to define the scope to iterate over.  The
same requester ID could mean something totally unrelated in a
different scope, e.g., below a different IOMMU.

> > Returning both a pci_dev and a requester ID makes it more complicated.
> >  At the hardware level, transactions contain only a requester ID, and
> > bridges can either drop it, pass it unchanged, or assign a new one.  I
> > think the code will be simpler if we just model that.
> 
> I'm not convinced.  Patch 2/2 makes use of both the returned pci_dev and
> the returned requester ID and it's a huge simplification overall.

The IOMMU driver makes mappings so DMA from device A can reach a
buffer.  It needs to know about device A, and it needs to know what
source IDs might appear on those DMA transactions.  Why would it need
to know anything about any bridges between A and the IOMMU?

> > My thought was to pass a pci_dev (the originator of the DMA, which I
> > would call the "requester") and a callback.  The callback would accept
> > the requester pci_dev (always the same requester device) and a
> > requester ID.
> > 
> > This would call @fn for each possible requester ID for transactions
> > from the device.  IOMMU drivers should only need the requester ID to
> > manage mappings; they shouldn't need a pci_dev corresponding to any
> > intermediate bridges.
> 
> This implementation is almost the same with the only change being that
> the pci_dev passed to the callback is the one most closely associated
> with the requester ID.  For the IOMMU driver, it doesn't matter since
> it's only using the requester ID, but why would the callback care about
> the original requester?  If it needed to do something device specific,
> it's going to care about the closest device to the requester ID.

If this can be done without passing a pci_dev at all to the callback,
that would be even better.  If a caller needed the original pci_dev,
it could always pass it via the "void *data".

I don't see the point of passing a "device closest to the requester
ID."  What would the IOMMU do with that?  As far as the IOMMU is
concerned, the requester ID could be an arbitrary number completely
unrelated to a pci_dev.

> > >> > +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?
> > >
> > > I thought the point of this patch was to have an integrated interface
> > > for finding the requester ID and doing something across all devices with
> > > that requester ID
> > 
> > Oh, here's the difference in our understanding.  "Doing something
> > across all devices with that requester ID" sounds like identifying the
> > set of devices that have to be handled as a group by the IOMMU.
> > That's certainly an issue, but I wasn't considering it at all.
> > 
> > I was only concerned with the question of a single device that
> > requires multiple IOMMU mappings because DMA requests might use any of
> > several source IDs.  This is mentioned in sec 3.6.1.1 of the VT-d
> > spec, i.e., "requests arriving with the source-id in the original
> > PCI-X transaction or the source-id provided by the bridge. ...
> > software must program multiple context entries, each corresponding to
> > the possible set of source-ids."
> > 
> > My thought was that the IOMMU driver would use
> > pcie_for_each_requester_id() and have the callback set up a mapping
> > for one requester ID each time it was called.
> 
> Which is exactly what pcie_for_each_requester() does, but I try to solve
> the problem of a single device that needs multiple context entries the
> same as a bridge that masks multiple devices.

I'm not sure I understand your point yet, but I think it's better to
think of this as computing the set of requester IDs we might see from
a given device.  If we think of it as finding the set of devices that
use a given requester ID, then we have to worry about that set being
modified by hotplug.

> > > and thereby remove pci_find_upstream_pcie_bridge(),
> > > provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> > > devices that use the wrong requester ID.  In what case does a device
> > > appear to have multiple requester IDs?  We have cases where devices use
> > > the wrong requester ID, but AFAIK they only use a single wrong requester
> > > ID.
> > 
> > I think the example in sec 3.6.1.1 answers this, doesn't it?
> 
> It does, in summary we have to be prepared that a transaction from a
> device behind a PCIe-to-PCI/X bridge may use either the device requester
> ID (bus|devfn) or the bridge (subordinate|0) requester ID.  This is why
> pcie_for_each_requester() makes sense to me, we iterate across all
> requesters that may use the same requester ID.

Let's say device A and B use the same requester ID because they're
behind a bridge.  What happens if we map a buffer for A and then
hot-remove B?  Here's what I think your proposal would do:

  map buffer for A
    callback for A
    callback for B
  hot-remove B
  unmap buffer for A
    callback for A

We never do an unmap callback for B, so if we did anything while
mapping, it will never be undone.

> If we had a
> pcie_for_each_requester_id() how would we handle that?  Do we call it
> once for the (bus|devfn) requester ID of the device we're trying to map
> and then call it again for the (subordinate|0) requester ID of the
> bridge?  That's almost how pci_find_upstream_pcie_bridge() is used and
> it ends up putting a lot of the logic into the driver.  

That *is* what I'm proposing (calling the callback once for each
possible requester ID that could be seen on DMA from the device).  In
my opinion, the problem with pci_find_upstream_pcie_bridge() is that
you only get one device back from it, and the driver has to roll its
own looping.

> How do we write
> a pcie_for_each_requester_id() function that can uniquely identify a PCI
> hierarchy to traverse in the presences of multiple domains?  

I must be missing something in your question.  If we give
pcie_for_each_requester_id() the requester pci_dev, that uniquely
identifies the PCI hierarchy.  Then it's just a question of how far up
the hierarchy we need to go, and the bridge (or bus) argument tells us
that.

Bjorn

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

[Alex Williamson]

On Wed, 2013-07-24 at 17:24 -0600, Bjorn Helgaas wrote:
> On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
> > On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> > > On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> > > <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > > On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
> > > >> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> > >   As the DMA
> > > transaction propagates through the fabric, it may be tagged by bridges
> > > with different requester IDs.
> > > 
> > > The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> > > not sure the intermediate pci_devs are.
> > 
> > A u16 requester ID doesn't mean much on it's own though, it's not
> > necessarily even unique.  A requester ID associated with the context of
> > a pci_dev is unique and gives us a reference point if we need to perform
> > another operation on that requester ID.
> 
> A u16 requester ID better mean something to an IOMMU -- it's all the
> IOMMU can use to look up the correct mapping.  That's why we have to
> give the iterator something to define the scope to iterate over.  The
> same requester ID could mean something totally unrelated in a
> different scope, e.g., below a different IOMMU.

The point I'm trying to make is that a requester ID depends on it's
context (minimally, the PCI segment).  The caller can assume the context
based on the calling parameters or we can provide context in the form of
an associated pci_dev.  I chose the latter path because I prefer
explicit interfaces and it has some usefulness in the intel-iommu
implementation.

For instance, get_domain_for_dev() first looks to see if a pci_dev
already has a domain.  If it doesn't, we want to look to see if there's
an upstream device that would use the same requester ID that already has
a domain.  If our get-requester-ID-for-device function returns only the
requester ID, we don't know if that requester ID is the device we're
searching from or some upstream device.  Therefore we potentially do an
unnecessary search for the domain.

The other user is intel_iommu_add_device() where we're trying to build
IOMMU groups.  Visibility is the first requirement of an IOMMU group.
If the IOMMU cannot distinguish between devices, they must be part of
the same IOMMU group.  Here we want to find the pci_dev that hosts the
requester ID.  I don't even know how we'd implement this with a function
that only returned the requester ID.  Perhaps we'd have to walk upstream
from the device calling the get-requester-ID-for-device function at each
step and noticing when it changed.  That moves significant logic back
into the caller code.

> > > Returning both a pci_dev and a requester ID makes it more complicated.
> > >  At the hardware level, transactions contain only a requester ID, and
> > > bridges can either drop it, pass it unchanged, or assign a new one.  I
> > > think the code will be simpler if we just model that.
> > 
> > I'm not convinced.  Patch 2/2 makes use of both the returned pci_dev and
> > the returned requester ID and it's a huge simplification overall.
> 
> The IOMMU driver makes mappings so DMA from device A can reach a
> buffer.  It needs to know about device A, and it needs to know what
> source IDs might appear on those DMA transactions.  Why would it need
> to know anything about any bridges between A and the IOMMU?

Maybe it doesn't, but it might want to know about the top level bridge
for the examples above and I prefer that we be consistent in providing
both a requester ID and context device.  It's not currently used in the
callback function, but I could imagine it could be.  For instance, what
if there are multiple endpoints downstream of a bridge and the IOMMU
wants to use the bridge pci_dev to track how many routes it's managing
through this bridge.

I think the latter is actually a current bug in intel-iommu as noted by
the XXX in patch 2/2.  iommu_detach_dependent_devices() will tear down
all context entries along a path, but there's no reference counting as
to how many devices are using that path.  Should that reference counting
be associated with the pci_dev?  I suppose the IOMMU could already have
data structures per requester ID or could create them and use an idr for
lookup, but does this interface want to require that?

> > > My thought was to pass a pci_dev (the originator of the DMA, which I
> > > would call the "requester") and a callback.  The callback would accept
> > > the requester pci_dev (always the same requester device) and a
> > > requester ID.
> > > 
> > > This would call @fn for each possible requester ID for transactions
> > > from the device.  IOMMU drivers should only need the requester ID to
> > > manage mappings; they shouldn't need a pci_dev corresponding to any
> > > intermediate bridges.
> > 
> > This implementation is almost the same with the only change being that
> > the pci_dev passed to the callback is the one most closely associated
> > with the requester ID.  For the IOMMU driver, it doesn't matter since
> > it's only using the requester ID, but why would the callback care about
> > the original requester?  If it needed to do something device specific,
> > it's going to care about the closest device to the requester ID.
> 
> If this can be done without passing a pci_dev at all to the callback,
> that would be even better.  If a caller needed the original pci_dev,
> it could always pass it via the "void *data".

Agreed, the original "device A" is the caller's problem if it wants to
know about it, but I think providing full context of a requester ID
(which means a pci_dev) makes sense too.

> I don't see the point of passing a "device closest to the requester
> ID."  What would the IOMMU do with that?  As far as the IOMMU is
> concerned, the requester ID could be an arbitrary number completely
> unrelated to a pci_dev.

Do you have an example of a case where a requester ID doesn't have some
association to a pci_dev?

> > > >> > +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?
> > > >
> > > > I thought the point of this patch was to have an integrated interface
> > > > for finding the requester ID and doing something across all devices with
> > > > that requester ID
> > > 
> > > Oh, here's the difference in our understanding.  "Doing something
> > > across all devices with that requester ID" sounds like identifying the
> > > set of devices that have to be handled as a group by the IOMMU.
> > > That's certainly an issue, but I wasn't considering it at all.
> > > 
> > > I was only concerned with the question of a single device that
> > > requires multiple IOMMU mappings because DMA requests might use any of
> > > several source IDs.  This is mentioned in sec 3.6.1.1 of the VT-d
> > > spec, i.e., "requests arriving with the source-id in the original
> > > PCI-X transaction or the source-id provided by the bridge. ...
> > > software must program multiple context entries, each corresponding to
> > > the possible set of source-ids."
> > > 
> > > My thought was that the IOMMU driver would use
> > > pcie_for_each_requester_id() and have the callback set up a mapping
> > > for one requester ID each time it was called.
> > 
> > Which is exactly what pcie_for_each_requester() does, but I try to solve
> > the problem of a single device that needs multiple context entries the
> > same as a bridge that masks multiple devices.
> 
> I'm not sure I understand your point yet, but I think it's better to
> think of this as computing the set of requester IDs we might see from
> a given device.  If we think of it as finding the set of devices that
> use a given requester ID, then we have to worry about that set being
> modified by hotplug.

Perhaps I'm explain it wrong, I've had to refresh myself on how this all
works since I posted it.  Looking at pcie_for_each_requester() we first
find the requester ID and associated device for the requested device
(yet another users of the associated device part of the interface).  We
then walk up from the requested device to the associated device, calling
the callback on each bridge or dma quirk in between.  So, I was wrong to
indicate that pcie_for_each_requester() does anything across all devices
with the same requester ID.  It traverses all devices and quirks of
devices along the path from the requested device to the device
associated with the requester ID.

> > > > and thereby remove pci_find_upstream_pcie_bridge(),
> > > > provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> > > > devices that use the wrong requester ID.  In what case does a device
> > > > appear to have multiple requester IDs?  We have cases where devices use
> > > > the wrong requester ID, but AFAIK they only use a single wrong requester
> > > > ID.
> > > 
> > > I think the example in sec 3.6.1.1 answers this, doesn't it?
> > 
> > It does, in summary we have to be prepared that a transaction from a
> > device behind a PCIe-to-PCI/X bridge may use either the device requester
> > ID (bus|devfn) or the bridge (subordinate|0) requester ID.  This is why
> > pcie_for_each_requester() makes sense to me, we iterate across all
> > requesters that may use the same requester ID.
> 
> Let's say device A and B use the same requester ID because they're
> behind a bridge.  What happens if we map a buffer for A and then
> hot-remove B?  Here's what I think your proposal would do:
> 
>   map buffer for A
>     callback for A
>     callback for B
>   hot-remove B
>   unmap buffer for A
>     callback for A
> 
> We never do an unmap callback for B, so if we did anything while
> mapping, it will never be undone.

So I think I've again lost my context since I posted this, apologies.
pcie_for_each_requester() is implemented to handle the path, not the
subtree.  I believe what pcie_for_each_requester() as written would do
is:

   map buffer for A
     callback for A
   hot-remove B
   unmap buffer for A
     callback for A

Furthermore, if we have:

     -- A
    /
X--Y
    \
     -- B

Where X is the associated device for the requester ID, we'd do:

  map buffer for A
    callback for A
    callback for Y
    callback for X
  hot-remove B
  unmap buffer for A
    callback for A
    callback for Y
    callback for X

That hot-remove though is where I think intel-iommu has a bug as the set
of dependent devices for B includes Y & X, which would then have their
context entries cleared.  That's yet another intel-iommu bug though, not
anything that requires a change to this interface.

> > If we had a
> > pcie_for_each_requester_id() how would we handle that?  Do we call it
> > once for the (bus|devfn) requester ID of the device we're trying to map
> > and then call it again for the (subordinate|0) requester ID of the
> > bridge?  That's almost how pci_find_upstream_pcie_bridge() is used and
> > it ends up putting a lot of the logic into the driver.  
> 
> That *is* what I'm proposing (calling the callback once for each
> possible requester ID that could be seen on DMA from the device).  In
> my opinion, the problem with pci_find_upstream_pcie_bridge() is that
> you only get one device back from it, and the driver has to roll its
> own looping.

I'm not sure if my argument above is still relevant, but what I was
trying to suggest was that the caller would need to make multiple calls,
which puts us in the same boat as the roll-your-own loop with
find_upstream_pcie_bridge.  Of course the callback needs to be called
for each requester ID.

Let me go back to the X-Y-A|B example above to see if I can explain why
pcie_for_each_requester_id() doesn't make sense to me.  Generally a
for_each_foo function should iterate across all things with the same
foo.  So a for_each_requester_id should iterate across all things with
the same requester ID.  If we look at an endpoint device like A, only A
has A's requester ID.  Therefore, why would for_each_requester_id(A)
traverse up to Y?  Y can take ownership and become the requester for A,
but if we were to call for_each_requester_id(Y), wouldn't you expect the
callback to happen on {Y, A, B} since all of those can use that
requester ID?  That makes it a downstream looking for_each callback
which is not really want we want.

As you indicate above, we want an upstream looking function.  If I call
for_each_requester(A), A is obviously a requester for A.  Y can take
ownership of a transaction from A, therefore we must assume Y is a
requester for A.  Same with X.  I believe this is what we want and this
is what the code does.

> > How do we write
> > a pcie_for_each_requester_id() function that can uniquely identify a PCI
> > hierarchy to traverse in the presences of multiple domains?  
> 
> I must be missing something in your question.  If we give
> pcie_for_each_requester_id() the requester pci_dev, that uniquely
> identifies the PCI hierarchy.  Then it's just a question of how far up
> the hierarchy we need to go, and the bridge (or bus) argument tells us
> that.

I think the code actually does what you want, so we're only in
disagreement about the naming and usefulness (appropriate-ness) of
passing a pci_dev associated with a requester ID.  Is that correct?

There are several examples above about the associated device being
useful to the implementation.  It may not be required, but it's only
going to make the caller implement more code on their side not to
provide it.  The for_each_requester_id vs for_each_requester is a matter
of personal taste, but I think for_each_requester actually makes more
sense given the parameters and the upstream vs downstream nature of the
call.  Sorry again for the confusion, I can barely identify my own code
after a couple weeks.  Thanks,

Alex

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

[Bjorn Helgaas]

On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
> On Wed, 2013-07-24 at 17:24 -0600, Bjorn Helgaas wrote:
> > On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
> > > On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> > > > On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> > > > <alex.williamson@redhat.com> wrote:
> > > > > On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
> > > > >> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> > > >   As the DMA
> > > > transaction propagates through the fabric, it may be tagged by bridges
> > > > with different requester IDs.
> > > > 
> > > > The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> > > > not sure the intermediate pci_devs are.
> > > 
> > > A u16 requester ID doesn't mean much on it's own though, it's not
> > > necessarily even unique.  A requester ID associated with the context of
> > > a pci_dev is unique and gives us a reference point if we need to perform
> > > another operation on that requester ID.
> > 
> > A u16 requester ID better mean something to an IOMMU -- it's all the
> > IOMMU can use to look up the correct mapping.  That's why we have to
> > give the iterator something to define the scope to iterate over.  The
> > same requester ID could mean something totally unrelated in a
> > different scope, e.g., below a different IOMMU.
> 
> The point I'm trying to make is that a requester ID depends on it's
> context (minimally, the PCI segment).  The caller can assume the context
> based on the calling parameters or we can provide context in the form of
> an associated pci_dev.  I chose the latter path because I prefer
> explicit interfaces and it has some usefulness in the intel-iommu
> implementation.
> 
> For instance, get_domain_for_dev() first looks to see if a pci_dev
> already has a domain.  If it doesn't, we want to look to see if there's
> an upstream device that would use the same requester ID that already has
> a domain.  If our get-requester-ID-for-device function returns only the
> requester ID, we don't know if that requester ID is the device we're
> searching from or some upstream device.  Therefore we potentially do an
> unnecessary search for the domain.
> 
> The other user is intel_iommu_add_device() where we're trying to build
> IOMMU groups.  Visibility is the first requirement of an IOMMU group.
> If the IOMMU cannot distinguish between devices, they must be part of
> the same IOMMU group.  Here we want to find the pci_dev that hosts the
> requester ID.  I don't even know how we'd implement this with a function
> that only returned the requester ID.  Perhaps we'd have to walk upstream
> from the device calling the get-requester-ID-for-device function at each
> step and noticing when it changed.  That moves significant logic back
> into the caller code.
> ...

> > I don't see the point of passing a "device closest to the requester
> > ID."  What would the IOMMU do with that?  As far as the IOMMU is
> > concerned, the requester ID could be an arbitrary number completely
> > unrelated to a pci_dev.
> 
> Do you have an example of a case where a requester ID doesn't have some
> association to a pci_dev?

I think our confusion here is the same as what Don & I have been
hashing out -- I'm saying a requester ID fabricated by a bridge
need not correspond to a specific pci_dev, and you probably mean
that every requester ID is by definition the result of *some* PCI
device making a DMA request.

> ...
> Furthermore, if we have:
> 
>      -- A
>     /
> X--Y
>     \
>      -- B
> ... 

> Let me go back to the X-Y-A|B example above to see if I can explain why
> pcie_for_each_requester_id() doesn't make sense to me.  Generally a
> for_each_foo function should iterate across all things with the same
> foo.  So a for_each_requester_id should iterate across all things with
> the same requester ID.  

Hm, that's not the way I think of for_each_FOO() interfaces.  I
think of it as "execute the body (or callback) for every possible
FOO", where FOO is different each time.  for_each_pci_dev(),
pci_bus_for_each_resource(), for_each_zone(), for_each_cpu(), etc.,
work like that.

But the more important question is what arguments we give to the
callback.  My proposal was to map

  {pci_dev -> {requester-ID-A, requester-ID-B, ...}}

Yours is to map

  {pci_dev -> {{pci_dev-A, requester-ID-A}, {pci_dev-B, requester-ID-B}, ...}}

i.e., your callback gets both a pci_dev and a requester-ID.  I'm
trying to figure out how you handle cases where requester-id-A
doesn't have a corresponding pci_dev-A.  I guess you pass the device
most closely associated with requester-id-A

The "most closely associated device" idea seems confusing to
describe and think about.  I think you want to use it as part of
grouping devices into domains.  But I think we should attack that
problem separately.  For grouping or isolation questions, we have
to pay attention to things like ACS, which are not relevant for
mapping.

> If we look at an endpoint device like A, only A
> has A's requester ID.  Therefore, why would for_each_requester_id(A)
> traverse up to Y?

Even if A is a PCIe device, we have to traverse upwards to find any
bridges that might drop A's requester ID or take ownership, e.g., if
we have this:

  00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
  01:00.0 PCI-to-PCIe bridge to [bus 02]
  02:00.0 PCIe endpoint A

the IOMMU has to look for requester-ID 0100.

> Y can take ownership and become the requester for A,
> but if we were to call for_each_requester_id(Y), wouldn't you expect the
> callback to happen on {Y, A, B} since all of those can use that
> requester ID?

No.  If I call for_each_requester_id(Y), I'm expecting the callback
to happen for each requester ID that could be used for transactions
originated by *Y*.  I'm trying to make an IOMMU mapping for use by
Y, so any devices downstream of Y, e.g., A and B, are irrelevant.

I think a lot of the confusion here is because we're trying to solve
both two questions at once: (1) what requester-IDs need to be mapped
to handle DMA from a device, and (2) what devices can't be isolated
from each other and must be in the same domain.  I think things will
be simpler if we can deal with those separately.  Even if it ends up
being more code, at least each piece will be easier to understand by
itself.

Bjorn

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

[Alex Williamson]

On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
> > On Wed, 2013-07-24 at 17:24 -0600, Bjorn Helgaas wrote:
> > > On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
> > > > On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> > > > > On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> > > > > <alex.williamson@redhat.com> wrote:
> > > > > > On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
> > > > > >> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> > > > >   As the DMA
> > > > > transaction propagates through the fabric, it may be tagged by bridges
> > > > > with different requester IDs.
> > > > > 
> > > > > The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> > > > > not sure the intermediate pci_devs are.
> > > > 
> > > > A u16 requester ID doesn't mean much on it's own though, it's not
> > > > necessarily even unique.  A requester ID associated with the context of
> > > > a pci_dev is unique and gives us a reference point if we need to perform
> > > > another operation on that requester ID.
> > > 
> > > A u16 requester ID better mean something to an IOMMU -- it's all the
> > > IOMMU can use to look up the correct mapping.  That's why we have to
> > > give the iterator something to define the scope to iterate over.  The
> > > same requester ID could mean something totally unrelated in a
> > > different scope, e.g., below a different IOMMU.
> > 
> > The point I'm trying to make is that a requester ID depends on it's
> > context (minimally, the PCI segment).  The caller can assume the context
> > based on the calling parameters or we can provide context in the form of
> > an associated pci_dev.  I chose the latter path because I prefer
> > explicit interfaces and it has some usefulness in the intel-iommu
> > implementation.
> > 
> > For instance, get_domain_for_dev() first looks to see if a pci_dev
> > already has a domain.  If it doesn't, we want to look to see if there's
> > an upstream device that would use the same requester ID that already has
> > a domain.  If our get-requester-ID-for-device function returns only the
> > requester ID, we don't know if that requester ID is the device we're
> > searching from or some upstream device.  Therefore we potentially do an
> > unnecessary search for the domain.
> > 
> > The other user is intel_iommu_add_device() where we're trying to build
> > IOMMU groups.  Visibility is the first requirement of an IOMMU group.
> > If the IOMMU cannot distinguish between devices, they must be part of
> > the same IOMMU group.  Here we want to find the pci_dev that hosts the
> > requester ID.  I don't even know how we'd implement this with a function
> > that only returned the requester ID.  Perhaps we'd have to walk upstream
> > from the device calling the get-requester-ID-for-device function at each
> > step and noticing when it changed.  That moves significant logic back
> > into the caller code.
> > ...
> 
> > > I don't see the point of passing a "device closest to the requester
> > > ID."  What would the IOMMU do with that?  As far as the IOMMU is
> > > concerned, the requester ID could be an arbitrary number completely
> > > unrelated to a pci_dev.
> > 
> > Do you have an example of a case where a requester ID doesn't have some
> > association to a pci_dev?
> 
> I think our confusion here is the same as what Don & I have been
> hashing out -- I'm saying a requester ID fabricated by a bridge
> need not correspond to a specific pci_dev, and you probably mean
> that every requester ID is by definition the result of *some* PCI
> device making a DMA request.

Yes

> > ...
> > Furthermore, if we have:
> > 
> >      -- A
> >     /
> > X--Y
> >     \
> >      -- B
> > ... 
> 
> > Let me go back to the X-Y-A|B example above to see if I can explain why
> > pcie_for_each_requester_id() doesn't make sense to me.  Generally a
> > for_each_foo function should iterate across all things with the same
> > foo.  So a for_each_requester_id should iterate across all things with
> > the same requester ID.  
> 
> Hm, that's not the way I think of for_each_FOO() interfaces.  I
> think of it as "execute the body (or callback) for every possible
> FOO", where FOO is different each time.  for_each_pci_dev(),
> pci_bus_for_each_resource(), for_each_zone(), for_each_cpu(), etc.,
> work like that.

Most of these aren't relevant because they iterate over everything.  I
think they only show that if we had a pci_for_each_requester_id() that
it should iterate over every possible PCI requester ID in the system and
the same could be said for pci_for_each_requester().

pci_bus_for_each_resource() is the only one we can build from; for all
resources on a bus.  We want all requester IDs for a pci_dev.  Does that
perhaps mean it should be called pci_dev_for_each_requester_id()?  I'd
be ok with that name, but I think it even more implies that a pci_dev is
associated with a requester ID.

> But the more important question is what arguments we give to the
> callback.  My proposal was to map
> 
>   {pci_dev -> {requester-ID-A, requester-ID-B, ...}}
> 
> Yours is to map
> 
>   {pci_dev -> {{pci_dev-A, requester-ID-A}, {pci_dev-B, requester-ID-B}, ...}}
> 
> i.e., your callback gets both a pci_dev and a requester-ID.  I'm
> trying to figure out how you handle cases where requester-id-A
> doesn't have a corresponding pci_dev-A.  I guess you pass the device
> most closely associated with requester-id-A

Yes

> The "most closely associated device" idea seems confusing to
> describe and think about.  I think you want to use it as part of
> grouping devices into domains.  But I think we should attack that
> problem separately.  For grouping or isolation questions, we have
> to pay attention to things like ACS, which are not relevant for
> mapping.

We are only touch isolation insofar as providing an interface for a
driver to determine the point in the PCI topology where the requester ID
is rooted.  Yes, grouping can make use of that, but I object to the idea
that we're approaching some slippery slope of rolling multiple concepts
into this patch.  What I'm proposing here is strictly a requester ID
interface.  I believe that providing a way for a caller to determine
that two devices have a requester ID rooted at the same point in the PCI
topology is directly relevant to that interface.

pci_find_upstream_pcie_bridge() attempts to do this same thing.
Unfortunately, the interface is convoluted, it doesn't handle quirks,
and it requires a great deal of work by the caller to then walk the
topology and create requester IDs at each step.  This also indicates
that at each step, the requester ID is associated with some pci_dev.
Providing a pci_dev is simply showing our work and providing context to
the requester ID (ie. here's the requester ID and the step along the
path from which it was derived.  Here's your top level requester ID and
the point in the topology where it's based).

> > If we look at an endpoint device like A, only A
> > has A's requester ID.  Therefore, why would for_each_requester_id(A)
> > traverse up to Y?
> 
> Even if A is a PCIe device, we have to traverse upwards to find any
> bridges that might drop A's requester ID or take ownership, e.g., if
> we have this:
> 
>   00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
>   01:00.0 PCI-to-PCIe bridge to [bus 02]
>   02:00.0 PCIe endpoint A
> 
> the IOMMU has to look for requester-ID 0100.

And I believe this patch handles this case correctly; I mentioned this
exact example in the v2 RFC cover letter.  This is another example where
pci_find_upstream_pcie_bridge() will currently fail.

> > Y can take ownership and become the requester for A,
> > but if we were to call for_each_requester_id(Y), wouldn't you expect the
> > callback to happen on {Y, A, B} since all of those can use that
> > requester ID?
> 
> No.  If I call for_each_requester_id(Y), I'm expecting the callback
> to happen for each requester ID that could be used for transactions
> originated by *Y*.  I'm trying to make an IOMMU mapping for use by
> Y, so any devices downstream of Y, e.g., A and B, are irrelevant.

Ok, you think of for_each_requester_id() the same as I think of
for_each_requester().  Can we split the difference and call it
pci_dev_for_each_requester_id()?

> I think a lot of the confusion here is because we're trying to solve
> both two questions at once: (1) what requester-IDs need to be mapped
> to handle DMA from a device, and (2) what devices can't be isolated
> from each other and must be in the same domain.  I think things will
> be simpler if we can deal with those separately.  Even if it ends up
> being more code, at least each piece will be easier to understand by
> itself.

Don't we already have this split in the code?

(1) pcie_for_each_requester
    (or pci_dev_for_each_requester_id)

(2) pci_get_visible_pcie_requester
    (or pci_get_visible_pcie_requester_id)

Note however that (2) does not impose anything about domains or
isolation, it is strictly based on PCI topology.  It's left to the
caller to determine how that translates to IOMMU domains, but the
typical case is trivial.  Thanks,

Alex

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

[Bjorn Helgaas]

On Mon, Jul 29, 2013 at 10:06 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
>> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:

>> The "most closely associated device" idea seems confusing to
>> describe and think about.  I think you want to use it as part of
>> grouping devices into domains.  But I think we should attack that
>> problem separately.  For grouping or isolation questions, we have
>> to pay attention to things like ACS, which are not relevant for
>> mapping.
>
> We are only touch isolation insofar as providing an interface for a
> driver to determine the point in the PCI topology where the requester ID
> is rooted.  Yes, grouping can make use of that, but I object to the idea
> that we're approaching some slippery slope of rolling multiple concepts
> into this patch.  What I'm proposing here is strictly a requester ID
> interface.  I believe that providing a way for a caller to determine
> that two devices have a requester ID rooted at the same point in the PCI
> topology is directly relevant to that interface.
>
> pci_find_upstream_pcie_bridge() attempts to do this same thing.
> Unfortunately, the interface is convoluted, it doesn't handle quirks,
> and it requires a great deal of work by the caller to then walk the
> topology and create requester IDs at each step.  This also indicates
> that at each step, the requester ID is associated with some pci_dev.
> Providing a pci_dev is simply showing our work and providing context to
> the requester ID (ie. here's the requester ID and the step along the
> path from which it was derived.  Here's your top level requester ID and
> the point in the topology where it's based).

What does the driver *do* with the pci_dev?  If it's not necessary,
then showing our work and providing context just complicates the
interface and creates opportunities for mistakes.  If we're creating
IOMMU mappings, only the requester ID is needed.

I think you used get_domain_for_dev() and intel_iommu_add_device() as
examples, but as far as I can tell, they use the pci_dev as a way to
learn about isolation.  For that purpose, I don't think you want an
iterator -- you only want to know about the single pci_dev that's the
root of the isolation domain, and requester IDs really aren't
relevant.

>> > If we look at an endpoint device like A, only A
>> > has A's requester ID.  Therefore, why would for_each_requester_id(A)
>> > traverse up to Y?
>>
>> Even if A is a PCIe device, we have to traverse upwards to find any
>> bridges that might drop A's requester ID or take ownership, e.g., if
>> we have this:
>>
>>   00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
>>   01:00.0 PCI-to-PCIe bridge to [bus 02]
>>   02:00.0 PCIe endpoint A
>>
>> the IOMMU has to look for requester-ID 0100.
>
> And I believe this patch handles this case correctly; I mentioned this
> exact example in the v2 RFC cover letter.  This is another example where
> pci_find_upstream_pcie_bridge() will currently fail.

OK, I was just trying to answer your question "why we would need to
traverse up to Y."  But apparently we agree about that.

>> > Y can take ownership and become the requester for A,
>> > but if we were to call for_each_requester_id(Y), wouldn't you expect the
>> > callback to happen on {Y, A, B} since all of those can use that
>> > requester ID?
>>
>> No.  If I call for_each_requester_id(Y), I'm expecting the callback
>> to happen for each requester ID that could be used for transactions
>> originated by *Y*.  I'm trying to make an IOMMU mapping for use by
>> Y, so any devices downstream of Y, e.g., A and B, are irrelevant.
>
> Ok, you think of for_each_requester_id() the same as I think of
> for_each_requester().  Can we split the difference and call it
> pci_dev_for_each_requester_id()?

Sure.

>> I think a lot of the confusion here is because we're trying to solve
>> both two questions at once: (1) what requester-IDs need to be mapped
>> to handle DMA from a device, and (2) what devices can't be isolated
>> from each other and must be in the same domain. ...
>
> Don't we already have this split in the code?
>
> (1) pcie_for_each_requester
>     (or pci_dev_for_each_requester_id)
>
> (2) pci_get_visible_pcie_requester
>     (or pci_get_visible_pcie_requester_id)
>
> Note however that (2) does not impose anything about domains or
> isolation, it is strictly based on PCI topology.  It's left to the
> caller to determine how that translates to IOMMU domains, but the
> typical case is trivial.

Can you expand on this a little bit?  What's involved in translating
the pci_get_visible_pcie_requester() result to an IOMMU domain?  Is
there something IOMMU-specific about this?

I assume that for any given PCI device A, there's a subtree of devices
that the IOMMU can't distinguish from A, and that this subtree is not
IOMMU-specific so it's reasonable to to have a PCI interface to
compute it.  An IOMMU driver might want to expand the domain to
include more devices, of course, and *that* seems like something to
leave to the driver.

I'm also assuming that a pci_dev (the root of the subtree) is a good
way to identify this set of indistinguishable devices because it's
easy to test whether a device B is part of it.  But the requester ID
seems redundant in this case because you can just associate an IOMMU
domain with the pci_dev.

Where does ACS come into the picture?

Bjorn

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

[Alex Williamson]

On Mon, 2013-07-29 at 15:02 -0600, Bjorn Helgaas wrote:
> On Mon, Jul 29, 2013 at 10:06 AM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
> >> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
> 
> >> The "most closely associated device" idea seems confusing to
> >> describe and think about.  I think you want to use it as part of
> >> grouping devices into domains.  But I think we should attack that
> >> problem separately.  For grouping or isolation questions, we have
> >> to pay attention to things like ACS, which are not relevant for
> >> mapping.
> >
> > We are only touch isolation insofar as providing an interface for a
> > driver to determine the point in the PCI topology where the requester ID
> > is rooted.  Yes, grouping can make use of that, but I object to the idea
> > that we're approaching some slippery slope of rolling multiple concepts
> > into this patch.  What I'm proposing here is strictly a requester ID
> > interface.  I believe that providing a way for a caller to determine
> > that two devices have a requester ID rooted at the same point in the PCI
> > topology is directly relevant to that interface.
> >
> > pci_find_upstream_pcie_bridge() attempts to do this same thing.
> > Unfortunately, the interface is convoluted, it doesn't handle quirks,
> > and it requires a great deal of work by the caller to then walk the
> > topology and create requester IDs at each step.  This also indicates
> > that at each step, the requester ID is associated with some pci_dev.
> > Providing a pci_dev is simply showing our work and providing context to
> > the requester ID (ie. here's the requester ID and the step along the
> > path from which it was derived.  Here's your top level requester ID and
> > the point in the topology where it's based).
> 
> What does the driver *do* with the pci_dev?  If it's not necessary,
> then showing our work and providing context just complicates the
> interface and creates opportunities for mistakes.  If we're creating
> IOMMU mappings, only the requester ID is needed.

It's true, I don't have a use for the pci_dev in
pci_dev_for_each_requester_id() today.  But I think providing the
context for a requester ID is valuable information.  Otherwise we have
to make assumptions about the requester ID.  For example, if I have
devices in different PCI segments with the same requester ID the
callback function only knows that those are actually different requester
IDs from information the caller provides itself in the opaque pointer.
This is the case with intel-iommu, but why would we design an API that
requires the caller to provide that kind of context?  I also provide the
pci_dev because I think both the pci_dev_for_each_requester_id() and
pci_get_visible_pcie_requester() should provide similar APIs.  There's
an association of a requester ID to a pci_dev.  Why hide that?

> I think you used get_domain_for_dev() and intel_iommu_add_device() as
> examples, but as far as I can tell, they use the pci_dev as a way to
> learn about isolation.  For that purpose, I don't think you want an
> iterator -- you only want to know about the single pci_dev that's the
> root of the isolation domain, and requester IDs really aren't
> relevant.

See get_domain_for_dev() in patch 2/2.  It uses the returned pci_dev to
know whether the requester ID is rooted upstream or at the device
itself.  If upstream, it then uses the requester ID to search for an
existing domain.  The requester ID is relevant here.  If the returned
pci_dev is the device itself, it proceeds to allocate a domain because
the code path has already checked whether the device itself belongs to a
domain.

The use in intel_iommu_add_device() doesn't use the requester ID, it
only wants the pci_dev since it needs to then search above that for
additional isolation capabilities.  That interface only wants to know
the point in the topology where we have bus level granularity of
devices, so it doesn't care about the actual requester ID.

> >> > If we look at an endpoint device like A, only A
> >> > has A's requester ID.  Therefore, why would for_each_requester_id(A)
> >> > traverse up to Y?
> >>
> >> Even if A is a PCIe device, we have to traverse upwards to find any
> >> bridges that might drop A's requester ID or take ownership, e.g., if
> >> we have this:
> >>
> >>   00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
> >>   01:00.0 PCI-to-PCIe bridge to [bus 02]
> >>   02:00.0 PCIe endpoint A
> >>
> >> the IOMMU has to look for requester-ID 0100.
> >
> > And I believe this patch handles this case correctly; I mentioned this
> > exact example in the v2 RFC cover letter.  This is another example where
> > pci_find_upstream_pcie_bridge() will currently fail.
> 
> OK, I was just trying to answer your question "why we would need to
> traverse up to Y."  But apparently we agree about that.
> 
> >> > Y can take ownership and become the requester for A,
> >> > but if we were to call for_each_requester_id(Y), wouldn't you expect the
> >> > callback to happen on {Y, A, B} since all of those can use that
> >> > requester ID?
> >>
> >> No.  If I call for_each_requester_id(Y), I'm expecting the callback
> >> to happen for each requester ID that could be used for transactions
> >> originated by *Y*.  I'm trying to make an IOMMU mapping for use by
> >> Y, so any devices downstream of Y, e.g., A and B, are irrelevant.
> >
> > Ok, you think of for_each_requester_id() the same as I think of
> > for_each_requester().  Can we split the difference and call it
> > pci_dev_for_each_requester_id()?
> 
> Sure.
> 
> >> I think a lot of the confusion here is because we're trying to solve
> >> both two questions at once: (1) what requester-IDs need to be mapped
> >> to handle DMA from a device, and (2) what devices can't be isolated
> >> from each other and must be in the same domain. ...
> >
> > Don't we already have this split in the code?
> >
> > (1) pcie_for_each_requester
> >     (or pci_dev_for_each_requester_id)
> >
> > (2) pci_get_visible_pcie_requester
> >     (or pci_get_visible_pcie_requester_id)
> >
> > Note however that (2) does not impose anything about domains or
> > isolation, it is strictly based on PCI topology.  It's left to the
> > caller to determine how that translates to IOMMU domains, but the
> > typical case is trivial.
> 
> Can you expand on this a little bit?  What's involved in translating
> the pci_get_visible_pcie_requester() result to an IOMMU domain?  Is
> there something IOMMU-specific about this?

I try not to make any assumptions about what's required to get from a
pci_dev/requester ID to a domain.  The IOMMU may only be able to filter
a subset of the requester ID, for instance what if it has no function
level visibility, or perhaps only bus level visibility.  intel-iommu has
full BDF granularity, so it's perhaps the simplest case.

> I assume that for any given PCI device A, there's a subtree of devices
> that the IOMMU can't distinguish from A, and that this subtree is not
> IOMMU-specific so it's reasonable to to have a PCI interface to
> compute it.  An IOMMU driver might want to expand the domain to
> include more devices, of course, and *that* seems like something to
> leave to the driver.

Yes, I agree.  We're only trying to determine the PCI bus level
visibility, anything else is left to the IOMMU driver.

> I'm also assuming that a pci_dev (the root of the subtree) is a good
> way to identify this set of indistinguishable devices because it's
> easy to test whether a device B is part of it.  But the requester ID
> seems redundant in this case because you can just associate an IOMMU
> domain with the pci_dev.

I don't think we want to assume that an IOMMU driver bases a domain on a
pci_dev or a BDF.  That's specific to the driver.  The requester ID is
not redundant though since it depends on topology and quirks.  A given
pci_dev may have it's own legitimate requester ID or a quirk, where the
quirk may be specific to the device or architected, like a bridge using
the (bridge->subordinate->number << 8 | 0) requester ID.

> Where does ACS come into the picture?

ACS determines whether a transaction must necessarily reach the IOMMU
without being redirected.  Therefore once we figure out the point at
which we have bus level granularity for a transaction, we look upstream
to determine if a transaction must necessarily reach the IOMMU.  In the
case of intel-iommu, IOMMU groups are only dependent on the visibility
provided by the PCI topology and the isolation guaranteed through ACS,
but it's up to the IOMMU driver to determine whether there are
additional constraints.  Thanks,

Alex

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

[Bjorn Helgaas]

On Mon, Jul 29, 2013 at 4:32 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2013-07-29 at 15:02 -0600, Bjorn Helgaas wrote:
>> On Mon, Jul 29, 2013 at 10:06 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
>> >> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
>>
>> >> The "most closely associated device" idea seems confusing to
>> >> describe and think about.  I think you want to use it as part of
>> >> grouping devices into domains.  But I think we should attack that
>> >> problem separately.  For grouping or isolation questions, we have
>> >> to pay attention to things like ACS, which are not relevant for
>> >> mapping.
>> >
>> > We are only touch isolation insofar as providing an interface for a
>> > driver to determine the point in the PCI topology where the requester ID
>> > is rooted.  Yes, grouping can make use of that, but I object to the idea
>> > that we're approaching some slippery slope of rolling multiple concepts
>> > into this patch.  What I'm proposing here is strictly a requester ID
>> > interface.  I believe that providing a way for a caller to determine
>> > that two devices have a requester ID rooted at the same point in the PCI
>> > topology is directly relevant to that interface.
>> >
>> > pci_find_upstream_pcie_bridge() attempts to do this same thing.
>> > Unfortunately, the interface is convoluted, it doesn't handle quirks,
>> > and it requires a great deal of work by the caller to then walk the
>> > topology and create requester IDs at each step.  This also indicates
>> > that at each step, the requester ID is associated with some pci_dev.
>> > Providing a pci_dev is simply showing our work and providing context to
>> > the requester ID (ie. here's the requester ID and the step along the
>> > path from which it was derived.  Here's your top level requester ID and
>> > the point in the topology where it's based).
>>
>> What does the driver *do* with the pci_dev?  If it's not necessary,
>> then showing our work and providing context just complicates the
>> interface and creates opportunities for mistakes.  If we're creating
>> IOMMU mappings, only the requester ID is needed.
>
> It's true, I don't have a use for the pci_dev in
> pci_dev_for_each_requester_id() today.  But I think providing the
> context for a requester ID is valuable information.  Otherwise we have
> to make assumptions about the requester ID.  For example, if I have
> devices in different PCI segments with the same requester ID the
> callback function only knows that those are actually different requester
> IDs from information the caller provides itself in the opaque pointer.
> This is the case with intel-iommu, but why would we design an API that
> requires the caller to provide that kind of context?

The caller already has to provide context, e.g., the domain in which
to create a mapping, anyway via the opaque pointer.  So I would argue
that it's pointless to supply context twice in different ways.

We only have one caller of pci_dev_for_each_requester_id() anyway
(intel-iommu.c).  That seems really strange to me.  All I can assume
is that other IOMMU drivers *should* be doing something like this, but
aren't yet.  Anyway, since we only have one user, why not just provide
the minimum (only the requester ID), and add the pci_dev later if a
requirement for it turns up?

>  I also provide the
> pci_dev because I think both the pci_dev_for_each_requester_id() and
> pci_get_visible_pcie_requester() should provide similar APIs.  There's
> an association of a requester ID to a pci_dev.  Why hide that?

Information hiding is a basic idea of abstraction and encapsulation.
If we don't hide unnecessary information, we end up with unnecessary
dependencies.

>> I think you used get_domain_for_dev() and intel_iommu_add_device() as
>> examples, but as far as I can tell, they use the pci_dev as a way to
>> learn about isolation.  For that purpose, I don't think you want an
>> iterator -- you only want to know about the single pci_dev that's the
>> root of the isolation domain, and requester IDs really aren't
>> relevant.
>
> See get_domain_for_dev() in patch 2/2.  It uses the returned pci_dev to
> know whether the requester ID is rooted upstream or at the device
> itself.  If upstream, it then uses the requester ID to search for an
> existing domain.  The requester ID is relevant here.  If the returned
> pci_dev is the device itself, it proceeds to allocate a domain because
> the code path has already checked whether the device itself belongs to a
> domain.

I guess I got myself confused here.  get_domain_for_dev() uses
pci_get_visible_pcie_requester(), not pci_dev_for_each_requester_id().

>> > (1) pcie_for_each_requester
>> >     (or pci_dev_for_each_requester_id)
>> >
>> > (2) pci_get_visible_pcie_requester
>> >     (or pci_get_visible_pcie_requester_id)
>> >
>> > Note however that (2) does not impose anything about domains or
>> > isolation, it is strictly based on PCI topology.  It's left to the
>> > caller to determine how that translates to IOMMU domains, but the
>> > typical case is trivial.
>>
>> Can you expand on this a little bit?  What's involved in translating
>> the pci_get_visible_pcie_requester() result to an IOMMU domain?  Is
>> there something IOMMU-specific about this?
>
> I try not to make any assumptions about what's required to get from a
> pci_dev/requester ID to a domain.  The IOMMU may only be able to filter
> a subset of the requester ID, for instance what if it has no function
> level visibility, or perhaps only bus level visibility.  intel-iommu has
> full BDF granularity, so it's perhaps the simplest case.
>
>> I assume that for any given PCI device A, there's a subtree of devices
>> that the IOMMU can't distinguish from A, and that this subtree is not
>> IOMMU-specific so it's reasonable to to have a PCI interface to
>> compute it.  An IOMMU driver might want to expand the domain to
>> include more devices, of course, and *that* seems like something to
>> leave to the driver.
>
> Yes, I agree.  We're only trying to determine the PCI bus level
> visibility, anything else is left to the IOMMU driver.
>
>> I'm also assuming that a pci_dev (the root of the subtree) is a good
>> way to identify this set of indistinguishable devices because it's
>> easy to test whether a device B is part of it.  But the requester ID
>> seems redundant in this case because you can just associate an IOMMU
>> domain with the pci_dev.
>
> I don't think we want to assume that an IOMMU driver bases a domain on a
> pci_dev or a BDF.  That's specific to the driver.  The requester ID is
> not redundant though since it depends on topology and quirks.  A given
> pci_dev may have it's own legitimate requester ID or a quirk, where the
> quirk may be specific to the device or architected, like a bridge using
> the (bridge->subordinate->number << 8 | 0) requester ID.
>
>> Where does ACS come into the picture?
>
> ACS determines whether a transaction must necessarily reach the IOMMU
> without being redirected.  Therefore once we figure out the point at
> which we have bus level granularity for a transaction, we look upstream
> to determine if a transaction must necessarily reach the IOMMU.  In the
> case of intel-iommu, IOMMU groups are only dependent on the visibility
> provided by the PCI topology and the isolation guaranteed through ACS,
> but it's up to the IOMMU driver to determine whether there are
> additional constraints.  Thanks,

What code will look for ACS support and enablement?  Do you envision
that being in the IOMMU code or somewhere in PCI?  I was hoping that
the isolation/domain identification stuff could be fairly generic, but
it sounds like that's not the case so I'm guessing the interesting
parts of it will be in IOMMU code.

So I think I'm willing to go along with
pci_get_visible_pcie_requester() returning both a pci_dev and a
requester ID.  Is a single one of each sufficient?  I guess it is -- I
don't see any loops around where you're calling it.

Bjorn

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

[Alex Williamson]

On Thu, 2013-08-01 at 16:08 -0600, Bjorn Helgaas wrote:
> On Mon, Jul 29, 2013 at 4:32 PM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, 2013-07-29 at 15:02 -0600, Bjorn Helgaas wrote:
> >> On Mon, Jul 29, 2013 at 10:06 AM, Alex Williamson
> >> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
> >> >> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
> >>
> >> >> The "most closely associated device" idea seems confusing to
> >> >> describe and think about.  I think you want to use it as part of
> >> >> grouping devices into domains.  But I think we should attack that
> >> >> problem separately.  For grouping or isolation questions, we have
> >> >> to pay attention to things like ACS, which are not relevant for
> >> >> mapping.
> >> >
> >> > We are only touch isolation insofar as providing an interface for a
> >> > driver to determine the point in the PCI topology where the requester ID
> >> > is rooted.  Yes, grouping can make use of that, but I object to the idea
> >> > that we're approaching some slippery slope of rolling multiple concepts
> >> > into this patch.  What I'm proposing here is strictly a requester ID
> >> > interface.  I believe that providing a way for a caller to determine
> >> > that two devices have a requester ID rooted at the same point in the PCI
> >> > topology is directly relevant to that interface.
> >> >
> >> > pci_find_upstream_pcie_bridge() attempts to do this same thing.
> >> > Unfortunately, the interface is convoluted, it doesn't handle quirks,
> >> > and it requires a great deal of work by the caller to then walk the
> >> > topology and create requester IDs at each step.  This also indicates
> >> > that at each step, the requester ID is associated with some pci_dev.
> >> > Providing a pci_dev is simply showing our work and providing context to
> >> > the requester ID (ie. here's the requester ID and the step along the
> >> > path from which it was derived.  Here's your top level requester ID and
> >> > the point in the topology where it's based).
> >>
> >> What does the driver *do* with the pci_dev?  If it's not necessary,
> >> then showing our work and providing context just complicates the
> >> interface and creates opportunities for mistakes.  If we're creating
> >> IOMMU mappings, only the requester ID is needed.
> >
> > It's true, I don't have a use for the pci_dev in
> > pci_dev_for_each_requester_id() today.  But I think providing the
> > context for a requester ID is valuable information.  Otherwise we have
> > to make assumptions about the requester ID.  For example, if I have
> > devices in different PCI segments with the same requester ID the
> > callback function only knows that those are actually different requester
> > IDs from information the caller provides itself in the opaque pointer.
> > This is the case with intel-iommu, but why would we design an API that
> > requires the caller to provide that kind of context?
> 
> The caller already has to provide context, e.g., the domain in which
> to create a mapping, anyway via the opaque pointer.  So I would argue
> that it's pointless to supply context twice in different ways.
> 
> We only have one caller of pci_dev_for_each_requester_id() anyway
> (intel-iommu.c).  That seems really strange to me.  All I can assume
> is that other IOMMU drivers *should* be doing something like this, but
> aren't yet.  Anyway, since we only have one user, why not just provide
> the minimum (only the requester ID), and add the pci_dev later if a
> requirement for it turns up?


Ok, I can do that.  I think we only have one user because intel-iommu is
the only in-tree user of pci_find_upstream_pcie_bridge().  I know the
powerpc folks have been using similar code though.  amd_iommu bypasses
because they generate an alias table based on information for firmware.
That replaces some of this manual grep'ing through the topology.
Perhaps Joerg could comment about whether this is a useful interface for
amd_iommu.

> >  I also provide the
> > pci_dev because I think both the pci_dev_for_each_requester_id() and
> > pci_get_visible_pcie_requester() should provide similar APIs.  There's
> > an association of a requester ID to a pci_dev.  Why hide that?
> 
> Information hiding is a basic idea of abstraction and encapsulation.
> If we don't hide unnecessary information, we end up with unnecessary
> dependencies.
> 
> >> I think you used get_domain_for_dev() and intel_iommu_add_device() as
> >> examples, but as far as I can tell, they use the pci_dev as a way to
> >> learn about isolation.  For that purpose, I don't think you want an
> >> iterator -- you only want to know about the single pci_dev that's the
> >> root of the isolation domain, and requester IDs really aren't
> >> relevant.
> >
> > See get_domain_for_dev() in patch 2/2.  It uses the returned pci_dev to
> > know whether the requester ID is rooted upstream or at the device
> > itself.  If upstream, it then uses the requester ID to search for an
> > existing domain.  The requester ID is relevant here.  If the returned
> > pci_dev is the device itself, it proceeds to allocate a domain because
> > the code path has already checked whether the device itself belongs to a
> > domain.
> 
> I guess I got myself confused here.  get_domain_for_dev() uses
> pci_get_visible_pcie_requester(), not pci_dev_for_each_requester_id().
> 
> >> > (1) pcie_for_each_requester
> >> >     (or pci_dev_for_each_requester_id)
> >> >
> >> > (2) pci_get_visible_pcie_requester
> >> >     (or pci_get_visible_pcie_requester_id)
> >> >
> >> > Note however that (2) does not impose anything about domains or
> >> > isolation, it is strictly based on PCI topology.  It's left to the
> >> > caller to determine how that translates to IOMMU domains, but the
> >> > typical case is trivial.
> >>
> >> Can you expand on this a little bit?  What's involved in translating
> >> the pci_get_visible_pcie_requester() result to an IOMMU domain?  Is
> >> there something IOMMU-specific about this?
> >
> > I try not to make any assumptions about what's required to get from a
> > pci_dev/requester ID to a domain.  The IOMMU may only be able to filter
> > a subset of the requester ID, for instance what if it has no function
> > level visibility, or perhaps only bus level visibility.  intel-iommu has
> > full BDF granularity, so it's perhaps the simplest case.
> >
> >> I assume that for any given PCI device A, there's a subtree of devices
> >> that the IOMMU can't distinguish from A, and that this subtree is not
> >> IOMMU-specific so it's reasonable to to have a PCI interface to
> >> compute it.  An IOMMU driver might want to expand the domain to
> >> include more devices, of course, and *that* seems like something to
> >> leave to the driver.
> >
> > Yes, I agree.  We're only trying to determine the PCI bus level
> > visibility, anything else is left to the IOMMU driver.
> >
> >> I'm also assuming that a pci_dev (the root of the subtree) is a good
> >> way to identify this set of indistinguishable devices because it's
> >> easy to test whether a device B is part of it.  But the requester ID
> >> seems redundant in this case because you can just associate an IOMMU
> >> domain with the pci_dev.
> >
> > I don't think we want to assume that an IOMMU driver bases a domain on a
> > pci_dev or a BDF.  That's specific to the driver.  The requester ID is
> > not redundant though since it depends on topology and quirks.  A given
> > pci_dev may have it's own legitimate requester ID or a quirk, where the
> > quirk may be specific to the device or architected, like a bridge using
> > the (bridge->subordinate->number << 8 | 0) requester ID.
> >
> >> Where does ACS come into the picture?
> >
> > ACS determines whether a transaction must necessarily reach the IOMMU
> > without being redirected.  Therefore once we figure out the point at
> > which we have bus level granularity for a transaction, we look upstream
> > to determine if a transaction must necessarily reach the IOMMU.  In the
> > case of intel-iommu, IOMMU groups are only dependent on the visibility
> > provided by the PCI topology and the isolation guaranteed through ACS,
> > but it's up to the IOMMU driver to determine whether there are
> > additional constraints.  Thanks,
> 
> What code will look for ACS support and enablement?  Do you envision
> that being in the IOMMU code or somewhere in PCI?  I was hoping that
> the isolation/domain identification stuff could be fairly generic, but
> it sounds like that's not the case so I'm guessing the interesting
> parts of it will be in IOMMU code.

ACS support is currently in the IOMMUs and it's pretty much a mirror
image in both intel-iommu and amd_iommu (and soon powerpc), so it's
another good opportunity to create something to facilitate it.  The
specific IOMMU drivers are definitely more involved in determining
device visibility since that directly translates into their page table
entries, but ACS determines isolation, which is generic and determined
exclusively by device capabilities and quirks.

> So I think I'm willing to go along with
> pci_get_visible_pcie_requester() returning both a pci_dev and a
> requester ID.  Is a single one of each sufficient?  I guess it is -- I
> don't see any loops around where you're calling it.

Great!  I'm still trying to figure out how to handle the quirk around
Intel PCI-to-PCI bridge on the root complex as just another quirk.  I
respin another version once I have that worked out.  Thanks,

Alex

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

[Bjorn Helgaas]

[+cc George]

On Fri, Aug 2, 2013 at 10:59 AM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> ...
> Great!  I'm still trying to figure out how to handle the quirk around
> Intel PCI-to-PCI bridge on the root complex as just another quirk.  I
> respin another version once I have that worked out.  Thanks,

Is anything happening here?  These buggy IOMMU/DMA source problems,
e.g., [1], have been lingering a long time, and I don't know if we're
stuck because I haven't been giving them enough attention, or if we
don't really have a good solution yet.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=42679

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

[Alex Williamson]

On Thu, 2014-04-03 at 15:48 -0600, Bjorn Helgaas wrote:
> [+cc George]
> 
> On Fri, Aug 2, 2013 at 10:59 AM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > ...
> > Great!  I'm still trying to figure out how to handle the quirk around
> > Intel PCI-to-PCI bridge on the root complex as just another quirk.  I
> > respin another version once I have that worked out.  Thanks,
> 
> Is anything happening here?  These buggy IOMMU/DMA source problems,
> e.g., [1], have been lingering a long time, and I don't know if we're
> stuck because I haven't been giving them enough attention, or if we
> don't really have a good solution yet.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=42679

Sorry, no.  This has completely dropped off my radar.  I'll try to
resurrect it and figure out how to move forward.  Thanks,

Alex

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

[Bjorn Helgaas]

On Thu, Apr 3, 2014 at 8:51 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 2014-04-03 at 15:48 -0600, Bjorn Helgaas wrote:
>> [+cc George]
>>
>> On Fri, Aug 2, 2013 at 10:59 AM, Alex Williamson
>> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > ...
>> > Great!  I'm still trying to figure out how to handle the quirk around
>> > Intel PCI-to-PCI bridge on the root complex as just another quirk.  I
>> > respin another version once I have that worked out.  Thanks,
>>
>> Is anything happening here?  These buggy IOMMU/DMA source problems,
>> e.g., [1], have been lingering a long time, and I don't know if we're
>> stuck because I haven't been giving them enough attention, or if we
>> don't really have a good solution yet.
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=42679
>
> Sorry, no.  This has completely dropped off my radar.  I'll try to
> resurrect it and figure out how to move forward.  Thanks,

Not a problem; I'm just way behind on processing patches, so I'm
trying to clean up my backlog and take care of things that are waiting
on me.

Bjorn

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

[Don Dutile]

On 07/29/2013 12:06 PM, Alex Williamson wrote:
> On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
>> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
>>> On Wed, 2013-07-24 at 17:24 -0600, Bjorn Helgaas wrote:
>>>> On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
>>>>> On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
>>>>>> On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
>>>>>> <alex.williamson@redhat.com>  wrote:
>>>>>>> On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
>>>>>>>> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
>>>>>>    As the DMA
>>>>>> transaction propagates through the fabric, it may be tagged by bridges
>>>>>> with different requester IDs.
>>>>>>
>>>>>> The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
>>>>>> not sure the intermediate pci_devs are.
>>>>>
>>>>> A u16 requester ID doesn't mean much on it's own though, it's not
>>>>> necessarily even unique.  A requester ID associated with the context of
>>>>> a pci_dev is unique and gives us a reference point if we need to perform
>>>>> another operation on that requester ID.
>>>>
>>>> A u16 requester ID better mean something to an IOMMU -- it's all the
>>>> IOMMU can use to look up the correct mapping.  That's why we have to
>>>> give the iterator something to define the scope to iterate over.  The
>>>> same requester ID could mean something totally unrelated in a
>>>> different scope, e.g., below a different IOMMU.
>>>
>>> The point I'm trying to make is that a requester ID depends on it's
>>> context (minimally, the PCI segment).  The caller can assume the context
>>> based on the calling parameters or we can provide context in the form of
>>> an associated pci_dev.  I chose the latter path because I prefer
>>> explicit interfaces and it has some usefulness in the intel-iommu
>>> implementation.
>>>
>>> For instance, get_domain_for_dev() first looks to see if a pci_dev
>>> already has a domain.  If it doesn't, we want to look to see if there's
>>> an upstream device that would use the same requester ID that already has
>>> a domain.  If our get-requester-ID-for-device function returns only the
>>> requester ID, we don't know if that requester ID is the device we're
>>> searching from or some upstream device.  Therefore we potentially do an
>>> unnecessary search for the domain.
>>>
>>> The other user is intel_iommu_add_device() where we're trying to build
>>> IOMMU groups.  Visibility is the first requirement of an IOMMU group.
>>> If the IOMMU cannot distinguish between devices, they must be part of
>>> the same IOMMU group.  Here we want to find the pci_dev that hosts the
>>> requester ID.  I don't even know how we'd implement this with a function
>>> that only returned the requester ID.  Perhaps we'd have to walk upstream
>>> from the device calling the get-requester-ID-for-device function at each
>>> step and noticing when it changed.  That moves significant logic back
>>> into the caller code.
>>> ...
>>
>>>> I don't see the point of passing a "device closest to the requester
>>>> ID."  What would the IOMMU do with that?  As far as the IOMMU is
>>>> concerned, the requester ID could be an arbitrary number completely
>>>> unrelated to a pci_dev.
>>>
>>> Do you have an example of a case where a requester ID doesn't have some
>>> association to a pci_dev?
>>
>> I think our confusion here is the same as what Don&  I have been
>> hashing out -- I'm saying a requester ID fabricated by a bridge
>> need not correspond to a specific pci_dev, and you probably mean
>> that every requester ID is by definition the result of *some* PCI
>> device making a DMA request.
>
> Yes
>
>>> ...
>>> Furthermore, if we have:
>>>
>>>       -- A
>>>      /
>>> X--Y
>>>      \
>>>       -- B
>>> ...
>>
>>> Let me go back to the X-Y-A|B example above to see if I can explain why
>>> pcie_for_each_requester_id() doesn't make sense to me.  Generally a
>>> for_each_foo function should iterate across all things with the same
>>> foo.  So a for_each_requester_id should iterate across all things with
>>> the same requester ID.
>>
>> Hm, that's not the way I think of for_each_FOO() interfaces.  I
>> think of it as "execute the body (or callback) for every possible
>> FOO", where FOO is different each time.  for_each_pci_dev(),
>> pci_bus_for_each_resource(), for_each_zone(), for_each_cpu(), etc.,
>> work like that.
>
> Most of these aren't relevant because they iterate over everything.  I
> think they only show that if we had a pci_for_each_requester_id() that
> it should iterate over every possible PCI requester ID in the system and
> the same could be said for pci_for_each_requester().
>
Lost me here.... I thought the functions took in a pdev, so it'll only
iterate on the segment that pdev is associated with.

> pci_bus_for_each_resource() is the only one we can build from; for all
> resources on a bus.  We want all requester IDs for a pci_dev.  Does that
> perhaps mean it should be called pci_dev_for_each_requester_id()?  I'd
> be ok with that name, but I think it even more implies that a pci_dev is
> associated with a requester ID.
>
and the fcn call name changes have equally lost me here.
AIUI, IOMMUs want to call a PCI core function with a pdev, asking for
the req-id's that the pdev may generate to the IOMMU when performing DMA.
that doesn't strike me as a for-each-requester-id() but a 'get-requester-id()'
operation.

>> But the more important question is what arguments we give to the
>> callback.  My proposal was to map
>>
>>    {pci_dev ->  {requester-ID-A, requester-ID-B, ...}}
>>
>> Yours is to map
>>
>>    {pci_dev ->  {{pci_dev-A, requester-ID-A}, {pci_dev-B, requester-ID-B}, ...}}
>>
>> i.e., your callback gets both a pci_dev and a requester-ID.  I'm
>> trying to figure out how you handle cases where requester-id-A
>> doesn't have a corresponding pci_dev-A.  I guess you pass the device
>> most closely associated with requester-id-A
>
> Yes
>
>> The "most closely associated device" idea seems confusing to
>> describe and think about.  I think you want to use it as part of
>> grouping devices into domains.  But I think we should attack that
>> problem separately.  For grouping or isolation questions, we have
>> to pay attention to things like ACS, which are not relevant for
>> mapping.
>
> We are only touch isolation insofar as providing an interface for a
> driver to determine the point in the PCI topology where the requester ID
> is rooted.  Yes, grouping can make use of that, but I object to the idea
> that we're approaching some slippery slope of rolling multiple concepts
> into this patch.  What I'm proposing here is strictly a requester ID
> interface.  I believe that providing a way for a caller to determine
> that two devices have a requester ID rooted at the same point in the PCI
> topology is directly relevant to that interface.
>
I strongly agree here.  providing the pdev's associated with each req-id rtn'd
cannot hurt, and in fact, I believe it is an improvement, avoiding a potential
set of more interfaces that may be needed to do (segment, req-id)->pdev mapping/matching/get-ing.


> pci_find_upstream_pcie_bridge() attempts to do this same thing.
> Unfortunately, the interface is convoluted, it doesn't handle quirks,
> and it requires a great deal of work by the caller to then walk the
> topology and create requester IDs at each step.  This also indicates
> that at each step, the requester ID is associated with some pci_dev.
> Providing a pci_dev is simply showing our work and providing context to
> the requester ID (ie. here's the requester ID and the step along the
> path from which it was derived.  Here's your top level requester ID and
> the point in the topology where it's based).
>
IMO, getting rid of pci_find_upstream_pcie_bridge() gets non-PCI code
out of the biz of knowing too much about PCI topology and the idiosyncracies
around req-id's, and the proposed interface cleans up the IOMMU (PCI) support.
Having the IOMMU api get the pdev rtn'd with a req-id, and then passing it
back to the core (for release/free) seems like the proper get/put logic
btwn the PCI core and another kernel subsystem.

>>> If we look at an endpoint device like A, only A
>>> has A's requester ID.  Therefore, why would for_each_requester_id(A)
>>> traverse up to Y?
>>
>> Even if A is a PCIe device, we have to traverse upwards to find any
>> bridges that might drop A's requester ID or take ownership, e.g., if
>> we have this:
>>
>>    00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
>>    01:00.0 PCI-to-PCIe bridge to [bus 02]
>>    02:00.0 PCIe endpoint A
>>
>> the IOMMU has to look for requester-ID 0100.
>
> And I believe this patch handles this case correctly; I mentioned this
> exact example in the v2 RFC cover letter.  This is another example where
> pci_find_upstream_pcie_bridge() will currently fail.
>
+1

>>> Y can take ownership and become the requester for A,
>>> but if we were to call for_each_requester_id(Y), wouldn't you expect the
>>> callback to happen on {Y, A, B} since all of those can use that
>>> requester ID?
>>
>> No.  If I call for_each_requester_id(Y), I'm expecting the callback
>> to happen for each requester ID that could be used for transactions
>> originated by *Y*.  I'm trying to make an IOMMU mapping for use by
>> Y, so any devices downstream of Y, e.g., A and B, are irrelevant.
>
> Ok, you think of for_each_requester_id() the same as I think of
> for_each_requester().  Can we split the difference and call it
> pci_dev_for_each_requester_id()?
>
I can only ditto my sentiment above.
Can I toss in 'pci_get_requester_id()'?!? ... ;-)

>> I think a lot of the confusion here is because we're trying to solve
>> both two questions at once: (1) what requester-IDs need to be mapped
>> to handle DMA from a device, and (2) what devices can't be isolated
>> from each other and must be in the same domain.  I think things will
>> be simpler if we can deal with those separately.  Even if it ends up
>> being more code, at least each piece will be easier to understand by
>> itself.
>
> Don't we already have this split in the code?
>
> (1) pcie_for_each_requester
>      (or pci_dev_for_each_requester_id)
>
> (2) pci_get_visible_pcie_requester
>      (or pci_get_visible_pcie_requester_id)
>
again, unless you understand all of the mapped/coerced/modified request-id
of PCIe, PCIe-to-PCI(x) bridges, and the potential (RC, other) quirks,
these interface names are confusing. ....
... that could be me, and I need to go back to look at patches and
the description of the functions, to see if they help to clarify their uses.

> Note however that (2) does not impose anything about domains or
> isolation, it is strictly based on PCI topology.  It's left to the
> caller to determine how that translates to IOMMU domains, but the
> typical case is trivial.  Thanks,
>
+1, again.

> Alex
>

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

[Alex Williamson]

On Mon, 2013-07-29 at 17:03 -0400, Don Dutile wrote:
> On 07/29/2013 12:06 PM, Alex Williamson wrote:
> > On Fri, 2013-07-26 at 15:54 -0600, Bjorn Helgaas wrote:
> >> On Thu, Jul 25, 2013 at 11:56:56AM -0600, Alex Williamson wrote:
> >>> On Wed, 2013-07-24 at 17:24 -0600, Bjorn Helgaas wrote:
> >>>> On Wed, Jul 24, 2013 at 12:12:28PM -0600, Alex Williamson wrote:
> >>>>> On Wed, 2013-07-24 at 10:47 -0600, Bjorn Helgaas wrote:
> >>>>>> On Tue, Jul 23, 2013 at 5:21 PM, Alex Williamson
> >>>>>> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>  wrote:
> >>>>>>> On Tue, 2013-07-23 at 16:35 -0600, Bjorn Helgaas wrote:
> >>>>>>>> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> >>>>>>    As the DMA
> >>>>>> transaction propagates through the fabric, it may be tagged by bridges
> >>>>>> with different requester IDs.
> >>>>>>
> >>>>>> The requester IDs are needed outside PCI (by IOMMU drivers), but I'm
> >>>>>> not sure the intermediate pci_devs are.
> >>>>>
> >>>>> A u16 requester ID doesn't mean much on it's own though, it's not
> >>>>> necessarily even unique.  A requester ID associated with the context of
> >>>>> a pci_dev is unique and gives us a reference point if we need to perform
> >>>>> another operation on that requester ID.
> >>>>
> >>>> A u16 requester ID better mean something to an IOMMU -- it's all the
> >>>> IOMMU can use to look up the correct mapping.  That's why we have to
> >>>> give the iterator something to define the scope to iterate over.  The
> >>>> same requester ID could mean something totally unrelated in a
> >>>> different scope, e.g., below a different IOMMU.
> >>>
> >>> The point I'm trying to make is that a requester ID depends on it's
> >>> context (minimally, the PCI segment).  The caller can assume the context
> >>> based on the calling parameters or we can provide context in the form of
> >>> an associated pci_dev.  I chose the latter path because I prefer
> >>> explicit interfaces and it has some usefulness in the intel-iommu
> >>> implementation.
> >>>
> >>> For instance, get_domain_for_dev() first looks to see if a pci_dev
> >>> already has a domain.  If it doesn't, we want to look to see if there's
> >>> an upstream device that would use the same requester ID that already has
> >>> a domain.  If our get-requester-ID-for-device function returns only the
> >>> requester ID, we don't know if that requester ID is the device we're
> >>> searching from or some upstream device.  Therefore we potentially do an
> >>> unnecessary search for the domain.
> >>>
> >>> The other user is intel_iommu_add_device() where we're trying to build
> >>> IOMMU groups.  Visibility is the first requirement of an IOMMU group.
> >>> If the IOMMU cannot distinguish between devices, they must be part of
> >>> the same IOMMU group.  Here we want to find the pci_dev that hosts the
> >>> requester ID.  I don't even know how we'd implement this with a function
> >>> that only returned the requester ID.  Perhaps we'd have to walk upstream
> >>> from the device calling the get-requester-ID-for-device function at each
> >>> step and noticing when it changed.  That moves significant logic back
> >>> into the caller code.
> >>> ...
> >>
> >>>> I don't see the point of passing a "device closest to the requester
> >>>> ID."  What would the IOMMU do with that?  As far as the IOMMU is
> >>>> concerned, the requester ID could be an arbitrary number completely
> >>>> unrelated to a pci_dev.
> >>>
> >>> Do you have an example of a case where a requester ID doesn't have some
> >>> association to a pci_dev?
> >>
> >> I think our confusion here is the same as what Don&  I have been
> >> hashing out -- I'm saying a requester ID fabricated by a bridge
> >> need not correspond to a specific pci_dev, and you probably mean
> >> that every requester ID is by definition the result of *some* PCI
> >> device making a DMA request.
> >
> > Yes
> >
> >>> ...
> >>> Furthermore, if we have:
> >>>
> >>>       -- A
> >>>      /
> >>> X--Y
> >>>      \
> >>>       -- B
> >>> ...
> >>
> >>> Let me go back to the X-Y-A|B example above to see if I can explain why
> >>> pcie_for_each_requester_id() doesn't make sense to me.  Generally a
> >>> for_each_foo function should iterate across all things with the same
> >>> foo.  So a for_each_requester_id should iterate across all things with
> >>> the same requester ID.
> >>
> >> Hm, that's not the way I think of for_each_FOO() interfaces.  I
> >> think of it as "execute the body (or callback) for every possible
> >> FOO", where FOO is different each time.  for_each_pci_dev(),
> >> pci_bus_for_each_resource(), for_each_zone(), for_each_cpu(), etc.,
> >> work like that.
> >
> > Most of these aren't relevant because they iterate over everything.  I
> > think they only show that if we had a pci_for_each_requester_id() that
> > it should iterate over every possible PCI requester ID in the system and
> > the same could be said for pci_for_each_requester().
> >
> Lost me here.... I thought the functions took in a pdev, so it'll only
> iterate on the segment that pdev is associated with.

Right, but how does the callback function know the segment we're
traversing?  Without a pci_dev a requester ID could belong to any PCI
segment.  It would be up to the caller to provide context in the opaque
pointer.

> > pci_bus_for_each_resource() is the only one we can build from; for all
> > resources on a bus.  We want all requester IDs for a pci_dev.  Does that
> > perhaps mean it should be called pci_dev_for_each_requester_id()?  I'd
> > be ok with that name, but I think it even more implies that a pci_dev is
> > associated with a requester ID.
> >
> and the fcn call name changes have equally lost me here.
> AIUI, IOMMUs want to call a PCI core function with a pdev, asking for
> the req-id's that the pdev may generate to the IOMMU when performing DMA.
> that doesn't strike me as a for-each-requester-id() but a 'get-requester-id()'
> operation.

But we have to support multiple requester IDs per endpoint.  As Bjorn
pointed out in the spec, a PCI-X bridge may pass the requester ID or
take ownership of the transaction.  Therefore we need to add both the
"endpoint" and the (bridge->secondary->number << 8 | 0) requester IDs.
We could do a get-requester-id() interface, but then the caller must
walk upstream through each device that may possibly take ownership of
the transactions.  That leaves a lot of logic in the caller versus a
for-each-requester-id() operation.


> >> But the more important question is what arguments we give to the
> >> callback.  My proposal was to map
> >>
> >>    {pci_dev ->  {requester-ID-A, requester-ID-B, ...}}
> >>
> >> Yours is to map
> >>
> >>    {pci_dev ->  {{pci_dev-A, requester-ID-A}, {pci_dev-B, requester-ID-B}, ...}}
> >>
> >> i.e., your callback gets both a pci_dev and a requester-ID.  I'm
> >> trying to figure out how you handle cases where requester-id-A
> >> doesn't have a corresponding pci_dev-A.  I guess you pass the device
> >> most closely associated with requester-id-A
> >
> > Yes
> >
> >> The "most closely associated device" idea seems confusing to
> >> describe and think about.  I think you want to use it as part of
> >> grouping devices into domains.  But I think we should attack that
> >> problem separately.  For grouping or isolation questions, we have
> >> to pay attention to things like ACS, which are not relevant for
> >> mapping.
> >
> > We are only touch isolation insofar as providing an interface for a
> > driver to determine the point in the PCI topology where the requester ID
> > is rooted.  Yes, grouping can make use of that, but I object to the idea
> > that we're approaching some slippery slope of rolling multiple concepts
> > into this patch.  What I'm proposing here is strictly a requester ID
> > interface.  I believe that providing a way for a caller to determine
> > that two devices have a requester ID rooted at the same point in the PCI
> > topology is directly relevant to that interface.
> >
> I strongly agree here.  providing the pdev's associated with each req-id rtn'd
> cannot hurt, and in fact, I believe it is an improvement, avoiding a potential
> set of more interfaces that may be needed to do (segment, req-id)->pdev mapping/matching/get-ing.
> 
> 
> > pci_find_upstream_pcie_bridge() attempts to do this same thing.
> > Unfortunately, the interface is convoluted, it doesn't handle quirks,
> > and it requires a great deal of work by the caller to then walk the
> > topology and create requester IDs at each step.  This also indicates
> > that at each step, the requester ID is associated with some pci_dev.
> > Providing a pci_dev is simply showing our work and providing context to
> > the requester ID (ie. here's the requester ID and the step along the
> > path from which it was derived.  Here's your top level requester ID and
> > the point in the topology where it's based).
> >
> IMO, getting rid of pci_find_upstream_pcie_bridge() gets non-PCI code
> out of the biz of knowing too much about PCI topology and the idiosyncracies
> around req-id's, and the proposed interface cleans up the IOMMU (PCI) support.
> Having the IOMMU api get the pdev rtn'd with a req-id, and then passing it
> back to the core (for release/free) seems like the proper get/put logic
> btwn the PCI core and another kernel subsystem.
> 
> >>> If we look at an endpoint device like A, only A
> >>> has A's requester ID.  Therefore, why would for_each_requester_id(A)
> >>> traverse up to Y?
> >>
> >> Even if A is a PCIe device, we have to traverse upwards to find any
> >> bridges that might drop A's requester ID or take ownership, e.g., if
> >> we have this:
> >>
> >>    00:1c.0 PCIe-to-PCI bridge to [bus 01-02]
> >>    01:00.0 PCI-to-PCIe bridge to [bus 02]
> >>    02:00.0 PCIe endpoint A
> >>
> >> the IOMMU has to look for requester-ID 0100.
> >
> > And I believe this patch handles this case correctly; I mentioned this
> > exact example in the v2 RFC cover letter.  This is another example where
> > pci_find_upstream_pcie_bridge() will currently fail.
> >
> +1
> 
> >>> Y can take ownership and become the requester for A,
> >>> but if we were to call for_each_requester_id(Y), wouldn't you expect the
> >>> callback to happen on {Y, A, B} since all of those can use that
> >>> requester ID?
> >>
> >> No.  If I call for_each_requester_id(Y), I'm expecting the callback
> >> to happen for each requester ID that could be used for transactions
> >> originated by *Y*.  I'm trying to make an IOMMU mapping for use by
> >> Y, so any devices downstream of Y, e.g., A and B, are irrelevant.
> >
> > Ok, you think of for_each_requester_id() the same as I think of
> > for_each_requester().  Can we split the difference and call it
> > pci_dev_for_each_requester_id()?
> >
> I can only ditto my sentiment above.
> Can I toss in 'pci_get_requester_id()'?!? ... ;-)

I assume pci_get_requester_id() would return just the requester ID for a
given device.  The problem with that is that it doesn't have much
meaning.  What's the requester ID of a bridge?  It depends on whether
the bridge is mastering the transaction (bridge->bus->number << 8 |
bridge->devfn) or whether it's taking ownership of a transaction for a
subordinate device (bridge->secondary->number << 8 | 0).  So we really
want the for-each interface because then we have a set of devices and a
vector through them.

> >> I think a lot of the confusion here is because we're trying to solve
> >> both two questions at once: (1) what requester-IDs need to be mapped
> >> to handle DMA from a device, and (2) what devices can't be isolated
> >> from each other and must be in the same domain.  I think things will
> >> be simpler if we can deal with those separately.  Even if it ends up
> >> being more code, at least each piece will be easier to understand by
> >> itself.
> >
> > Don't we already have this split in the code?
> >
> > (1) pcie_for_each_requester
> >      (or pci_dev_for_each_requester_id)
> >
> > (2) pci_get_visible_pcie_requester
> >      (or pci_get_visible_pcie_requester_id)
> >
> again, unless you understand all of the mapped/coerced/modified request-id
> of PCIe, PCIe-to-PCI(x) bridges, and the potential (RC, other) quirks,
> these interface names are confusing. ....
> ... that could be me, and I need to go back to look at patches and
> the description of the functions, to see if they help to clarify their uses.

Perhaps it's time for a v3, but I'm not sure what all we've learned from
v2 yet or what direction a v3 should take.  We've learned that root
complex PCIe-to-PCI bridges use implementation specific requester IDs
and we know that Intel uses the (bus|devfn) requester ID instead of the
standard (secondary|0).  This should probably be handled as a quirk.
We've renamed pcie_for_each_requester() to
pci_dev_for_each_requester_id().  I don't know if we've renamed
pci_get_visible_pcie_requester() yet, I was just trying to be consistent
with naming by adding _id to the end.  Thanks,

Alex

> > Note however that (2) does not impose anything about domains or
> > isolation, it is strictly based on PCI topology.  It's left to the
> > caller to determine how that translates to IOMMU domains, but the
> > typical case is trivial.  Thanks,
> >
> +1, again.
> 
> > Alex
> >
> 

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

[Don Dutile]

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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Alex Williamson<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   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
>>

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

[Alex Williamson]

On Wed, 2013-07-24 at 16:42 -0400, Don Dutile wrote:
> 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@google.com>
> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> >> ---
> >>   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.

I agree completely, the question is whether we have a PCIe root complex
or a conventional PCI host bus.  I don't think we have any way to tell,
so I'm assuming pci_is_root_bus() indicates we're on a PCIe root complex
and therefore have requester IDs.  If there's some way to determine this
let me know and we can avoid any kind of assumption.

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

Yes

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

I'm referring to (pdev->subordinate->number << 8 | 0) vs
(pdev->bus->number << 8 | pdev->devfn).  The subordinate struct pci_bus
is the secondary interface bus.

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

So this is potentially Intel specific.  How can we tell it's an Intel
root complex?

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

(pdev->bus->number << 8 | pdev->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... :-/

One of the cases Bjorn is referring to is probably the simple case of a
PCIe-to-PCI bridge.  The requester ID is (bridge->subordinate->number <<
8 | 0), which is not an actual device.  As coded here, the function
returns bridge, but requester_id is (bridge->subordinate->number << 8 |
0).

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

That does not sound safe.  Again, this is why I think the
pcie_for_each_requester() works.  Given a requester, call fn() on every
device with the same requester ID.  That means when you have a bridge,
we call it for the bridge and every device and every device quirk behind
the bridge, fully populating context entries for all of them.  Thanks,

Alex

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



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Don Dutile]

On 07/24/2013 05:22 PM, Alex Williamson wrote:
> On Wed, 2013-07-24 at 16:42 -0400, Don Dutile wrote:
>> 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@google.com>
>>>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>>>> ---
>>>>    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.
>
> I agree completely, the question is whether we have a PCIe root complex
> or a conventional PCI host bus.  I don't think we have any way to tell,
> so I'm assuming pci_is_root_bus() indicates we're on a PCIe root complex
> and therefore have requester IDs.  If there's some way to determine this
> let me know and we can avoid any kind of assumption.
>
>>>> +	 * 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).
>
> Yes
>
>>>> + * 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.
>> ^^^^^^^^^^
>
> I'm referring to (pdev->subordinate->number<<  8 | 0) vs
> (pdev->bus->number<<  8 | pdev->devfn).  The subordinate struct pci_bus
> is the secondary interface bus.
>
>> 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... ;-)
>
> So this is potentially Intel specific.  How can we tell it's an Intel
> root complex?
>
Good question. ... another question to ask Intel.... :(

>> aw: which 'bus' do you mean above in  '(bus<<   8 | devfn)' ?
>
> (pdev->bus->number<<  8 | pdev->devfn)
>
k.

>>> 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... :-/
>
> One of the cases Bjorn is referring to is probably the simple case of a
> PCIe-to-PCI bridge.  The requester ID is (bridge->subordinate->number<<
> 8 | 0), which is not an actual device.  As coded here, the function
> returns bridge, but requester_id is (bridge->subordinate->number<<  8 |
> 0).
>
right, that requester-id doesn't map (most likely) to a pdev w/matching BDF.

>>>> +{
>>>> +	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 ???
>
> That does not sound safe.  Again, this is why I think the
> pcie_for_each_requester() works.  Given a requester, call fn() on every
> device with the same requester ID.  That means when you have a bridge,
> we call it for the bridge and every device and every device quirk behind
> the bridge, fully populating context entries for all of them.  Thanks,
>
> Alex
>
ok.

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Bjorn Helgaas]

On Wed, Jul 24, 2013 at 04:42:03PM -0400, Don Dutile wrote:
> 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!).

Generally a device does not have multiple requester IDs, but the
IOMMU may see one of several requester IDs for DMAs from a given
device because bridges may take ownership of those transactions (sec
3.6.1.1 of the VT-d spec).

Just to be clear, I envision this whole interface as being
specifically for use by IOMMU drivers, so I'm only trying to provide
what's necessary to build IOMMU mappings.

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

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

This *might* be true, but I don't think we should rely on it.  For
example:

  00:1c.0 PCIe to PCI bridge to [bus 01]
  01:01.0 PCI endpoint

The bridge will take ownership of DMA transactions from the 01:01.0
endpoint.  An IOMMU on bus 00 will see a bridge-assigned requester
ID of 01:00.0 (subordinate bus number, devfn zero), but there is no
01:00.0 device.

Maybe the rules of conventional PCI require a device zero (I don't
remember), but even if they do, it's ugly to rely on that here
because I don't think device 01:00.0 is relevant to mappings for
device 01:01.0.

Obviously we also have to be aware that 01:00.0 and 01:01.0 can't be
isolated from each other, but I think that issue is separate from
the question of what requester IDs have to be mapped to make 01:01.0
work.

Bjorn

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

[Don Dutile]

On 07/25/2013 01:19 PM, Bjorn Helgaas wrote:
> On Wed, Jul 24, 2013 at 04:42:03PM -0400, Don Dutile wrote:
>> 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!).
>
> Generally a device does not have multiple requester IDs, but the
> IOMMU may see one of several requester IDs for DMAs from a given
> device because bridges may take ownership of those transactions (sec
> 3.6.1.1 of the VT-d spec).
>
> Just to be clear, I envision this whole interface as being
> specifically for use by IOMMU drivers, so I'm only trying to provide
> what's necessary to build IOMMU mappings.
>
This is all just PCIe-PCI bridge spec; the code Alex is proposing handles this case.

>>>> + * 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... :-/
>
>>>> + * 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....
>
> This *might* be true, but I don't think we should rely on it.  For
> example:
>
>    00:1c.0 PCIe to PCI bridge to [bus 01]
>    01:01.0 PCI endpoint
>
> The bridge will take ownership of DMA transactions from the 01:01.0
> endpoint.  An IOMMU on bus 00 will see a bridge-assigned requester
> ID of 01:00.0 (subordinate bus number, devfn zero), but there is no
> 01:00.0 device.
>
Clarification:
I meant that each requester-id must have at least 1 PCI device associated
with it.  when PCIe-PCI(X) bridges are in the path, then iommu-groups
lumps multiple requester-ids into a single group, and all devices behind
the bridges are lumped into that group.
The only time its possible for the requester-id for a device behind a
PCIe-PCI(X) Bridge to not be <secondary-bus>:0.0 is when the device is PCI-X.
I can't find anything in the spec that such a PPB can tell the sw what
requester-id alg it's using (pass PCI-X device's requester-id, or use <sbus>:0.0).
The (vtd) spec does say that all device behind a bridge (PCI-X or PCI) must
be in the same domain, which means same iommu group.
So, comments in the code or reviewers implied a requester-id could exist
w/o a pdev; IMO, the (iommu?) code ought to track/tag req-id -> pdev and vice-versa.

> Maybe the rules of conventional PCI require a device zero (I don't
> remember), but even if they do, it's ugly to rely on that here
> because I don't think device 01:00.0 is relevant to mappings for
> device 01:01.0.
>
no requirement for any device-number on a PCI bus.

> Obviously we also have to be aware that 01:00.0 and 01:01.0 can't be
> isolated from each other, but I think that issue is separate from
> the question of what requester IDs have to be mapped to make 01:01.0
> work.
>
yup.
> Bjorn

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

[Bjorn Helgaas]

On Thu, Jul 25, 2013 at 02:25:04PM -0400, Don Dutile wrote:
> On 07/25/2013 01:19 PM, Bjorn Helgaas wrote:
> >On Wed, Jul 24, 2013 at 04:42:03PM -0400, Don Dutile wrote:
> >>On 07/23/2013 06:35 PM, Bjorn Helgaas wrote:
> >>>On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
> >>>>+ * 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....
> >
> >This *might* be true, but I don't think we should rely on it.  For
> >example:
> >
> >   00:1c.0 PCIe to PCI bridge to [bus 01]
> >   01:01.0 PCI endpoint
> >
> >The bridge will take ownership of DMA transactions from the 01:01.0
> >endpoint.  An IOMMU on bus 00 will see a bridge-assigned requester
> >ID of 01:00.0 (subordinate bus number, devfn zero), but there is no
> >01:00.0 device.
> >
> Clarification:
> I meant that each requester-id must have at least 1 PCI device associated
> with it.

I don't think that's true, as in the example above.  Requester ID
0x0100 has no pci_dev associated with it.  What am I missing?

Maybe you mean that requester ID 0x0100 is associated with pci_dev
01:01.0 in the sense that DMAs from 01:01.0 appear with that ID?
That's true, but I can't think of a reason why we would start with
ID 0x0100 and try to look up 01:01.0 from it.  And of course, if you
*did* try to look up the device, there could be several of them.

Bjorn

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

[Don Dutile]

On 07/26/2013 03:48 PM, Bjorn Helgaas wrote:
> On Thu, Jul 25, 2013 at 02:25:04PM -0400, Don Dutile wrote:
>> On 07/25/2013 01:19 PM, Bjorn Helgaas wrote:
>>> On Wed, Jul 24, 2013 at 04:42:03PM -0400, Don Dutile wrote:
>>>> On 07/23/2013 06:35 PM, Bjorn Helgaas wrote:
>>>>> On Thu, Jul 11, 2013 at 03:03:27PM -0600, Alex Williamson wrote:
>>>>>> + * 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....
>>>
>>> This *might* be true, but I don't think we should rely on it.  For
>>> example:
>>>
>>>    00:1c.0 PCIe to PCI bridge to [bus 01]
>>>    01:01.0 PCI endpoint
>>>
>>> The bridge will take ownership of DMA transactions from the 01:01.0
>>> endpoint.  An IOMMU on bus 00 will see a bridge-assigned requester
>>> ID of 01:00.0 (subordinate bus number, devfn zero), but there is no
>>> 01:00.0 device.
>>>
>> Clarification:
>> I meant that each requester-id must have at least 1 PCI device associated
>> with it.
>
> I don't think that's true, as in the example above.  Requester ID
> 0x0100 has no pci_dev associated with it.  What am I missing?
>
> Maybe you mean that requester ID 0x0100 is associated with pci_dev
> 01:01.0 in the sense that DMAs from 01:01.0 appear with that ID?
yes.
> That's true, but I can't think of a reason why we would start with
> ID 0x0100 and try to look up 01:01.0 from it.  And of course, if you
other than to figure out errant device behavior, or a bug in sw. ;)

> *did* try to look up the device, there could be several of them.
>
> Bjorn
What's most important for the consumers (IOMMUs) is to get all the requester-id's
related to a (p)dev for mapping & unmapping setup & teardown, resp.

[RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Alex Williamson]

This eliminates uses of pci_find_upstream_pcie_bridge() and
incorporates DMA quirks into dma_ops path.

Suggested-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/intel-iommu.c         |  164 ++++++++++++++---------------------
 drivers/iommu/intel_irq_remapping.c |    2 
 2 files changed, 65 insertions(+), 101 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b4f0e28..51488cb 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1675,80 +1675,59 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	return 0;
 }
 
-static int
-domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
-			int translation)
+struct context_mapping_info {
+	struct dmar_domain *domain;
+	int translation;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct context_mapping_info *info = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
 
-	ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-					 pdev->bus->number, pdev->devfn,
-					 translation);
-	if (ret)
-		return ret;
+	return domain_context_mapping_one(info->domain, pci_domain_nr(dev->bus),
+					  bus, devfn, info->translation);
+}
 
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return 0;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = domain_context_mapping_one(domain,
-						 pci_domain_nr(parent->bus),
-						 parent->bus->number,
-						 parent->devfn, translation);
-		if (ret)
-			return ret;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-		return domain_context_mapping_one(domain,
-					pci_domain_nr(tmp->subordinate),
-					tmp->subordinate->number, 0,
-					translation);
-	else /* this is a legacy PCI bridge */
-		return domain_context_mapping_one(domain,
-						  pci_domain_nr(tmp->bus),
-						  tmp->bus->number,
-						  tmp->devfn,
-						  translation);
+static int domain_context_mapping(struct dmar_domain *domain,
+				  struct pci_dev *pdev, int translation)
+{
+	struct context_mapping_info info = {
+		.domain = domain,
+		.translation = translation,
+	};
+
+	return pcie_for_each_requester(pdev, NULL, context_mapping, &info);
+}
+
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
+
+	if (!device_context_mapped(iommu, bus, devfn))
+		return 1; /* stop */
+
+	return 0;
 }
 
 static int domain_context_mapped(struct pci_dev *pdev)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
 	struct intel_iommu *iommu;
+	int ret;
 
-	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
-				pdev->devfn);
+	iommu = device_to_iommu(pci_domain_nr(pdev->bus),
+				pdev->bus->number, pdev->devfn);
 	if (!iommu)
 		return -ENODEV;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return ret;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
+	ret = pcie_for_each_requester(pdev, NULL, is_context_mapped, iommu);
+	if (ret < 0)
 		return ret;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = device_context_mapped(iommu, parent->bus->number,
-					    parent->devfn);
-		if (!ret)
-			return ret;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp))
-		return device_context_mapped(iommu, tmp->subordinate->number,
-					     0);
-	else
-		return device_context_mapped(iommu, tmp->bus->number,
-					     tmp->devfn);
+
+	return (ret == 0);
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */
@@ -1975,6 +1954,7 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
 	struct pci_dev *dev_tmp;
+	u16 requester_id;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
@@ -1986,15 +1966,11 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
 	segment = pci_domain_nr(pdev->bus);
 
-	dev_tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (dev_tmp) {
-		if (pci_is_pcie(dev_tmp)) {
-			bus = dev_tmp->subordinate->number;
-			devfn = 0;
-		} else {
-			bus = dev_tmp->bus->number;
-			devfn = dev_tmp->devfn;
-		}
+	dev_tmp = pci_get_visible_pcie_requester(pdev, NULL, &requester_id);
+	if (dev_tmp && dev_tmp != pdev) {
+		bus = requester_id >> 8;
+		devfn = requester_id & 0xFF;
+
 		spin_lock_irqsave(&device_domain_lock, flags);
 		list_for_each_entry(info, &device_domain_list, global) {
 			if (info->segment == segment &&
@@ -3749,31 +3725,24 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
+
+	iommu_detach_dev(iommu, bus, devfn);
+	return 0;
+}
+
 static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
 					   struct pci_dev *pdev)
 {
-	struct pci_dev *tmp, *parent;
-
 	if (!iommu || !pdev)
 		return;
 
-	/* dependent device detach */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	/* Secondary interface's bus number and devfn 0 */
-	if (tmp) {
-		parent = pdev->bus->self;
-		while (parent != tmp) {
-			iommu_detach_dev(iommu, parent->bus->number,
-					 parent->devfn);
-			parent = parent->bus->self;
-		}
-		if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-			iommu_detach_dev(iommu,
-				tmp->subordinate->number, 0);
-		else /* this is a legacy PCI bridge */
-			iommu_detach_dev(iommu, tmp->bus->number,
-					 tmp->devfn);
-	}
+	/* XXX What if there's something else using his path? */
+	pcie_for_each_requester(pdev, NULL, detach_requester, iommu);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
@@ -4158,7 +4127,7 @@ static int intel_iommu_domain_has_cap(struct iommu_domain *domain,
 static int intel_iommu_add_device(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct pci_dev *bridge, *dma_pdev = NULL;
+	struct pci_dev *dma_pdev = NULL;
 	struct iommu_group *group;
 	int ret;
 
@@ -4166,16 +4135,11 @@ static int intel_iommu_add_device(struct device *dev)
 			     pdev->bus->number, pdev->devfn))
 		return -ENODEV;
 
-	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);
+	dma_pdev = pci_get_visible_pcie_requester(pdev, NULL, NULL);
+	if (!dma_pdev)
+		return -EINVAL;
+
+	dma_pdev = pci_dev_get(dma_pdev);
 
 	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 5b19b2d..31214fe 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -385,7 +385,7 @@ static int set_msi_sid(struct irte *irte, struct pci_dev *dev)
 		return 0;
 	}
 
-	bridge = pci_find_upstream_pcie_bridge(dev);
+	bridge = pci_get_visible_pcie_requester(dev, NULL, NULL);
 	if (bridge) {
 		if (pci_is_pcie(bridge))/* this is a PCIe-to-PCI/PCIX bridge */
 			set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,