Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware map/unmaping
From: Shunsuke Mie
Date: Mon Jun 05 2023 - 06:35:29 EST
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@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.
I'd like to see the repo. Thank you for your cooperation.
Best,
Shunsuke