[PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Jayachandran C]

Add a new flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT to limit the DMA alias
search to go no further than the bridge where the IOMMU is attached.

This has been added to support Broadcom's Vulcan which has the SMMUv3
and GIC ITS associated with an intermediate bridge in the PCI topology.
Traversing to buses above would hit internal glue bridges which will
change the RID.

Update the function pci_for_each_dma_alias() to stop when it see a
bridge with this flag set.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---

Here is v2 of the patch, the previous discussion is at 
http://lists.linuxfoundation.org/pipermail/iommu/2016-February/015668.html

v1->v2 changes:
 - dropped the BAR quirk (not needed)
 - moved from using the 'skip' flag for some bridges to using
   similar approach to stop the traversal at the bridge with
   PCI_DEV_FLAGS_DMA_ALIAS_ROOT

Comments and suggestions are welcome

JC.

 drivers/pci/search.c | 4 ++++
 include/linux/pci.h  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..3ea9c27 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -56,6 +56,10 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 
 		tmp = bus->self;
 
+		/* stop at bridge where translation unit is associated */
+		if (tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_ROOT)
+			return ret;
+
 		/*
 		 * PCIe-to-PCI/X bridges alias transactions from downstream
 		 * devices using the subordinate bus number (PCI Express to
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 932ec74..b6f832b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -176,6 +176,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
 	/* Get VPD from function 0 VPD */
 	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
+	/* a non-root bridge where translation occurs, stop alias search here */
+	PCI_DEV_FLAGS_DMA_ALIAS_ROOT = (__force pci_dev_flags_t) (1 << 9),
 };
 
 enum pci_irq_reroute_variant {
-- 
1.9.1

[PATCH v2 2/2] PCI: Handle Broadcom Vulcan DMA alias calculation quirk

[Jayachandran C]

The Broadcom Vulcan PCI topology is slightly unusual, for a multi-node
system, it looks like:

 [bus 0]
     |
     +--[node 0 PCI bridge 0.0.0]
     |           |
     |        [bus 1]
     |           +---[SoC PCI devices 1.4.x, 1.5.x] ........
     |           +---[Glue bridges    1.a.x, 1.b.x]         \
     |                    |                        .....{node 0 GIC ITS}
     |                    |                       /
     |                    +----[SoC PCIe controller root ports]
     |                                |         \...... {SMMUv3 0..3}
     |                                |
     |                                +---- [external PCI devices]
     +- [node 1 PCI bridge 0.0.1]
     |           |
     |        [bus n - similar to bus 1 above]
     ...

The for_each_dma_alias call for external PCI devices should not go
beyond the PCIe controllers where the SMMU and ITS is associated, going
further above to the glue bridges or the node bridges will find aliases
which are not valid. For internal devices, the association is at the
node level bridge and the alias search should not go further.

To handle this quirk, we mark the SoC PCIe bridges and node PCI bridge
with flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT so that the alias and RID
calculations are correct.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---
 drivers/pci/quirks.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8e67802..62664b5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3734,6 +3734,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * The IOMMU and interrupt controller of Broadcom vulcan are associated
+ * not at the root bus, but at a bridge below. The DMA alias search should
+ * not go above the bridge at which the association exists.
+ */
+static void quirk_bridge_brcm_vulcan_pcie_root(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_ROOT;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
+				quirk_bridge_brcm_vulcan_pcie_root);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9084,
+				quirk_bridge_brcm_vulcan_pcie_root);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.9.1

Re: [PATCH v2 2/2] PCI: Handle Broadcom Vulcan DMA alias calculation quirk

[Bjorn Helgaas]

On Sun, May 08, 2016 at 03:03:21PM +0530, Jayachandran C wrote:
> The Broadcom Vulcan PCI topology is slightly unusual, for a multi-node
> system, it looks like:
> 
>  [bus 0]
>      |
>      +--[node 0 PCI bridge 0.0.0]
>      |           |
>      |        [bus 1]
>      |           +---[SoC PCI devices 1.4.x, 1.5.x] ........
>      |           +---[Glue bridges    1.a.x, 1.b.x]         \
>      |                    |                        .....{node 0 GIC ITS}
>      |                    |                       /
>      |                    +----[SoC PCIe controller root ports]
>      |                                |         \...... {SMMUv3 0..3}
>      |                                |
>      |                                +---- [external PCI devices]
>      +- [node 1 PCI bridge 0.0.1]
>      |           |
>      |        [bus n - similar to bus 1 above]
>      ...
> 
> The for_each_dma_alias call for external PCI devices should not go
> beyond the PCIe controllers where the SMMU and ITS is associated, going
> further above to the glue bridges or the node bridges will find aliases
> which are not valid. For internal devices, the association is at the
> node level bridge and the alias search should not go further.

