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:52:50 -0700 |
| |
On 5/22/2018 9:48 AM, Ray Jui wrote: > 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). >
And forgot to mention, the logic in our bootloader to set up 1:1 mapping to work around this issue is chip dependent and only activated for certain revisions of PAXC that has this issue.
>> >> Robin.
| |