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

[Damien Le Moal]

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@xxxxxxxxxx/
> 
>>
>> 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.


-- 
Damien Le Moal
Western Digital Research