I like the line of reasoning that we should not iterate higher in the
hierarchy than where the translation hardware is attached.  That seems
like a reasonable thing in general, not just for this hardware.

Is there some generic way to learn where the translation hardware is
attached?  If there is, we could make pci_for_each_dma_alias() use
that information, and maybe we could solve the problem for other
systems as well as for Vulcan.

> To handle this quirk, we mark the SoC PCIe bridges and node PCI bridge
> with flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT so that the alias and RID
> calculations are correct.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
>  drivers/pci/quirks.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 8e67802..62664b5 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3734,6 +3734,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>  
>  /*
> + * The IOMMU and interrupt controller of Broadcom vulcan are associated
> + * not at the root bus, but at a bridge below. The DMA alias search should
> + * not go above the bridge at which the association exists.
> + */
> +static void quirk_bridge_brcm_vulcan_pcie_root(struct pci_dev *pdev)
> +{
> +	pdev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_ROOT;
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
> +				quirk_bridge_brcm_vulcan_pcie_root);
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9084,
> +				quirk_bridge_brcm_vulcan_pcie_root);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>   * class code.  Fix it.
>   */
> -- 
> 1.9.1
> 

Re: [PATCH v2 2/2] PCI: Handle Broadcom Vulcan DMA alias calculation quirk

[Jayachandran C]

[Adding ARM GIC and SMMU maintainers]

On Sat, Jun 11, 2016 at 10:54 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sun, May 08, 2016 at 03:03:21PM +0530, Jayachandran C wrote:
>> The Broadcom Vulcan PCI topology is slightly unusual, for a multi-node
>> system, it looks like:
>>
>>  [bus 0]
>>      |
>>      +--[node 0 PCI bridge 0.0.0]
>>      |           |
>>      |        [bus 1]
>>      |           +---[SoC PCI devices 1.4.x, 1.5.x] ........
>>      |           +---[Glue bridges    1.a.x, 1.b.x]         \
>>      |                    |                        .....{node 0 GIC ITS}
>>      |                    |                       /
>>      |                    +----[SoC PCIe controller root ports]
>>      |                                |         \...... {SMMUv3 0..3}
>>      |                                |
>>      |                                +---- [external PCI devices]
>>      +- [node 1 PCI bridge 0.0.1]
>>      |           |
>>      |        [bus n - similar to bus 1 above]
>>      ...
>>
>> The for_each_dma_alias call for external PCI devices should not go
>> beyond the PCIe controllers where the SMMU and ITS is associated, going
>> further above to the glue bridges or the node bridges will find aliases
>> which are not valid. For internal devices, the association is at the
>> node level bridge and the alias search should not go further.
>
> I like the line of reasoning that we should not iterate higher in the
> hierarchy than where the translation hardware is attached.  That seems
> like a reasonable thing in general, not just for this hardware.
>
> Is there some generic way to learn where the translation hardware is
> attached?  If there is, we could make pci_for_each_dma_alias() use
> that information, and maybe we could solve the problem for other
> systems as well as for Vulcan.

There was discussion on using the return value of the callback function
to stop the alias search when we reach the bridge to which the IOMMU
or MSI controller is attached[1]. There was an iommu patch proposed to
handle this for the device tree case[2], It is possible that we can do
something similar for MSI and extend it to work when ACPI IORT is
used as well.

I will spend some time to see if this can be done.

JC.
[1] http://www.spinics.net/lists/linux-pci/msg51093.html
[2] http://www.spinics.net/lists/arm-kernel/msg508266.html

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Robin Murphy]

On 08/05/16 10:33, Jayachandran C via iommu wrote:
> Add a new flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT to limit the DMA alias
> search to go no further than the bridge where the IOMMU is attached.
>
> This has been added to support Broadcom's Vulcan which has the SMMUv3
> and GIC ITS associated with an intermediate bridge in the PCI topology.
> Traversing to buses above would hit internal glue bridges which will
> change the RID.

Can you not just have the relevant callback function detect the relevant 
node and terminate the walk of its own accord? That's what I was aiming 
for in this patch for the IOMMU setup:

http://article.gmane.org/gmane.linux.kernel.iommu/12456

Is there some flaw in that approach I've missed?

Robin.

> Update the function pci_for_each_dma_alias() to stop when it see a
> bridge with this flag set.
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
>
> Here is v2 of the patch, the previous discussion is at
> http://lists.linuxfoundation.org/pipermail/iommu/2016-February/015668.html
>
> v1->v2 changes:
>   - dropped the BAR quirk (not needed)
>   - moved from using the 'skip' flag for some bridges to using
>     similar approach to stop the traversal at the bridge with
>     PCI_DEV_FLAGS_DMA_ALIAS_ROOT
>
> Comments and suggestions are welcome
>
> JC.
>
>   drivers/pci/search.c | 4 ++++
>   include/linux/pci.h  | 2 ++
>   2 files changed, 6 insertions(+)
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d..3ea9c27 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -56,6 +56,10 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>
>   		tmp = bus->self;
>
> +		/* stop at bridge where translation unit is associated */
> +		if (tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_ROOT)
> +			return ret;
> +
>   		/*
>   		 * PCIe-to-PCI/X bridges alias transactions from downstream
>   		 * devices using the subordinate bus number (PCI Express to
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..b6f832b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>   	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>   	/* Get VPD from function 0 VPD */
>   	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> +	/* a non-root bridge where translation occurs, stop alias search here */
> +	PCI_DEV_FLAGS_DMA_ALIAS_ROOT = (__force pci_dev_flags_t) (1 << 9),
>   };
>
>   enum pci_irq_reroute_variant {
>

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Jayachandran C]

On Mon, May 9, 2016 at 3:40 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/05/16 10:33, Jayachandran C via iommu wrote:
>>
>> Add a new flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT to limit the DMA alias
>> search to go no further than the bridge where the IOMMU is attached.
>>
>> This has been added to support Broadcom's Vulcan which has the SMMUv3
>> and GIC ITS associated with an intermediate bridge in the PCI topology.
>> Traversing to buses above would hit internal glue bridges which will
>> change the RID.
>
>
> Can you not just have the relevant callback function detect the relevant
> node and terminate the walk of its own accord? That's what I was aiming for
> in this patch for the IOMMU setup:
>
> http://article.gmane.org/gmane.linux.kernel.iommu/12456
>
> Is there some flaw in that approach I've missed?

Not flaw as such, but:
 -  We need to support OF as well as ACPI, so the firmware dependent
   approach will not work. The ACPI code does not exist right now, but
   there are patches for this in development.
 - GICv3 ITS also uses the RIDs. I need to replicate the same logic in
   pci/msi.c and irqchip/irq-gic-v3-its-pci-msi.c for MSI. For IOMMU, I
   have to update iommu/arm-smmu-v3.c and in other places in iommu
   code  where pci_for_each_dma_alias is called.
 - since it is non-standard PCIe topology for the processor, a quirk seemed
   to be the right way to handle it.

I am still figuring out the right approach here, so comments are welcome.

JC.

>
>> Update the function pci_for_each_dma_alias() to stop when it see a
>> bridge with this flag set.
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> ---
>>
>> Here is v2 of the patch, the previous discussion is at
>> http://lists.linuxfoundation.org/pipermail/iommu/2016-February/015668.html
>>
>> v1->v2 changes:
>>   - dropped the BAR quirk (not needed)
>>   - moved from using the 'skip' flag for some bridges to using
>>     similar approach to stop the traversal at the bridge with
>>     PCI_DEV_FLAGS_DMA_ALIAS_ROOT
>>
>> Comments and suggestions are welcome
>>
>> JC.
>>
>>   drivers/pci/search.c | 4 ++++
>>   include/linux/pci.h  | 2 ++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>> index a20ce7d..3ea9c27 100644
>> --- a/drivers/pci/search.c
>> +++ b/drivers/pci/search.c
>> @@ -56,6 +56,10 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>>
>>                 tmp = bus->self;
>>
>> +               /* stop at bridge where translation unit is associated */
>> +               if (tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_ROOT)
>> +                       return ret;
>> +
>>                 /*
>>                  * PCIe-to-PCI/X bridges alias transactions from
>> downstream
>>                  * devices using the subordinate bus number (PCI Express
>> to
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 932ec74..b6f832b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>>         PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>>         /* Get VPD from function 0 VPD */
>>         PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
>> +       /* a non-root bridge where translation occurs, stop alias search
>> here */
>> +       PCI_DEV_FLAGS_DMA_ALIAS_ROOT = (__force pci_dev_flags_t) (1 << 9),
>>   };
>>
>>   enum pci_irq_reroute_variant {
>>
>

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Robin Murphy]

