Date:   Tue, 22 May 2018 09:48:55 -0700
From:   Ray Jui <ray.jui@...adcom.com>
To:     Robin Murphy <robin.murphy@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     poza@...eaurora.org, Bjorn Helgaas <bhelgaas@...gle.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com, linux-pci@...r.kernel.org,
        linux-pci-owner@...r.kernel.org
Subject: Re: [PATCH INTERNAL 3/3] PCI: iproc: Disable MSI parsing in certain
 PAXC blocks

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@...eaurora.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@...adcom.com>
>>>>>> Reviewed-by: Scott Branden <scott.branden@...adcom.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.

Powered by blists - more mailing lists