[PATCH v3 0/2] Handle Cavium ThunderX2 PCI topology quirk

[PATCH v3 0/2] Handle Cavium ThunderX2 PCI topology quirk

[Jayachandran C]

Hi Bjorn, Alex,

Here is v3 of the patchset to handle the PCIe topology quirk of
Cavium ThunderX2 (previously called Broadcom Vulcan).

The earlier discussions on this can be seen at:
http://www.spinics.net/lists/linux-pci/msg51001.html
https://patchwork.ozlabs.org/patch/582633/ and
https://lists.linuxfoundation.org/pipermail/iommu/2016-June/017681.html

The earlier discussion on this patchset had stalled with a suggestion
that it may be possible to fix up this quirk by handling the issue in
the function argument of pci_for_each_dma_alias(). But at that point
all the ACPI and OF code for SMMU and GIC was to merged, and we did not
have reasonable codebase to make the changes.

For 4.11, I tried to fix it in both the SMMU and the GIC ITS code based
on this suggestion, but after going thru the effort, that does not look
like the right approach. I have the code changes at:
https://github.com/jchandra-cavm/linux/commits/rid-xlate-fixup
if anyone want to look over the code.

The problems with that approach is:
 - of the 14 uses of pci_for_each_dma_alias in the function in the kernel
   tree, I have to fixup 6 callers (which is all but one ofthe callers
   outside x86)
 - 4 of these can be reasonably handled (please see the github repo above),
   but the calls in drivers/irqchip/irq-gic-v3-its-pci-msi.c and
   drivers/iommu/iommu.c cannot be reasonably fixed up.
 - Even without the 2 above two changes I can get it to work for now.
   But pci_for_each_dma_alias does not work as expected on this platform
   and we have to be aware of that for all future uses of the function.
  
For now, I have ruled out that approach, and I have rebased the earlier
patch on to 4.11-rc and submitting again for review. The changes are:

v2>v3:
 - changed device flag name from PCI_DEV_FLAGS_DMA_ALIAS_ROOT to
   PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT
 - updated commit message to make the quirk clearer.

Let me know your comments and suggestions.

Thanks,
JC.

Jayachandran C (2):
  PCI: Add device flag PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT
  PCI: quirks: Fix ThunderX2 dma alias handling

 drivers/pci/quirks.c | 14 ++++++++++++++
 drivers/pci/search.c |  4 ++++
 include/linux/pci.h  |  2 ++
 3 files changed, 20 insertions(+)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/2] PCI: Add device flag PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT

[Jayachandran C]

From: Jayachandran C <jchandra@broadcom.com>

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

The flag will be used to indicate a bridge device which forwards the
address translation requests to the IOMMU, i.e where the interrupt and
DMA requests leave the PCIe hierarchy and go into the system blocks.

Usually this happens at the PCI RC, so this flag is not needed. But
on systems where there are bridges that introduce aliases above the
"real" root bridge, this flag is needed to ensure that the function
pci_for_each_dma_alias() works correctly.

The function pci_for_each_dma_alias() is updated to stop when it see a
bridge with this flag set.

Signed-off-by: Jayachandran C <jnair@caviumnetworks.com>
---
 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 33e0f03..4c6044a 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -60,6 +60,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_BRIDGE_XLATE_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 eb3da1a..3f596ac 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -178,6 +178,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_BRIDGE_XLATE_ROOT = (__force pci_dev_flags_t) (1 << 9),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.7.4

[PATCH v3 2/2] PCI: quirks: Fix ThunderX2 dma alias handling

[Jayachandran C]

From: Jayachandran C <jchandra@broadcom.com>

The Cavium ThunderX2 arm64 SoCs (called Broadcom Vulcan earlier), the PCI
topology is slightly unusual. For a multi-node system, it looks like:

[node level PCI bridges - one per node]
    [SoC PCI devices with MSI-X but no IOMMU]
    [PCI-PCIe "glue" bridges - upto 14, one per real bridge below]
        [PCIe real root ports associated with IOMMU and GICv3 ITS]
            [External PCI devices connected to PCIe links]

The top two levels of bridges should have introduced aliases since they
are PCI and PCI/PCIe bridges, but in the case of ThunderX2 they do not.
In the case of external PCIe devices, the "real" root ports are connected
to the SMMU and the GIC ITS, so there is no aliases. The SoC PCI devices
are directly connected to the GIC ITS and the bridges do not introduce
an alias either.

To handle this quirk, we mark the real PCIe root ports and node level
PCIe bridges with the flag PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT.  With this,
pci_for_each_dma_alias() works correctly for external PCIe devices and
SoC PCI devices.

For the current revision of Cavium ThunderX2, the VendorID and Device ID
are from Broadcom Vulcan (14e4:90XX).

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

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6736836..564a84a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3958,6 +3958,20 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
 
 /*
+ * The IOMMU and interrupt controller on Broadcom Vulcan/Cavium ThunderX2 are
+ * associated not at the root bus, but at a bridge below. This quirk flag
+ * will ensure that the aliases are identified correctly.
+ */
+static void quirk_bridge_cavm_thrx2_pcie_root(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
+				quirk_bridge_cavm_thrx2_pcie_root);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9084,
+				quirk_bridge_cavm_thrx2_pcie_root);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
2.7.4

Re: [PATCH v3 1/2] PCI: Add device flag PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT

[Jayachandran C]

On Wed, Mar 22, 2017 at 08:51:10AM +0000, Jayachandran C wrote:

> From: Jayachandran C <jchandra@broadcom.com>

Looks like I did not fix up the author to my new mail ID. Please ignore
this part.

I can send out a clean revision if needed, until then please drop the
broadcom.com mail id in any reply to avoid bounces. Sorry for the mess up.

Thanks,
JC.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/2] Handle Cavium ThunderX2 PCI topology quirk

[Jon Masters]

On 03/22/2017 04:51 AM, Jayachandran C wrote:
> Hi Bjorn, Alex,
> 
> Here is v3 of the patchset to handle the PCIe topology quirk of
> Cavium ThunderX2 (previously called Broadcom Vulcan).
> 
> The earlier discussions on this can be seen at:
> http://www.spinics.net/lists/linux-pci/msg51001.html
> https://patchwork.ozlabs.org/patch/582633/ and
> https://lists.linuxfoundation.org/pipermail/iommu/2016-June/017681.html
> 
> The earlier discussion on this patchset had stalled with a suggestion
> that it may be possible to fix up this quirk by handling the issue in
> the function argument of pci_for_each_dma_alias(). But at that point
> all the ACPI and OF code for SMMU and GIC was to merged, and we did not
> have reasonable codebase to make the changes.
> 
> For 4.11, I tried to fix it in both the SMMU and the GIC ITS code based
> on this suggestion, but after going thru the effort, that does not look
> like the right approach. I have the code changes at:
> https://github.com/jchandra-cavm/linux/commits/rid-xlate-fixup
> if anyone want to look over the code.
> 
> The problems with that approach is:
>  - of the 14 uses of pci_for_each_dma_alias in the function in the kernel
>    tree, I have to fixup 6 callers (which is all but one ofthe callers
>    outside x86)
>  - 4 of these can be reasonably handled (please see the github repo above),
>    but the calls in drivers/irqchip/irq-gic-v3-its-pci-msi.c and
>    drivers/iommu/iommu.c cannot be reasonably fixed up.
>  - Even without the 2 above two changes I can get it to work for now.
>    But pci_for_each_dma_alias does not work as expected on this platform
>    and we have to be aware of that for all future uses of the function.
>   
> For now, I have ruled out that approach, and I have rebased the earlier
> patch on to 4.11-rc and submitting again for review. The changes are:
> 
> v2>v3:
>  - changed device flag name from PCI_DEV_FLAGS_DMA_ALIAS_ROOT to
>    PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT
>  - updated commit message to make the quirk clearer.
> 
> Let me know your comments and suggestions.