On 11/05/16 07:28, Jayachandran C wrote:
> On Mon, May 9, 2016 at 3:40 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 08/05/16 10:33, Jayachandran C via iommu wrote:
>>>
>>> Add a new flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT to limit the DMA alias
>>> search to go no further than the bridge where the IOMMU is attached.
>>>
>>> This has been added to support Broadcom's Vulcan which has the SMMUv3
>>> and GIC ITS associated with an intermediate bridge in the PCI topology.
>>> Traversing to buses above would hit internal glue bridges which will
>>> change the RID.
>>
>>
>> Can you not just have the relevant callback function detect the relevant
>> node and terminate the walk of its own accord? That's what I was aiming for
>> in this patch for the IOMMU setup:
>>
>> http://article.gmane.org/gmane.linux.kernel.iommu/12456
>>
>> Is there some flaw in that approach I've missed?
>
> Not flaw as such, but:
>   -  We need to support OF as well as ACPI, so the firmware dependent
>     approach will not work. The ACPI code does not exist right now, but
>     there are patches for this in development.
>   - GICv3 ITS also uses the RIDs. I need to replicate the same logic in
>     pci/msi.c and irqchip/irq-gic-v3-its-pci-msi.c for MSI. For IOMMU, I
>     have to update iommu/arm-smmu-v3.c and in other places in iommu
>     code  where pci_for_each_dma_alias is called.
>   - since it is non-standard PCIe topology for the processor, a quirk seemed
>     to be the right way to handle it.
>
> I am still figuring out the right approach here, so comments are welcome.

Oh for sure, I didn't mean to imply that that code is a complete 
ready-made solution, just that in general it seems a fairly 
straightforward thing to handle without touching the PCI core:

- We already have to know whichever parent device has the 
msi-map/iommu-map/IORT table.
- We're aware of that parent at the point we're doing the DMA/MSI 
configuration.
- Therefore those operations already have everything they need for their 
callback to be able to stop when it reaches the relevant parent device. 
Doing it in a firmware-agnostic manner is merely an implementation detail.

Since we'll basically be funnelling both MSI and IOMMU RID translation 
through a common code path, there should only need to be be one place to 
handle the DMA alias walk - with that first cut of my PCI/generic IOMMU 
bindings series I didn't try very hard to refactor things - v2 (probably 
post-merge-window now) will be a bit more thorough, now that I have a 
better idea of what thing should look like

Ignore what arm-smmu-v3.c does, because it's wrong anyway. I need to fix 
that in the next version as well, even if it's just calling out directly 
to of_pci_map_rid rather than a proper of_xlate implementation.

Now what I realise I *have* missed is the alias detection in 
iommu_get_group_for_dev, to which the "known parent device" reasoning 
doesn't apply, and phantom aliasing might well muck things up. Thinking 
further, if these upstream bridges exist but don't affect the outgoing 
RID, then might it make sense to quirk them as transparent? (I have no 
actual objection to this patch, though, and at this point I'm just 
chucking ideas about).

Robin.

> JC.
>
>>
>>> Update the function pci_for_each_dma_alias() to stop when it see a
>>> bridge with this flag set.
>>>
>>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>>> ---
>>>
>>> Here is v2 of the patch, the previous discussion is at
>>> http://lists.linuxfoundation.org/pipermail/iommu/2016-February/015668.html
>>>
>>> v1->v2 changes:
>>>    - dropped the BAR quirk (not needed)
>>>    - moved from using the 'skip' flag for some bridges to using
>>>      similar approach to stop the traversal at the bridge with
>>>      PCI_DEV_FLAGS_DMA_ALIAS_ROOT
>>>
>>> Comments and suggestions are welcome
>>>
>>> JC.
>>>
>>>    drivers/pci/search.c | 4 ++++
>>>    include/linux/pci.h  | 2 ++
>>>    2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>>> index a20ce7d..3ea9c27 100644
>>> --- a/drivers/pci/search.c
>>> +++ b/drivers/pci/search.c
>>> @@ -56,6 +56,10 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>>>
>>>                  tmp = bus->self;
>>>
>>> +               /* stop at bridge where translation unit is associated */
>>> +               if (tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_ROOT)
>>> +                       return ret;
>>> +
>>>                  /*
>>>                   * PCIe-to-PCI/X bridges alias transactions from
>>> downstream
>>>                   * devices using the subordinate bus number (PCI Express
>>> to
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 932ec74..b6f832b 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>>>          PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>>>          /* Get VPD from function 0 VPD */
>>>          PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
>>> +       /* a non-root bridge where translation occurs, stop alias search
>>> here */
>>> +       PCI_DEV_FLAGS_DMA_ALIAS_ROOT = (__force pci_dev_flags_t) (1 << 9),
>>>    };
>>>
>>>    enum pci_irq_reroute_variant {
>>>
>>
>

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Jayachandran C]

