Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware map/unmaping

[Shunsuke Mie <mie@igel.co.jp>]

On 2023/06/02 21:21, Damien Le Moal wrote:
> On 6/2/23 18:42, Shunsuke Mie wrote:
>> Hi Damien,
>>
>> On 2023/06/02 8:43, 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.
>>>
>>>> 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.
>> I think so to.
>>
>> I'm also proposing virtio-console function driver[1]. I suppose the
>> virtio function
>> driver and your nvme function driver are the same in that the spec is
>> defined and
>> host side driver must work as is.
>>
>> [1]
>> https://lore.kernel.org/linux-pci/20230427104428.862643-4-mie@igel.co.jp/
>>
>>> 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
>>> 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
>>> alignment details to the function drivers, which greatlyu simplyfies the code.
>>> But that is not enough as alignment also implies that we have to deal with
>>> boundaries (due to limited window size) and so sometimes endpu failing a mapping
>>> that is too large because the host used a PCI address close to the boundary.
>>> More work is needed to have pci_epc_map() also hide that with tricks like
>>> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
>>> API changes are also needed.
>> Could you submit the your changes if you can?
>>
>> I'd like to solve the current EPC limitation for the mapping in a better
>> way and avoid doing similar work.
> Will try to cleanup my patches and send an RFC next week. Need to rebase,
> cleanup etc. Not sure I can make it soon as I am busy with other things for 6.5
> right now.
>
> You can have a look at the work in progress here:
>
> https://github.com/damien-lemoal/linux/tree/rockpro64_ep_v21
>
> There are a bunch of epf and epc core patches as well as some rockchip driver
> patches. The first half of the patches on top of Linus 6.3 tag patch are already
> in pci-next.

I'd like to see the repo. Thank you for your cooperation.

Best,

Shunsuke