Messages in this thread | | | Subject | Re: [PATCH INTERNAL 3/3] PCI: iproc: Disable MSI parsing in certain PAXC blocks | From | Ray Jui <> | Date | Tue, 22 May 2018 09:48:55 -0700 |
| |
Hi Robin/Lorenzo,
On 5/21/2018 7:26 AM, Robin Murphy wrote: > On 21/05/18 14:33, Lorenzo Pieralisi wrote: >> [+Robin] >> >> On Fri, May 18, 2018 at 12:48:28PM -0700, Ray Jui wrote: >>> Hi Lorenzo, >>> >>> On 5/18/2018 6:56 AM, Lorenzo Pieralisi wrote: >>>> On Fri, May 18, 2018 at 02:53:41PM +0530, poza@codeaurora.org wrote: >>>>> On 2018-05-17 22:51, Ray Jui wrote: >>>>>> The internal MSI parsing logic in certain revisions of PAXC root >>>>>> complexes does not work properly and can casue corruptions on the >>>>>> writes. They need to be disabled >>>>>> >>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>>> --- >>>>>> drivers/pci/host/pcie-iproc.c | 34 ++++++++++++++++++++++++++++++++-- >>>>>> 1 file changed, 32 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/pci/host/pcie-iproc.c >>>>>> b/drivers/pci/host/pcie-iproc.c >>>>>> index 3c76c5f..b906d80 100644 >>>>>> --- a/drivers/pci/host/pcie-iproc.c >>>>>> +++ b/drivers/pci/host/pcie-iproc.c >>>>>> @@ -1144,10 +1144,22 @@ static int >>>>>> iproc_pcie_paxb_v2_msi_steer(struct >>>>>> iproc_pcie *pcie, u64 msi_addr) >>>>>> return ret; >>>>>> } >>>>>> >>>>>> -static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, >>>>>> u64 >>>>>> msi_addr) >>>>>> +static void iproc_pcie_paxc_v2_msi_steer(struct iproc_pcie *pcie, >>>>>> u64 >>>>>> msi_addr, >>>>>> + bool enable) >>>>>> { >>>>>> u32 val; >>>>>> >>>>>> + if (!enable) { >>>>>> + /* >>>>>> + * Disable PAXC MSI steering. All write transfers will be >>>>>> + * treated as non-MSI transfers >>>>>> + */ >>>>>> + val = iproc_pcie_read_reg(pcie, IPROC_PCIE_MSI_EN_CFG); >>>>>> + val &= ~MSI_ENABLE_CFG; >>>>>> + iproc_pcie_write_reg(pcie, IPROC_PCIE_MSI_EN_CFG, val); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> /* >>>>>> * Program bits [43:13] of address of GITS_TRANSLATER register >>>>>> into >>>>>> * bits [30:0] of the MSI base address register. In fact, in >>>>>> all iProc >>>>>> @@ -1201,7 +1213,7 @@ static int iproc_pcie_msi_steer(struct >>>>>> iproc_pcie >>>>>> *pcie, >>>>>> return ret; >>>>>> break; >>>>>> case IPROC_PCIE_PAXC_V2: >>>>>> - iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr); >>>>>> + iproc_pcie_paxc_v2_msi_steer(pcie, msi_addr, true); >>>>> >>>>> Are you calling iproc_pcie_paxc_v2_msi_steer() anywhere else with >>>>> 'false' ? >>>>> I see its getting called only from one place in current code >>>>> iproc_pcie_msi_steer(). >>>> >>>> It is called below with the false field to disable MSIs. That's >>>> probably >>>> one reason more to create a function to enable/disable MSIs instead of >>>> adding a parameter to iproc_pcie_paxc_v2_msi_steer(). >>> >>> Correct. Thanks for helping to explain. >>> >>>> >>>> Which brings me to the question: what happens to those MSIs writes >>>> when MSIs are disabled according to this patch ? Are they terminated >>>> at the root complex ? >>> >>> Note the PAXC block parses MSI writes from our internally connected >>> endpoints (i.e., an embedded network processor). The PAXC block examines >>> these MSI writes and modifies the memory attributes (to DEVICE) of these >>> data and then send them out to the AXI bus. These MSI writes will >>> then be >>> forwarded to the GIC (e.g., GICv2m, GICv3-ITS from ARM) for further >>> processing. This is saying, PAXC itself does not process these MSI >>> writes. >>> They are processed by the GIC and associated interrupt will be generated >>> form the GIC. PAXC's job is to parse and tag them properly so these MSI >>> writes can reach the GIC, and at the same, reach the GIC at the right >>> order. >>> >>> On some of these problematic PAXCs, we are being forced to disable >>> this PAXC >>> internal parsing logic. In this case, we set up static mapping with the >>> IOMMU to modify the memory attributes of these MSI writes. These MSI >>> writes >>> are always designated towards a specific memory address (e.g., on the >>> GICv3 >>> based system, it's the address of the translator register), and >>> that's why >>> static mapping table can be set up to work around this. >> >> Which means, I presume, that there must be some code that somehow >> somewhere in the kernel sets-up those mappings and it has to be related >> to this patch, in which case I wonder why we enable the PAXC steering at >> all given that this (hack) can be done through the IOMMU (and I assume >> that on all PAXC platforms that do need MSIs there is an IOMMU IP >> upstream the root bridge, otherwise I have no idea what will happen to >> those MSI writes). > > If it's only rewriting memory attributes (FWIW it sounds like this thing > is comparable to the AXI translation table of the PLDA root complex we > have in Juno), then if the IOMMU is controlled by Linux the PAXC > shouldn't be needed anyway. In that situation the GICv2m/ITS doorbell > will be already mapped for the endpoint as device memory (and usually at > some arbitrary address that the PAXC won't recognise anyway) - see > iommu_dma_get_msi_page(). > > If Linux *doesn't* know about the IOMMU, then the firmware should be > free to set it up with a static 1:1 mapping of the PA space and leave it > that way, provided Linux can't inadvertently stomp on those page tables > later.
Right, in our case, we had our ATF bootloader set up 1:1 mapping for this. In this case, PAXC internal MSI parsing is completely disabled (which is what this patch does for those PAXC blocks that have this issue).
> > Robin.
| |