On Wed, May 11, 2016 at 7:56 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 11/05/16 07:28, Jayachandran C wrote:
>>
>> On Mon, May 9, 2016 at 3:40 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>>>
>>> On 08/05/16 10:33, Jayachandran C via iommu wrote:
>>>>
>>>>
>>>> Add a new flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT to limit the DMA alias
>>>> search to go no further than the bridge where the IOMMU is attached.
>>>>
>>>> This has been added to support Broadcom's Vulcan which has the SMMUv3
>>>> and GIC ITS associated with an intermediate bridge in the PCI topology.
>>>> Traversing to buses above would hit internal glue bridges which will
>>>> change the RID.
>>>
>>>
>>>
>>> Can you not just have the relevant callback function detect the relevant
>>> node and terminate the walk of its own accord? That's what I was aiming
>>> for
>>> in this patch for the IOMMU setup:
>>>
>>> http://article.gmane.org/gmane.linux.kernel.iommu/12456
>>>
>>> Is there some flaw in that approach I've missed?
>>
>>
>> Not flaw as such, but:
>>   -  We need to support OF as well as ACPI, so the firmware dependent
>>     approach will not work. The ACPI code does not exist right now, but
>>     there are patches for this in development.
>>   - GICv3 ITS also uses the RIDs. I need to replicate the same logic in
>>     pci/msi.c and irqchip/irq-gic-v3-its-pci-msi.c for MSI. For IOMMU, I
>>     have to update iommu/arm-smmu-v3.c and in other places in iommu
>>     code  where pci_for_each_dma_alias is called.
>>   - since it is non-standard PCIe topology for the processor, a quirk
>> seemed
>>     to be the right way to handle it.
>>
>> I am still figuring out the right approach here, so comments are welcome.
>
>
> Oh for sure, I didn't mean to imply that that code is a complete ready-made
> solution, just that in general it seems a fairly straightforward thing to
> handle without touching the PCI core:
>
> - We already have to know whichever parent device has the
> msi-map/iommu-map/IORT table.
> - We're aware of that parent at the point we're doing the DMA/MSI
> configuration.
> - Therefore those operations already have everything they need for their
> callback to be able to stop when it reaches the relevant parent device.
> Doing it in a firmware-agnostic manner is merely an implementation detail.
>
> Since we'll basically be funnelling both MSI and IOMMU RID translation
> through a common code path, there should only need to be be one place to
> handle the DMA alias walk - with that first cut of my PCI/generic IOMMU
> bindings series I didn't try very hard to refactor things - v2 (probably
> post-merge-window now) will be a bit more thorough, now that I have a better
> idea of what thing should look like
>
> Ignore what arm-smmu-v3.c does, because it's wrong anyway. I need to fix
> that in the next version as well, even if it's just calling out directly to
> of_pci_map_rid rather than a proper of_xlate implementation.
>
> Now what I realise I *have* missed is the alias detection in
> iommu_get_group_for_dev, to which the "known parent device" reasoning
> doesn't apply, and phantom aliasing might well muck things up. Thinking
> further, if these upstream bridges exist but don't affect the outgoing RID,
> then might it make sense to quirk them as transparent? (I have no actual
> objection to this patch, though, and at this point I'm just chucking ideas
> about).

I had an earlier patch that had a quirk which marked some bridges to
be skipped https://patchwork.ozlabs.org/patch/582633/ which is similar
to the transparent idea. The discussion on that patch seems to indicate
that having a "root" flag may be better.

> Robin.
[...]

JC.

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Jon Masters]

On 05/11/2016 10:26 AM, Robin Murphy wrote:
> (I have no actual objection to this patch, though, and at this point
> I'm just chucking ideas about).

Can I ask what the next steps are here? We're looking for upstream
direction to guide some internal activities and could really do with
understanding how you'd like to solve this one longer term as well as
what interim solution could be acceptable until we get there.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Robin Murphy]

