Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware map/unmaping

[Kishon Vijay Abraham I]

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).

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.

Thanks,
Kishon

> Signed-off-by: Shunsuke Mie <mie@xxxxxxxxxx>
> ---
>   drivers/pci/endpoint/pci-epc-core.c | 57 ++++++++++++++++++++++++-----
>   include/linux/pci-epc.h             | 10 +++--
>   2 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 2542196e8c3d..60d586e05e7d 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -430,8 +430,12 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix);
>    * Invoke to unmap the CPU address from PCI address.
>    */
>   void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> -			phys_addr_t phys_addr)
> +			phys_addr_t phys_addr, void __iomem *virt_addr, size_t size)
>   {
> +	u64 aligned_phys;
> +	void __iomem *aligned_virt;
> +	size_t offset;
> +
>   	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
>   		return;
>   
> @@ -441,9 +445,22 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>   	if (!epc->ops->unmap_addr)
>   		return;
>   
> +	if (epc->ops->align_mem) {
> +		mutex_lock(&epc->lock);
> +		aligned_phys = epc->ops->align_mem(epc, phys_addr, &size);
> +		mutex_unlock(&epc->lock);
> +	} else {
> +		aligned_phys = phys_addr;
> +	}
> +
> +	offset = phys_addr - aligned_phys;
> +	aligned_virt = virt_addr - offset;
> +
>   	mutex_lock(&epc->lock);
> -	epc->ops->unmap_addr(epc, func_no, vfunc_no, phys_addr);
> +	epc->ops->unmap_addr(epc, func_no, vfunc_no, aligned_phys);
>   	mutex_unlock(&epc->lock);
> +
> +	pci_epc_mem_free_addr(epc, aligned_phys, aligned_virt, size);
>   }
>   EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>   
> @@ -458,26 +475,46 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>    *
>    * Invoke to map CPU address with PCI address.
>    */
> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> -		     phys_addr_t phys_addr, u64 pci_addr, size_t size)
> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +		u64 pci_addr, phys_addr_t *phys_addr, size_t size)
>   {
>   	int ret;
> +	u64 aligned_addr;
> +	size_t offset;
> +	void __iomem *virt_addr;
>   
>   	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
>   	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
>   	if (!epc->ops->map_addr)
> -		return 0;
> +		return ERR_PTR(-ENOPTSUPP);
> +
> +	if (epc->ops->align_mem) {
> +		mutex_lock(&epc->lock);
> +		aligned_addr = epc->ops->align_mem(epc, pci_addr, &size);
> +		mutex_unlock(&epc->lock);
> +	} else {
> +		aligned_addr = pci_addr;
> +	}
> +
> +	offset = pci_addr - aligned_addr;
> +
> +	virt_addr = pci_epc_mem_alloc_addr(epc, phys_addr, size);
> +	if (!virt_addr)
> +		return ERR_PTR(-ENOMEM);
>   
>   	mutex_lock(&epc->lock);
> -	ret = epc->ops->map_addr(epc, func_no, vfunc_no, phys_addr, pci_addr,
> -				 size);
> +	ret = epc->ops->map_addr(epc, func_no, vfunc_no, *phys_addr, aligned_addr, size);
>   	mutex_unlock(&epc->lock);
> +	if (ret)
> +		return ERR_PTR(ret);
>   
> -	return ret;
> +	*phys_addr += offset;
> +
> +	return virt_addr + offset;
>   }
>   EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>   
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index a48778e1a4ee..8f29161bce80 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -84,6 +84,7 @@ struct pci_epc_ops {
>   			       phys_addr_t phys_addr, u8 interrupt_num,
>   			       u32 entry_size, u32 *msi_data,
>   			       u32 *msi_addr_offset);
> +	u64	(*align_mem)(struct pci_epc *epc, u64 addr, size_t *size);
>   	int	(*start)(struct pci_epc *epc);
>   	void	(*stop)(struct pci_epc *epc);
>   	const struct pci_epc_features* (*get_features)(struct pci_epc *epc,
> @@ -218,11 +219,12 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>   		    struct pci_epf_bar *epf_bar);
>   void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>   		       struct pci_epf_bar *epf_bar);
> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> -		     phys_addr_t phys_addr,
> -		     u64 pci_addr, size_t size);
> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +			       u64 pci_addr, phys_addr_t *phys_addr,
> +			       size_t size);
>   void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> -			phys_addr_t phys_addr);
> +			phys_addr_t phys_addr, void __iomem *virt_addr,
> +			size_t size);
>   int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>   		    u8 interrupts);
>   int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no);