Message-ID: <38d41f97-14d5-e24b-3d19-6c4f96305c58@kernel.org>
Date:   Fri, 2 Jun 2023 21:10:22 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     Kishon Vijay Abraham I <kvijayab@....com>,
        Shunsuke Mie <mie@...l.co.jp>,
        Jingoo Han <jingoohan1@...il.com>
Cc:     Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        Hou Zhiqiang <Zhiqiang.Hou@....com>,
        Frank Li <Frank.Li@....com>, Li Chen <lchen@...arella.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware
 map/unmaping

On 6/2/23 20:39, Kishon Vijay Abraham I wrote:
> 
> 
> On 6/2/2023 5:13 AM, Damien Le Moal wrote:
>> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>>> Hi Shunsuke,
>>>
>>> On 1/13/2023 2:33 PM, Shunsuke Mie wrote:
>>>> Add an align_mem operation to the EPC ops, which function is used to
>>>> pci_epc_map/unmap_addr(). These change to enable mapping for any alignment
>>>> restriction of EPC. The map function maps an aligned memory to include a
>>>> requested memory region.
>>>
>>> I'd prefer all the PCIe address alignment restriction be handled in the
>>> endpoint function drivers and not inside the core layer (esp in map and
>>> unmap calls).
>>
>> That is a really *bad* idea ! Most function drivers should be able to work with
>> any EP controller hardware. Asking these drivers to support all the alignment
>> peculiarities of every possible EP controller is impossible.
> 
> Function drivers already work with various restrictions of EP controller 
> hardware. pci_epc_features was added to provide such restrictions to 
> function drivers. Not sure why it has to be different here.
>>
>>> IMO, get the pci address alignment restriction using pci_epc_features.
>>> And use a bigger size (based on alignment restriction) in
>>> pci_epc_mem_alloc_addr() and access the allocated window using an offset
>>> (based on alignment value). You can add separate helpers if required.
>>
>> That is too simplistic and not enough. Example: Rick and I working on an nvme
>> function driver are facing a lot of issues with the EPC API for mem & mapping
>> management because we have 0 control over the PCI address that the host will
>> use. Alignment is all over the place, and the current EPC memory API
>> restrictions (window size limitations) make it impossible to transparently
>> handle all cases. We endup with NVMe command failures simply because of the API
>> limitations.
> 
> You mean restrictions w.r.t OB window address and not PCIe address?
>>
>> And sure, we can modify that driver to better support the EP controller we are
>> using (rockchip). But we need to support other EP controllers as well. So API
> 
> Every EP controller can provide it's restrictions in pci_epc_features. 
> Unless the alignment is going to change dynamically, don't see a need 
> for adding new epc ops.
> 
> Not sure why the following cannot be handled from function driver?
> 
> From
> 
>         A                    A + S
>          ┌────────────────────────┐
>          │                        │
>          │        OB WIN          │
>          ├────────────────────────┤
> mapping │                        │
>          ▼                  B + S ▼
>        B ┌────────────────────────┐
>          │                        │
>          │       PCI Address      │
>          └────────────────────────┘
> 
> To
> 
> 
>       A   A'│              A + S      A+S+alignment
>        ┌────┼───────────────────┬──────┐
>        │    │                   │      │
>        │    │       OB WIN      │      │
>        ├────┴───────────────────┴──────┤
>        │                               |
>        │                               |
>     B' ▼   B                     B + S ▼
>        ┌────┬──────────────────────────┐
>        │    │                          │
>        │    │     PCI Address          │
>        └────┴──────────────────────────┘
> 
> So the changes in function driver will be
> 1) Get alignment value in epc_features
> 2) pci_epc_mem_alloc_addr()/pci_epc_map_addr() will take into account 
> the alignment value (change in size parameter)
> 3) Access host memory from an offset in the provided 
> pci_epc_mem_alloc_addr().

The problem with all this is that some EP controllers (at least the rockchip for
sure, likely the Cadence one as well) have alignment constraints that depend on
the *host* PCI address (yes, the rockchip driver is still buggy in that respect,
fixes coming, see at the end for the details about the rockchip). The current
API does not allow for that to be gracefully handled and using the epc_features
for that would not work at all.

With this dynamic constraint based on the host PCI address (which the EPF cannot
control), we need EPC core functions that:
1) allocate memory from windows based on the PCI address they will be mapped to
2) Depending on the size of the transfer + the alignment need for a PCI address,
a single memory window may not be enough, so we need the ability to allocate
memory over multiple windows
3) Some nice helpers that avoid that pattern of mem alloc + map pci addr and
simplify them with "map this PCI address for me and tell me the local CPU
address for it, completely hiding any alignment concerns.

>> changes are definitely needed. Working on that. That is not easy as the mapping
>> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>>
>> I do have a patch that does something similar as this one, but at a much higher
>> level with a helper function that gives the function driver the offset into the
>> allocated memory region to use for mapping a particular PCI address. And then
>> this helper is then in turn used into a new pci_epc_map() function which does
>> mem alloc + mapping in one go based on the EPC constraints. That hides all
> 
> pci_epc_map() was added only to perform mapping functionality. I'd 
> prefer it stays that way instead of adding bunch of other things into it.

I am not proposing to add to it or to modify it. That function can remain the
basic one for simple cases. But we need better functions for more complex EPF
functions that need to map potentially large memory areas to random PCI addresses.

What I am proposing is to have more intelligent helpers using the current simple
functions: essentially wrapping pci_epc_mem_alloc_addr()+pci_epc_map_addr() with
pci_epc_map(), and similar for unmap. That would greatly simplify the code of
EPF drivers that constantly need to map/unmap PCI address to serve IOs/transfers
as requested by the host/RP side. Developers would still be free to use the
verbose path if they wish to do so, modulo the mandatory fixes for gracefully
handling alignment and allocation size, for which we need either to modify
pci_epc_mem_alloc_addr() or new functions.

Note about the rk3399 EP controller: it has 1MB memory windows that can be used
to map up to 1MB of PCI address space. This limits comes from the fact that the
mapping controller uses at most the lower 22 bits from the local CPU address as
the lower bits for the PCI address. But this also implies that the offset (the
alignment) into the memory window must be equal to the mask of the PCI address
to map over the number of bits of PCI address that will change over the range of
addresses mapped (the number of bits of address changing over the address range
[PCI_addr .. PCI_addr + mapping_size - 1]).

Notifying this alignment need to an EPF driver can only be done using an API.
Cannot do that with epc_features fields.

-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists