lkml.org 
[lkml]   [2018]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH INTERNAL 3/3] PCI: iproc: Disable MSI parsing in certain PAXC blocks
From
Date
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.

\
 
 \ /
  Last update: 2018-05-22 18:50    [W:0.108 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site