On 23/06/16 06:01, Jon Masters wrote:
> On 05/11/2016 10:26 AM, Robin Murphy wrote:
>> (I have no actual objection to this patch, though, and at this point
>> I'm just chucking ideas about).
>
> Can I ask what the next steps are here? We're looking for upstream
> direction to guide some internal activities and could really do with
> understanding how you'd like to solve this one longer term as well as
> what interim solution could be acceptable until we get there.

Well, for now I'm planning to leave the explicit "terminate the alias 
walk from the callback function" behaviour in the DT-parsing code[1], 
since there doesn't seem any good reason not to. As Bjorn says, though, 
it probably is generally useful for the PCI code to have its own 
knowledge of exactly where DMA can escape the PCI hierarchy - I now 
wonder if we could actually just do that from the DT/IORT code; if 
firmware says a particular bridge/etc. has a relationship with an ITS or 
SMMU, then presumably it's reasonable to infer that DMA can come out of 
it, thus we could inform the PCI code there and then without it having 
to quirk things on its own?

Robin.

[1]:http://article.gmane.org/gmane.linux.kernel.iommu/13932

>
> Jon.
>

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Jon Masters]

Quick reply (sorry about top post): I am just getting online so I might discover that this callback is shared by both - in which case, cool - but if not, we need an interim ACPI solution.

Aside: Current RHEL Server for ARM (RHELSA) explicitly disables SMMUv3 support because (I warned the folks involved more than a year ago), we will only support ACPI/IORT, and as you can imagine, many people would like us to enable support for device passthrough and robustness, not to mention 32-bit devices (32-bit DMA mask). Nonetheless, that will only happen when the upstream kernel has IORT and quirks in place.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On Jun 23, 2016, at 08:04, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> On 23/06/16 06:01, Jon Masters wrote:
>>> On 05/11/2016 10:26 AM, Robin Murphy wrote:
>>> (I have no actual objection to this patch, though, and at this point
>>> I'm just chucking ideas about).
>> 
>> Can I ask what the next steps are here? We're looking for upstream
>> direction to guide some internal activities and could really do with
>> understanding how you'd like to solve this one longer term as well as
>> what interim solution could be acceptable until we get there.
> 
> Well, for now I'm planning to leave the explicit "terminate the alias walk from the callback function" behaviour in the DT-parsing code[1], since there doesn't seem any good reason not to. As Bjorn says, though, it probably is generally useful for the PCI code to have its own knowledge of exactly where DMA can escape the PCI hierarchy - I now wonder if we could actually just do that from the DT/IORT code; if firmware says a particular bridge/etc. has a relationship with an ITS or SMMU, then presumably it's reasonable to infer that DMA can come out of it, thus we could inform the PCI code there and then without it having to quirk things on its own?
> 
> Robin.
> 
> [1]:http://article.gmane.org/gmane.linux.kernel.iommu/13932
> 
>> 
>> Jon.
> 

Re: [PATCH v2 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_DMA_ALIAS_ROOT

[Bjorn Helgaas]

On Thu, Jun 23, 2016 at 01:04:01PM +0100, Robin Murphy wrote:
> On 23/06/16 06:01, Jon Masters wrote:
> >On 05/11/2016 10:26 AM, Robin Murphy wrote:
> >>(I have no actual objection to this patch, though, and at this point
> >>I'm just chucking ideas about).
> >
> >Can I ask what the next steps are here? We're looking for upstream
> >direction to guide some internal activities and could really do with
> >understanding how you'd like to solve this one longer term as well as
> >what interim solution could be acceptable until we get there.
> 
> Well, for now I'm planning to leave the explicit "terminate the
> alias walk from the callback function" behaviour in the DT-parsing
> code[1], since there doesn't seem any good reason not to. As Bjorn
> says, though, it probably is generally useful for the PCI code to
> have its own knowledge of exactly where DMA can escape the PCI
> hierarchy - I now wonder if we could actually just do that from the
> DT/IORT code; if firmware says a particular bridge/etc. has a
> relationship with an ITS or SMMU, then presumably it's reasonable to
> infer that DMA can come out of it, thus we could inform the PCI code
> there and then without it having to quirk things on its own?
> 
> Robin.
> 
> [1]:http://article.gmane.org/gmane.linux.kernel.iommu/13932

Just a reminder that I'm going to be on vacation for about the next
three weeks, so it's not that I'm ignoring this, but it seems like
it's not fully baked quite yet.

Bjorn