Re: [PATCH v2 02/18] PCI: endpoint: Introduce pci_epc_map_align()

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

On 4/3/24 21:33, Kishon Vijay Abraham I wrote:
> Hi Damien,
> 
> On 3/30/2024 9:49 AM, Damien Le Moal wrote:
>> Some endpoint controllers have requirements on the alignment of the
>> controller physical memory address that must be used to map a RC PCI
>> address region. For instance, the rockchip endpoint controller uses
>> at most the lower 20 bits of a physical memory address region as the
>> lower bits of an RC PCI address. For mapping a PCI address region of
>> size bytes starting from pci_addr, the exact number of address bits
>> used is the number of address bits changing in the address range
>> [pci_addr..pci_addr + size - 1].
>>
>> For this example, this creates the following constraints:
>> 1) The offset into the controller physical memory allocated for a
>>     mapping depends on the mapping size *and* the starting PCI address
>>     for the mapping.
>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>>     the offset needed into the allocated physical memory, which can end
>>     up being a smaller size than the desired mapping size.
>>
>> Handling these constraints independently of the controller being used in
>> a PCI EP function driver is not possible with the current EPC API as
>> it only provides the ->align field in struct pci_epc_features.
>> Furthermore, this alignment is static and does not depend on a mapping
>> pci address and size.
>>
>> Solve this by introducing the function pci_epc_map_align() and the
>> endpoint controller operation ->map_align to allow endpoint function
>> drivers to obtain the size and the offset into a controller address
>> region that must be used to map an RC PCI address region. The size
>> of the physical address region provided by pci_epc_map_align() can then
>> be used as the size argument for the function pci_epc_mem_alloc_addr().
>> The offset into the allocated controller memory can be used to
>> correctly handle data transfers. Of note is that pci_epc_map_align() may
>> indicate upon return a mapping size that is smaller (but not 0) than the
>> requested PCI address region size. For such case, an endpoint function
>> driver must handle data transfers in fragments.
>>
>> The controller operation ->map_align is optional: controllers that do
>> not have any address alignment constraints for mapping a RC PCI address
>> region do not need to implement this operation. For such controllers,
>> pci_epc_map_align() always returns the mapping size as equal
>> to the requested size and an offset equal to 0.
>>
>> The structure pci_epc_map is introduced to represent a mapping start PCI
>> address, size and the size and offset into the controller memory needed
>> for mapping the PCI address region.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/pci/endpoint/pci-epc-core.c | 66 +++++++++++++++++++++++++++++
>>   include/linux/pci-epc.h             | 33 +++++++++++++++
>>   2 files changed, 99 insertions(+)
>>
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index 754afd115bbd..37758ca91d7f 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -433,6 +433,72 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>   }
>>   EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>>   
>> +/**
>> + * pci_epc_map_align() - Get the offset into and the size of a controller memory
>> + *			 address region needed to map a RC PCI address region
>> + * @epc: the EPC device on which address is allocated
>> + * @func_no: the physical endpoint function number in the EPC device
>> + * @vfunc_no: the virtual endpoint function number in the physical function
>> + * @pci_addr: PCI address to which the physical address should be mapped
>> + * @size: the size of the mapping starting from @pci_addr
>> + * @map: populate here the actual size and offset into the controller memory
>> + *       that must be allocated for the mapping
>> + *
>> + * Invoke the controller map_align operation to obtain the size and the offset
>> + * into a controller address region that must be allocated to map @size
>> + * bytes of the RC PCI address space starting from @pci_addr.
>> + *
>> + * The size of the mapping that can be handled by the controller is indicated
>> + * using the pci_size field of @map. This size may be smaller than the requested
>> + * @size. In such case, the function driver must handle the mapping using
>> + * several fragments. The offset into the controller memory for the effective
>> + * mapping of the @pci_addr..@pci_addr+@map->pci_size address range is indicated
>> + * using the map_ofst field of @map.
>> + */
>> +int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>> +		      u64 pci_addr, size_t size, struct pci_epc_map *map)
>> +{
>> +	const struct pci_epc_features *features;
>> +	size_t mask;
>> +	int ret;
>> +
>> +	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
>> +		return -EINVAL;
>> +
>> +	if (!size || !map)
>> +		return -EINVAL;
>> +
>> +	memset(map, 0, sizeof(*map));
>> +	map->pci_addr = pci_addr;
>> +	map->pci_size = size;
>> +
>> +	if (epc->ops->map_align) {
>> +		mutex_lock(&epc->lock);
>> +		ret = epc->ops->map_align(epc, func_no, vfunc_no, map);
>> +		mutex_unlock(&epc->lock);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Assume a fixed alignment constraint as specified by the controller
>> +	 * features.
>> +	 */
>> +	features = pci_epc_get_features(epc, func_no, vfunc_no);
>> +	if (!features || !features->align) {
>> +		map->map_pci_addr = pci_addr;
>> +		map->map_size = size;
>> +		map->map_ofst = 0;
>> +	}
> 
> The 'align' of pci_epc_features was initially added only to address the 
> inbound ATU constraints. This is also added as comment in [1]. The PCI 
> address restrictions (only fixed alignment constraint) were handled by 
> the host side driver and depends on the connected endpoint device 
> (atleast it was like that for pci_endpoint_test.c [2]).
> So pci-epf-test.c used the 'align' in pci_epc_features only as part of 
> pci_epf_alloc_space().
> 
> Though I have abused 'align' of pci_epc_features in pci-epf-ntb.c using 
> it out of pci_epf_alloc_space(), I think we should keep the 'align' of 
> pci_epc_features only within pci_epf_alloc_space() and controllers with 
> any PCI address restrictions to implement ->map_align(). This could as 
> well be done in a phased manner to let controllers implement 
> ->map_align() and then remove using  pci_epc_features in 
> pci_epc_map_align(). Let me know what you think?

Yep, good idea. I will remove the use of "align" as a default alignment
constraint. For controllers that have a fixed alignment constraint (not
necessarilly epc->features->align), it is trivial to provide a generic helper
function that implements the ->map_align method.


-- 
Damien Le Moal
Western Digital Research