Re: [PATCH v3 0/7] Improve PCI memory mapping API

[Damien Le Moal <dlemoal@kernel.org>]

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