My opinion FWIW is that the quirk you have is one of the least intrusive
ways of handling this. Generally in the case of ARM servers, we have a
difference vs. x86 in that the latter usually have a magic RC at the
top level that everything sits beneath (and then, presumably, Intel
do some magic for multi-socket to fudge things over Q/UPI so that
things look nice and boring to software). On ARM, we're typically
dealing with third party RC IP that's disjoint from other parts of
the SoC. We're certainly in the process of bolstering the specs to
set some expectations and greater guidance around topologies that
we would like to avoid, so I don't see this getting out of hand.

That's my $0.02.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3 0/2] Handle Cavium ThunderX2 PCI topology quirk

[Jayachandran C]

On Wed, Mar 22, 2017 at 11:13:05AM -0400, Jon Masters wrote:
> On 03/22/2017 04:51 AM, Jayachandran C wrote:
> > Hi Bjorn, Alex,
> > 
> > Here is v3 of the patchset to handle the PCIe topology quirk of
> > Cavium ThunderX2 (previously called Broadcom Vulcan).
> > 
> > The earlier discussions on this can be seen at:
> > http://www.spinics.net/lists/linux-pci/msg51001.html
> > https://patchwork.ozlabs.org/patch/582633/ and
> > https://lists.linuxfoundation.org/pipermail/iommu/2016-June/017681.html
> > 
> > The earlier discussion on this patchset had stalled with a suggestion
> > that it may be possible to fix up this quirk by handling the issue in
> > the function argument of pci_for_each_dma_alias(). But at that point
> > all the ACPI and OF code for SMMU and GIC was to merged, and we did not
> > have reasonable codebase to make the changes.
> > 
> > For 4.11, I tried to fix it in both the SMMU and the GIC ITS code based
> > on this suggestion, but after going thru the effort, that does not look
> > like the right approach. I have the code changes at:
> > https://github.com/jchandra-cavm/linux/commits/rid-xlate-fixup
> > if anyone want to look over the code.
> > 
> > The problems with that approach is:
> >  - of the 14 uses of pci_for_each_dma_alias in the function in the kernel
> >    tree, I have to fixup 6 callers (which is all but one ofthe callers
> >    outside x86)
> >  - 4 of these can be reasonably handled (please see the github repo above),
> >    but the calls in drivers/irqchip/irq-gic-v3-its-pci-msi.c and
> >    drivers/iommu/iommu.c cannot be reasonably fixed up.
> >  - Even without the 2 above two changes I can get it to work for now.
> >    But pci_for_each_dma_alias does not work as expected on this platform
> >    and we have to be aware of that for all future uses of the function.
> >   
> > For now, I have ruled out that approach, and I have rebased the earlier
> > patch on to 4.11-rc and submitting again for review. The changes are:
> > 
> > v2>v3:
> >  - changed device flag name from PCI_DEV_FLAGS_DMA_ALIAS_ROOT to
> >    PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT
> >  - updated commit message to make the quirk clearer.
> > 
> > Let me know your comments and suggestions.
> 
> My opinion FWIW is that the quirk you have is one of the least intrusive
> ways of handling this. Generally in the case of ARM servers, we have a
> difference vs. x86 in that the latter usually have a magic RC at the
> top level that everything sits beneath (and then, presumably, Intel
> do some magic for multi-socket to fudge things over Q/UPI so that
> things look nice and boring to software). On ARM, we're typically
> dealing with third party RC IP that's disjoint from other parts of
> the SoC. We're certainly in the process of bolstering the specs to
> set some expectations and greater guidance around topologies that
> we would like to avoid, so I don't see this getting out of hand.
> 

The ACPI specification[1] and the device tree specification[2] have
provisions for mapping RID ranges under an RC to different SMMUs with
an offset to generate StreamIDs. This is also true for the mapping
from RID to ITS and DeviceID. So having multiple SMMUs/ITSs for
devices under the same RC is not really a quirk.

It is also reasonable to have the traversal looking for aliases to
stop at the point where the SMMU or ITS is attached. The quirk flag
is only needed because the information on where the SMMU or ITS is
attached is not available to the pci_for_each_dma_alias implementation.
Usually this is not an issue, but unfortunately on ThunderX2, there
are PCI and PCI/PCIe bridges above this point which causes the
current code to calculate RIDs incorrectly.

Thanks,
JC.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0049b/DEN0049B_IO_Remapping_Table.pdf
[2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel