Re: [PATCH INTERNAL 3/3] PCI: iproc: Disable MSI parsing in certain PAXC blocks

[Ray Jui <ray.jui@broadcom.com>]

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.