On 10/4/24 22:13, Niklas Cassel wrote: > On Fri, Oct 04, 2024 at 02:07:35PM +0900, Damien Le Moal wrote: >> This series introduces the new functions pci_epc_map_align(), >> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the >> PCI address mapping alignment constraints of endpoint controllers in a >> controller independent manner. >> >> The issue fixed is that the fixed alignment defined by the "align" field >> of struct pci_epc_features assumes is defined for inbound ATU entries >> (e.g. BARs) and is a fixed value, whereas some controllers need a PCI >> address alignment that depends on the PCI address itself. For instance, >> the rk3399 SoC controller in endpoint mode uses the lower bits of the >> local endpoint memory address as the lower bits for the PCI addresses >> for data transfers. That is, when mapping local memory, one must take >> into account the number of bits of the RC PCI address that change from >> the start address of the mapping. >> >> To fix this, the new endpoint controller method .map_align is introduced >> and called from pci_epc_map_align(). This method is optional and for >> controllers that do not define it, it is assumed that the controller has >> no PCI address constraint. >> >> The functions pci_epc_mem_map() is a helper function which obtains >> mapping information, allocates endpoint controller memory according to >> the mapping size obtained and maps the memory. pci_epc_mem_unmap() >> unmaps and frees the endpoint memory. >> >> This series is organized as follows: >> - Patch 1 tidy up the epc core code >> - Patch 2 improves pci_epc_mem_alloc_addr() >> - Patch 3 and 4 introduce the new map_align endpoint controller method >> and the epc functions pci_epc_mem_map() and pci_epc_mem_unmap(). >> - Patch 5 documents these new functions. >> - Patch 6 modifies the test endpoint function driver to use >> pci_epc_mem_map() and pci_epc_mem_unmap() to illustrate the use of >> these functions. >> - Finally, patch 7 implements the rk3588 endpoint controller driver >> .map_align operation to satisfy that controller 64K PCI address >> alignment constraint. >> >> Changes from v2: >> - Dropped all patches for the rockchip-ep. These patches will be sent >> later as a separate series. >> - Added patch 2 and 5 >> - Added review tags to patch 1 >> >> Changes from v1: >> - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch >> 1. >> - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()" >> (former patch 2 of v1) >> - Various typos cleanups all over. Also fixed some blank space >> indentation. >> - Added review tags > > > I think the cover letter is missing some text on how this series has been > tested. > > In V2 I suggested adding a new option to pcitest.c, so that it doesn't > ensure that buffers are aligned. pci_test will currently use a 4k alignment > by default, and for some PCI device IDs and vendor IDs, it will ensure that > the buffers are aligned to something else. (E.g. for the PCI device ID used > by rk3588, buffers will be aligned to 64K.) > > By adding an --no-alignment option to pci_test, we can ensure that this new > API is actually working. > > Did you perhaps ifdef out all the alignment from pci_endpoint_test.c when > testing? Yes I did. And I also extensively tested using the nvme epf function driver (coming soon !) which has very random PCI addresses for data buffers (e.g. BIOSes and GRUB are happy using on-stack totally unaligned buffers...). > I think that having a --no-alignment option included as part of the series > would give us higher confidence that the API is working as intended. Sure, we can add that as a followup patch. I really would like that series to not be hold up by this though as more stuff depend on it (the nvme epf function driver is one). > > (pci_test would still align buffers by default, and the long term plan is > to remove these eventually, but it would be nice to already have an option > to disable it.) > > > Kind regards, > Niklas -- Damien Le Moal Western Digital Research
