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

Shunsuke Mie <mie@xxxxxxxxxx> · Wed, 18 Jan 2023 19:33:51 +0900

On 2023/01/18 5:41, Bjorn Helgaas wrote:
On Fri, Jan 13, 2023 at 06:03:48PM +0900, 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 think this does two things: 1) add the .align_mem() function
pointer, and 2) move the pci_epc_mem_alloc_addr() call into
pci_epc_map_addr().  For 2), I would expect to see
pci_epc_mem_alloc_addr() being *removed* from somewhere else.

Anyway, both are significant and should be mentioned in the commit
log.  Possibly they could even be separate commits: move the
alloc/free first, then add .align_mem().
I understood. I attempt to arrange commits as your mention.

Another question below.

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);
Is there a requirement for multiple implementations of .align_mem()?
There's only one implementation in this series
(dw_pcie_ep_align_mem()), and it only needs pci->region_align.  That
*value* might be DWC-specific, but the concept really isn't, so maybe
there could be a generic function that uses the device-specific value.

That is the correct way, but some handlers require different implementation.

Sorry, this patch could have been misleading. it is my fault. I'll add 
the other

handlers to a next version.


  	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);
--
2.25.1

Best,

Shunsuke







