| Message ID | 20241004050742.140664-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve PCI memory mapping API | expand |
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 s/assumes// > (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 Introduces a small helper which cleans up the 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. Why mention that it implements it for rk3588 ? (And why mention 64K?) Patch 7 is not specific to rk3588. Better to mention that you implement it for all DWC PCIe EP controllers, based on the iATU hardware requirements, read from hardware registers (which is stored in pci->region_align), since that is what the commit does. You should probably also mention that as a result of this series, follow up patches can remove alignment entries in drivers/misc/pci_endpoint_test.c > > 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 I think that you should have mentioned that this fixes quite a serious bug in V2, that was reported here: https://lore.kernel.org/linux-pci/eb580d64-1110-479a-9a0b-c2f1eacd23e7@kernel.org/ > > 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 > > Damien Le Moal (7): > PCI: endpoint: Introduce pci_epc_function_is_valid() > PCI: endpoint: Improve pci_epc_mem_alloc_addr() > PCI: endpoint: Introduce pci_epc_map_align() > PCI: endpoint: Introduce pci_epc_mem_map()/unmap() > PCI: endpoint: Update documentation > PCI: endpoint: test: Use pci_epc_mem_map/unmap() > PCI: dwc: endpoint: Define the .map_align() controller operation > > Documentation/PCI/endpoint/pci-endpoint.rst | 33 ++ > .../pci/controller/dwc/pcie-designware-ep.c | 15 + > drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++--------- > drivers/pci/endpoint/pci-epc-core.c | 213 +++++++--- > drivers/pci/endpoint/pci-epc-mem.c | 9 +- > include/linux/pci-epc.h | 41 ++ > 6 files changed, 453 insertions(+), 230 deletions(-) > > -- > 2.46.2 >
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? 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. (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
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
On Fri, Oct 04, 2024 at 10:25:54PM +0900, Damien Le Moal wrote: > On 10/4/24 22:13, Niklas Cassel wrote: > > On Fri, Oct 04, 2024 at 02:07:35PM +0900, Damien Le Moal wrote: (snip) > > 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 know that you did test using a nvme EPF :) But for anyone reading the cover letter, it wasn't clear how this series was tested, so it would have been nice if the information which you provided above would have been part of the cover letter. Kind regards, Niklas