On Fri, Jun 02, 2023 at 09:10:22PM +0900, Damien Le Moal wrote: > 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. > I agree with this new API idea. Handling the alignment restrictions in the EPF core reduces code duplication among the EPF drivers. - Mani > 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 > -- மணிவண்ணன் சதாசிவம்
