[RFC PATCH 0/3] Deal with alignment restriction on EP side

[RFC PATCH 0/3] Deal with alignment restriction on EP side

[Shunsuke Mie]

Some PCIe EPC controllers have restriction to map PCIe address space to the
local memory space. The mapping is needed to access memory of other side.
On epf test, RC module prepares an aligned memory, and EP module maps the
region. However, a EP module which emulate a device (e.g. VirtIO, NVMe and
etc) cannot expect that a driver for the device prepares an aligned memory.
So, a EP side should deal with the alignment restriction.

This patchset addresses with the alignment restriction on EP size. A
content as follows:
1. Improve a pci epc unmap/map functions to cover the alignment restriction
with adding epc driver support as EPC ops.
2. Implement the support function for DWC EPC driver.
3. Adapt the pci-epf-test to the map/unmap function updated at first patch.

I tested this changes on RENESAS board has DWC PCIeC.

This is a RFC, and it has patches for testing only. Following changes are
not included yet:
1. Removing alignment codes on RC side completely
2. Adapting map/unmap() changes to pci-epf-ntb/vntb

Best,
Shunsuke

Shunsuke Mie (3):
  PCI: endpoint: support an alignment aware map/unmaping
  PCI: dwc: support align_mem() callback for pci_epc_epc
  PCI: endpoint: support pci_epc_mem_map/unmap API changes

 .../pci/controller/dwc/pcie-designware-ep.c   | 13 +++
 drivers/pci/endpoint/functions/pci-epf-test.c | 89 +++++--------------
 drivers/pci/endpoint/pci-epc-core.c           | 57 +++++++++---
 include/linux/pci-epc.h                       | 10 ++-
 4 files changed, 90 insertions(+), 79 deletions(-)

-- 
2.25.1

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

[Shunsuke Mie]

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.

Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
---
 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);
-- 
2.25.1

[RFC PATCH 2/3] PCI: dwc: support align_mem() callback for pci_epc_epc

[Shunsuke Mie]

DWC PCIe EPC driver has alignment restriction for mapping as
pci->region_align. Use it to align memory.

Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index d06654895eba..7a7d7513b612 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -444,6 +444,18 @@ static void dw_pcie_ep_stop(struct pci_epc *epc)
 	dw_pcie_stop_link(pci);
 }
 
+static u64 dw_pcie_ep_align_mem(struct pci_epc *epc, u64 addr, size_t *size)
+{
+	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	u64 aaddr;
+
+	aaddr = ALIGN_DOWN(addr, pci->region_align);
+	*size += addr - aaddr;
+
+	return aaddr;
+}
+
 static int dw_pcie_ep_start(struct pci_epc *epc)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
@@ -474,6 +486,7 @@ static const struct pci_epc_ops epc_ops = {
 	.set_msix		= dw_pcie_ep_set_msix,
 	.get_msix		= dw_pcie_ep_get_msix,
 	.raise_irq		= dw_pcie_ep_raise_irq,
+	.align_mem		= dw_pcie_ep_align_mem,
 	.start			= dw_pcie_ep_start,
 	.stop			= dw_pcie_ep_stop,
 	.get_features		= dw_pcie_ep_get_features,
-- 
2.25.1

[RFC PATCH 3/3] PCI: endpoint: support pci_epc_mem_map/unmap API changes

[Shunsuke Mie]

The APIs have changed to support non aligned memory mapping on
endpoint. Adapt the new API to pci-epf-test. The API allocate
pci epc memory inside, so remove allocations.

Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 89 +++++--------------
 1 file changed, 24 insertions(+), 65 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 55283d2379a6..73e75591fd81 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -323,37 +323,22 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 	struct pci_epc *epc = epf->epc;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	size_t size = reg->size;
 
-	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
-	if (!src_addr) {
-		dev_err(dev, "Failed to allocate source address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr,
-			       reg->src_addr, reg->size);
-	if (ret) {
+	src_addr = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, reg->src_addr,
+			       &src_phys_addr, size);
+	if (IS_ERR(src_addr)) {
 		dev_err(dev, "Failed to map source address\n");
 		reg->status = STATUS_SRC_ADDR_INVALID;
-		goto err_src_addr;
-	}
-
-	dst_addr = pci_epc_mem_alloc_addr(epc, &dst_phys_addr, reg->size);
-	if (!dst_addr) {
-		dev_err(dev, "Failed to allocate destination address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err_src_map_addr;
+		goto err;
 	}
 
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr,
-			       reg->dst_addr, reg->size);
+	dst_addr = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, reg->dst_addr,
+			       &dst_phys_addr, size);
 	if (ret) {
 		dev_err(dev, "Failed to map destination address\n");
 		reg->status = STATUS_DST_ADDR_INVALID;
-		goto err_dst_addr;
+		goto err_src_map_addr;
 	}
 
 	ktime_get_ts64(&start);
@@ -393,16 +378,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 	pci_epf_test_print_rate("COPY", reg->size, &start, &end, use_dma);
 
 err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
-
-err_dst_addr:
-	pci_epc_mem_free_addr(epc, dst_phys_addr, dst_addr, reg->size);
+	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr, dst_addr, size);
 
 err_src_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr);
-
-err_src_addr:
-	pci_epc_mem_free_addr(epc, src_phys_addr, src_addr, reg->size);
+	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr, src_addr, size);
 
 err:
 	return ret;
@@ -410,7 +389,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 
 static int pci_epf_test_read(struct pci_epf_test *epf_test)
 {
-	int ret;
+	int ret = 0;
 	void __iomem *src_addr;
 	void *buf;
 	u32 crc32;
@@ -424,21 +403,14 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 	struct device *dma_dev = epf->epc->dev.parent;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	size_t size = reg->size;
 
-	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
-	if (!src_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
-			       reg->src_addr, reg->size);
-	if (ret) {
+	src_addr = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no,
+				    reg->src_addr, &phys_addr, size);
+	if (IS_ERR(src_addr)) {
 		dev_err(dev, "Failed to map address\n");
 		reg->status = STATUS_SRC_ADDR_INVALID;
-		goto err_addr;
+		goto err;
 	}
 
 	buf = kzalloc(reg->size, GFP_KERNEL);
@@ -489,10 +461,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 	kfree(buf);
 
 err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
-
-err_addr:
-	pci_epc_mem_free_addr(epc, phys_addr, src_addr, reg->size);
+	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr, src_addr, size);
 
 err:
 	return ret;
@@ -500,7 +469,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 
 static int pci_epf_test_write(struct pci_epf_test *epf_test)
 {
-	int ret;
+	int ret = 0;
 	void __iomem *dst_addr;
 	void *buf;
 	bool use_dma;
@@ -513,21 +482,14 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
 	struct device *dma_dev = epf->epc->dev.parent;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	size_t size = reg->size;
 
-	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
-	if (!dst_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
-			       reg->dst_addr, reg->size);
-	if (ret) {
+	dst_addr = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no,
+				    reg->dst_addr, &phys_addr, size);
+	if (IS_ERR(dst_addr)) {
 		dev_err(dev, "Failed to map address\n");
 		reg->status = STATUS_DST_ADDR_INVALID;
-		goto err_addr;
+		goto err;
 	}
 
 	buf = kzalloc(reg->size, GFP_KERNEL);
@@ -585,10 +547,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
 	kfree(buf);
 
 err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
-
-err_addr:
-	pci_epc_mem_free_addr(epc, phys_addr, dst_addr, reg->size);
+	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr, dst_addr, size);
 
 err:
 	return ret;
-- 
2.25.1

Re: [RFC PATCH 0/3] Deal with alignment restriction on EP side

[Bjorn Helgaas]

On Fri, Jan 13, 2023 at 06:03:47PM +0900, Shunsuke Mie wrote:
> Some PCIe EPC controllers have restriction to map PCIe address space to the
> local memory space. The mapping is needed to access memory of other side.
> On epf test, RC module prepares an aligned memory, and EP module maps the
> region. However, a EP module which emulate a device (e.g. VirtIO, NVMe and
> etc) cannot expect that a driver for the device prepares an aligned memory.
> So, a EP side should deal with the alignment restriction.
> 
> This patchset addresses with the alignment restriction on EP size. A
> content as follows:
> 1. Improve a pci epc unmap/map functions to cover the alignment restriction
> with adding epc driver support as EPC ops.
> 2. Implement the support function for DWC EPC driver.
> 3. Adapt the pci-epf-test to the map/unmap function updated at first patch.
> 
> I tested this changes on RENESAS board has DWC PCIeC.
> 
> This is a RFC, and it has patches for testing only. Following changes are
> not included yet:
> 1. Removing alignment codes on RC side completely
> 2. Adapting map/unmap() changes to pci-epf-ntb/vntb
> 
> Best,
> Shunsuke
> 
> Shunsuke Mie (3):
>   PCI: endpoint: support an alignment aware map/unmaping
>   PCI: dwc: support align_mem() callback for pci_epc_epc
>   PCI: endpoint: support pci_epc_mem_map/unmap API changes

s/unmaping/unmapping/

Capitalize subject lines ("Support ...").

Would be nice to say something more specific than "support ... API
changes."

The last patch seems to be for a test case.  Some previous changes to
it use the "PCI: pci-epf-test" prefix so it's distinct from the
pci-epc-core changes.

>  .../pci/controller/dwc/pcie-designware-ep.c   | 13 +++
>  drivers/pci/endpoint/functions/pci-epf-test.c | 89 +++++--------------
>  drivers/pci/endpoint/pci-epc-core.c           | 57 +++++++++---
>  include/linux/pci-epc.h                       | 10 ++-
>  4 files changed, 90 insertions(+), 79 deletions(-)
> 
> -- 
> 2.25.1
> 

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

[Bjorn Helgaas]

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

Another question below.

> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> ---
>  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.

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

Re: [RFC PATCH 0/3] Deal with alignment restriction on EP side

[Shunsuke Mie]

On 2023/01/18 5:32, Bjorn Helgaas wrote:
> On Fri, Jan 13, 2023 at 06:03:47PM +0900, Shunsuke Mie wrote:
>> Some PCIe EPC controllers have restriction to map PCIe address space to the
>> local memory space. The mapping is needed to access memory of other side.
>> On epf test, RC module prepares an aligned memory, and EP module maps the
>> region. However, a EP module which emulate a device (e.g. VirtIO, NVMe and
>> etc) cannot expect that a driver for the device prepares an aligned memory.
>> So, a EP side should deal with the alignment restriction.
>>
>> This patchset addresses with the alignment restriction on EP size. A
>> content as follows:
>> 1. Improve a pci epc unmap/map functions to cover the alignment restriction
>> with adding epc driver support as EPC ops.
>> 2. Implement the support function for DWC EPC driver.
>> 3. Adapt the pci-epf-test to the map/unmap function updated at first patch.
>>
>> I tested this changes on RENESAS board has DWC PCIeC.
>>
>> This is a RFC, and it has patches for testing only. Following changes are
>> not included yet:
>> 1. Removing alignment codes on RC side completely
>> 2. Adapting map/unmap() changes to pci-epf-ntb/vntb
>>
>> Best,
>> Shunsuke
>>
>> Shunsuke Mie (3):
>>    PCI: endpoint: support an alignment aware map/unmaping
>>    PCI: dwc: support align_mem() callback for pci_epc_epc
>>    PCI: endpoint: support pci_epc_mem_map/unmap API changes
> s/unmaping/unmapping/
>
> Capitalize subject lines ("Support ...").
>
> Would be nice to say something more specific than "support ... API
> changes."
I'll reflect this remarks.
>
> The last patch seems to be for a test case.  Some previous changes to
> it use the "PCI: pci-epf-test" prefix so it's distinct from the
> pci-epc-core changes.
I'll follow the previous changes.
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 13 +++
>>   drivers/pci/endpoint/functions/pci-epf-test.c | 89 +++++--------------
>>   drivers/pci/endpoint/pci-epc-core.c           | 57 +++++++++---
>>   include/linux/pci-epc.h                       | 10 ++-
>>   4 files changed, 90 insertions(+), 79 deletions(-)
>>
>> -- 
>> 2.25.1
>>
Best,

Shunsuke

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

[Shunsuke Mie]

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@igel.co.jp>
>> ---
>>   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

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@igel.co.jp>
> ---
>   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);

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

[Damien Le Moal]

On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
> 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).

That is a really *bad* idea ! Most function drivers should be able to work with
any EP controller hardware. Asking these drivers to support all the alignment
peculiarities of every possible EP controller is impossible.

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

That is too simplistic and not enough. Example: Rick and I working on an nvme
function driver are facing a lot of issues with the EPC API for mem & mapping
management because we have 0 control over the PCI address that the host will
use. Alignment is all over the place, and the current EPC memory API
restrictions (window size limitations) make it impossible to transparently
handle all cases. We endup with NVMe command failures simply because of the API
limitations.

And sure, we can modify that driver to better support the EP controller we are
using (rockchip). But we need to support other EP controllers as well. So API
changes are definitely needed. Working on that. That is not easy as the mapping
API and its semantic impacts data transfers (memcpy_from|toio and DMA).

I do have a patch that does something similar as this one, but at a much higher
level with a helper function that gives the function driver the offset into the
allocated memory region to use for mapping a particular PCI address. And then
this helper is then in turn used into a new pci_epc_map() function which does
mem alloc + mapping in one go based on the EPC constraints. That hides all
alignment details to the function drivers, which greatlyu simplyfies the code.
But that is not enough as alignment also implies that we have to deal with
boundaries (due to limited window size) and so sometimes endpu failing a mapping
that is too large because the host used a PCI address close to the boundary.
More work is needed to have pci_epc_map() also hide that with tricks like
allowing the allocation and mapping of multiple contiguous windows. So EPC ops
API changes are also needed.


> 
> Thanks,
> Kishon
> 
>>
>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
>> ---
>>   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);

-- 
Damien Le Moal
Western Digital Research

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

[Shunsuke Mie]

Hi Damien,

On 2023/06/02 8:43, Damien Le Moal wrote:
> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>> 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).
> That is a really *bad* idea ! Most function drivers should be able to work with
> any EP controller hardware. Asking these drivers to support all the alignment
> peculiarities of every possible EP controller is impossible.
>
>> 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.
> That is too simplistic and not enough. Example: Rick and I working on an nvme
> function driver are facing a lot of issues with the EPC API for mem & mapping
> management because we have 0 control over the PCI address that the host will
> use. Alignment is all over the place, and the current EPC memory API
> restrictions (window size limitations) make it impossible to transparently
> handle all cases. We endup with NVMe command failures simply because of the API
> limitations.

I think so to.

I'm also proposing virtio-console function driver[1]. I suppose the 
virtio function
driver and your nvme function driver are the same in that the spec is 
defined and
host side driver must work as is.

[1] 
https://lore.kernel.org/linux-pci/20230427104428.862643-4-mie@igel.co.jp/

>
> And sure, we can modify that driver to better support the EP controller we are
> using (rockchip). But we need to support other EP controllers as well. So API
> changes are definitely needed. Working on that. That is not easy as the mapping
> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>
> I do have a patch that does something similar as this one, but at a much higher
> level with a helper function that gives the function driver the offset into the
> allocated memory region to use for mapping a particular PCI address. And then
> this helper is then in turn used into a new pci_epc_map() function which does
> mem alloc + mapping in one go based on the EPC constraints. That hides all
> alignment details to the function drivers, which greatlyu simplyfies the code.
> But that is not enough as alignment also implies that we have to deal with
> boundaries (due to limited window size) and so sometimes endpu failing a mapping
> that is too large because the host used a PCI address close to the boundary.
> More work is needed to have pci_epc_map() also hide that with tricks like
> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
> API changes are also needed.

Could you submit the your changes if you can?

I'd like to solve the current EPC limitation for the mapping in a better 
way and avoid doing similar work.

>
>
>> Thanks,
>> Kishon
>>
>>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
>>> ---
>>>    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);

Best regards,

Shunsuke

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

[Kishon Vijay Abraham I]

On 6/2/2023 5:13 AM, Damien Le Moal wrote:
> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>> 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).
> 
> That is a really *bad* idea ! Most function drivers should be able to work with
> any EP controller hardware. Asking these drivers to support all the alignment
> peculiarities of every possible EP controller is impossible.

Function drivers already work with various restrictions of EP controller 
hardware. pci_epc_features was added to provide such restrictions to 
function drivers. Not sure why it has to be different here.
> 
>> 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.
> 
> That is too simplistic and not enough. Example: Rick and I working on an nvme
> function driver are facing a lot of issues with the EPC API for mem & mapping
> management because we have 0 control over the PCI address that the host will
> use. Alignment is all over the place, and the current EPC memory API
> restrictions (window size limitations) make it impossible to transparently
> handle all cases. We endup with NVMe command failures simply because of the API
> limitations.

You mean restrictions w.r.t OB window address and not PCIe address?
> 
> And sure, we can modify that driver to better support the EP controller we are
> using (rockchip). But we need to support other EP controllers as well. So API

Every EP controller can provide it's restrictions in pci_epc_features. 
Unless the alignment is going to change dynamically, don't see a need 
for adding new epc ops.

Not sure why the following cannot be handled from function driver?

From

        A                    A + S
         ┌────────────────────────┐
         │                        │
         │        OB WIN          │
         ├────────────────────────┤
mapping │                        │
         ▼                  B + S ▼
       B ┌────────────────────────┐
         │                        │
         │       PCI Address      │
         └────────────────────────┘

To


      A   A'│              A + S      A+S+alignment
       ┌────┼───────────────────┬──────┐
       │    │                   │      │
       │    │       OB WIN      │      │
       ├────┴───────────────────┴──────┤
       │                               |
       │                               |
    B' ▼   B                     B + S ▼
       ┌────┬──────────────────────────┐
       │    │                          │
       │    │     PCI Address          │
       └────┴──────────────────────────┘

So the changes in function driver will be
1) Get alignment value in epc_features
2) pci_epc_mem_alloc_addr()/pci_epc_map_addr() will take into account 
the alignment value (change in size parameter)
3) Access host memory from an offset in the provided 
pci_epc_mem_alloc_addr().

> changes are definitely needed. Working on that. That is not easy as the mapping
> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
> 
> I do have a patch that does something similar as this one, but at a much higher
> level with a helper function that gives the function driver the offset into the
> allocated memory region to use for mapping a particular PCI address. And then
> this helper is then in turn used into a new pci_epc_map() function which does
> mem alloc + mapping in one go based on the EPC constraints. That hides all

pci_epc_map() was added only to perform mapping functionality. I'd 
prefer it stays that way instead of adding bunch of other things into it.

Thanks,
Kishon

> alignment details to the function drivers, which greatlyu simplyfies the code.
> But that is not enough as alignment also implies that we have to deal with
> boundaries (due to limited window size) and so sometimes endpu failing a mapping
> that is too large because the host used a PCI address close to the boundary.
> More work is needed to have pci_epc_map() also hide that with tricks like
> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
> API changes are also needed.
> 
> 
>>
>> Thanks,
>> Kishon
>>
>>>
>>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
>>> ---
>>>    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);
> 

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

[Damien Le Moal]

On 6/2/23 20:39, Kishon Vijay Abraham I wrote:
> 
> 
> On 6/2/2023 5:13 AM, Damien Le Moal wrote:
>> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>>> 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).
>>
>> That is a really *bad* idea ! Most function drivers should be able to work with
>> any EP controller hardware. Asking these drivers to support all the alignment
>> peculiarities of every possible EP controller is impossible.
> 
> Function drivers already work with various restrictions of EP controller 
> hardware. pci_epc_features was added to provide such restrictions to 
> function drivers. Not sure why it has to be different here.
>>
>>> 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.
>>
>> That is too simplistic and not enough. Example: Rick and I working on an nvme
>> function driver are facing a lot of issues with the EPC API for mem & mapping
>> management because we have 0 control over the PCI address that the host will
>> use. Alignment is all over the place, and the current EPC memory API
>> restrictions (window size limitations) make it impossible to transparently
>> handle all cases. We endup with NVMe command failures simply because of the API
>> limitations.
> 
> You mean restrictions w.r.t OB window address and not PCIe address?
>>
>> And sure, we can modify that driver to better support the EP controller we are
>> using (rockchip). But we need to support other EP controllers as well. So API
> 
> Every EP controller can provide it's restrictions in pci_epc_features. 
> Unless the alignment is going to change dynamically, don't see a need 
> for adding new epc ops.
> 
> Not sure why the following cannot be handled from function driver?
> 
> From
> 
>         A                    A + S
>          ┌────────────────────────┐
>          │                        │
>          │        OB WIN          │
>          ├────────────────────────┤
> mapping │                        │
>          ▼                  B + S ▼
>        B ┌────────────────────────┐
>          │                        │
>          │       PCI Address      │
>          └────────────────────────┘
> 
> To
> 
> 
>       A   A'│              A + S      A+S+alignment
>        ┌────┼───────────────────┬──────┐
>        │    │                   │      │
>        │    │       OB WIN      │      │
>        ├────┴───────────────────┴──────┤
>        │                               |
>        │                               |
>     B' ▼   B                     B + S ▼
>        ┌────┬──────────────────────────┐
>        │    │                          │
>        │    │     PCI Address          │
>        └────┴──────────────────────────┘
> 
> So the changes in function driver will be
> 1) Get alignment value in epc_features
> 2) pci_epc_mem_alloc_addr()/pci_epc_map_addr() will take into account 
> the alignment value (change in size parameter)
> 3) Access host memory from an offset in the provided 
> pci_epc_mem_alloc_addr().

The problem with all this is that some EP controllers (at least the rockchip for
sure, likely the Cadence one as well) have alignment constraints that depend on
the *host* PCI address (yes, the rockchip driver is still buggy in that respect,
fixes coming, see at the end for the details about the rockchip). The current
API does not allow for that to be gracefully handled and using the epc_features
for that would not work at all.

With this dynamic constraint based on the host PCI address (which the EPF cannot
control), we need EPC core functions that:
1) allocate memory from windows based on the PCI address they will be mapped to
2) Depending on the size of the transfer + the alignment need for a PCI address,
a single memory window may not be enough, so we need the ability to allocate
memory over multiple windows
3) Some nice helpers that avoid that pattern of mem alloc + map pci addr and
simplify them with "map this PCI address for me and tell me the local CPU
address for it, completely hiding any alignment concerns.

>> changes are definitely needed. Working on that. That is not easy as the mapping
>> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>>
>> I do have a patch that does something similar as this one, but at a much higher
>> level with a helper function that gives the function driver the offset into the
>> allocated memory region to use for mapping a particular PCI address. And then
>> this helper is then in turn used into a new pci_epc_map() function which does
>> mem alloc + mapping in one go based on the EPC constraints. That hides all
> 
> pci_epc_map() was added only to perform mapping functionality. I'd 
> prefer it stays that way instead of adding bunch of other things into it.

I am not proposing to add to it or to modify it. That function can remain the
basic one for simple cases. But we need better functions for more complex EPF
functions that need to map potentially large memory areas to random PCI addresses.

What I am proposing is to have more intelligent helpers using the current simple
functions: essentially wrapping pci_epc_mem_alloc_addr()+pci_epc_map_addr() with
pci_epc_map(), and similar for unmap. That would greatly simplify the code of
EPF drivers that constantly need to map/unmap PCI address to serve IOs/transfers
as requested by the host/RP side. Developers would still be free to use the
verbose path if they wish to do so, modulo the mandatory fixes for gracefully
handling alignment and allocation size, for which we need either to modify
pci_epc_mem_alloc_addr() or new functions.

Note about the rk3399 EP controller: it has 1MB memory windows that can be used
to map up to 1MB of PCI address space. This limits comes from the fact that the
mapping controller uses at most the lower 22 bits from the local CPU address as
the lower bits for the PCI address. But this also implies that the offset (the
alignment) into the memory window must be equal to the mask of the PCI address
to map over the number of bits of PCI address that will change over the range of
addresses mapped (the number of bits of address changing over the address range
[PCI_addr .. PCI_addr + mapping_size - 1]).

Notifying this alignment need to an EPF driver can only be done using an API.
Cannot do that with epc_features fields.

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/2/23 18:42, Shunsuke Mie wrote:
> Hi Damien,
> 
> On 2023/06/02 8:43, Damien Le Moal wrote:
>> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>>> 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).
>> That is a really *bad* idea ! Most function drivers should be able to work with
>> any EP controller hardware. Asking these drivers to support all the alignment
>> peculiarities of every possible EP controller is impossible.
>>
>>> 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.
>> That is too simplistic and not enough. Example: Rick and I working on an nvme
>> function driver are facing a lot of issues with the EPC API for mem & mapping
>> management because we have 0 control over the PCI address that the host will
>> use. Alignment is all over the place, and the current EPC memory API
>> restrictions (window size limitations) make it impossible to transparently
>> handle all cases. We endup with NVMe command failures simply because of the API
>> limitations.
> 
> I think so to.
> 
> I'm also proposing virtio-console function driver[1]. I suppose the 
> virtio function
> driver and your nvme function driver are the same in that the spec is 
> defined and
> host side driver must work as is.
> 
> [1] 
> https://lore.kernel.org/linux-pci/20230427104428.862643-4-mie@igel.co.jp/
> 
>>
>> And sure, we can modify that driver to better support the EP controller we are
>> using (rockchip). But we need to support other EP controllers as well. So API
>> changes are definitely needed. Working on that. That is not easy as the mapping
>> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>>
>> I do have a patch that does something similar as this one, but at a much higher
>> level with a helper function that gives the function driver the offset into the
>> allocated memory region to use for mapping a particular PCI address. And then
>> this helper is then in turn used into a new pci_epc_map() function which does
>> mem alloc + mapping in one go based on the EPC constraints. That hides all
>> alignment details to the function drivers, which greatlyu simplyfies the code.
>> But that is not enough as alignment also implies that we have to deal with
>> boundaries (due to limited window size) and so sometimes endpu failing a mapping
>> that is too large because the host used a PCI address close to the boundary.
>> More work is needed to have pci_epc_map() also hide that with tricks like
>> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
>> API changes are also needed.
> 
> Could you submit the your changes if you can?
> 
> I'd like to solve the current EPC limitation for the mapping in a better 
> way and avoid doing similar work.

Will try to cleanup my patches and send an RFC next week. Need to rebase,
cleanup etc. Not sure I can make it soon as I am busy with other things for 6.5
right now.

You can have a look at the work in progress here:

https://github.com/damien-lemoal/linux/tree/rockpro64_ep_v21

There are a bunch of epf and epc core patches as well as some rockchip driver
patches. The first half of the patches on top of Linus 6.3 tag patch are already
in pci-next.


-- 
Damien Le Moal
Western Digital Research

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

[Manivannan Sadhasivam]

On Fri, Jun 02, 2023 at 09:10:22PM +0900, Damien Le Moal wrote:
> On 6/2/23 20:39, Kishon Vijay Abraham I wrote:
> > 
> > 
> > On 6/2/2023 5:13 AM, Damien Le Moal wrote:
> >> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
> >>> 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).
> >>
> >> That is a really *bad* idea ! Most function drivers should be able to work with
> >> any EP controller hardware. Asking these drivers to support all the alignment
> >> peculiarities of every possible EP controller is impossible.
> > 
> > Function drivers already work with various restrictions of EP controller 
> > hardware. pci_epc_features was added to provide such restrictions to 
> > function drivers. Not sure why it has to be different here.
> >>
> >>> 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.
> >>
> >> That is too simplistic and not enough. Example: Rick and I working on an nvme
> >> function driver are facing a lot of issues with the EPC API for mem & mapping
> >> management because we have 0 control over the PCI address that the host will
> >> use. Alignment is all over the place, and the current EPC memory API
> >> restrictions (window size limitations) make it impossible to transparently
> >> handle all cases. We endup with NVMe command failures simply because of the API
> >> limitations.
> > 
> > You mean restrictions w.r.t OB window address and not PCIe address?
> >>
> >> And sure, we can modify that driver to better support the EP controller we are
> >> using (rockchip). But we need to support other EP controllers as well. So API
> > 
> > Every EP controller can provide it's restrictions in pci_epc_features. 
> > Unless the alignment is going to change dynamically, don't see a need 
> > for adding new epc ops.
> > 
> > Not sure why the following cannot be handled from function driver?
> > 
> > From
> > 
> >         A                    A + S
> >          ┌────────────────────────┐
> >          │                        │
> >          │        OB WIN          │
> >          ├────────────────────────┤
> > mapping │                        │
> >          ▼                  B + S ▼
> >        B ┌────────────────────────┐
> >          │                        │
> >          │       PCI Address      │
> >          └────────────────────────┘
> > 
> > To
> > 
> > 
> >       A   A'│              A + S      A+S+alignment
> >        ┌────┼───────────────────┬──────┐
> >        │    │                   │      │
> >        │    │       OB WIN      │      │
> >        ├────┴───────────────────┴──────┤
> >        │                               |
> >        │                               |
> >     B' ▼   B                     B + S ▼
> >        ┌────┬──────────────────────────┐
> >        │    │                          │
> >        │    │     PCI Address          │
> >        └────┴──────────────────────────┘
> > 
> > So the changes in function driver will be
> > 1) Get alignment value in epc_features
> > 2) pci_epc_mem_alloc_addr()/pci_epc_map_addr() will take into account 
> > the alignment value (change in size parameter)
> > 3) Access host memory from an offset in the provided 
> > pci_epc_mem_alloc_addr().
> 
> The problem with all this is that some EP controllers (at least the rockchip for
> sure, likely the Cadence one as well) have alignment constraints that depend on
> the *host* PCI address (yes, the rockchip driver is still buggy in that respect,
> fixes coming, see at the end for the details about the rockchip). The current
> API does not allow for that to be gracefully handled and using the epc_features
> for that would not work at all.
> 
> With this dynamic constraint based on the host PCI address (which the EPF cannot
> control), we need EPC core functions that:
> 1) allocate memory from windows based on the PCI address they will be mapped to
> 2) Depending on the size of the transfer + the alignment need for a PCI address,
> a single memory window may not be enough, so we need the ability to allocate
> memory over multiple windows
> 3) Some nice helpers that avoid that pattern of mem alloc + map pci addr and
> simplify them with "map this PCI address for me and tell me the local CPU
> address for it, completely hiding any alignment concerns.
> 
> >> changes are definitely needed. Working on that. That is not easy as the mapping
> >> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
> >>
> >> I do have a patch that does something similar as this one, but at a much higher
> >> level with a helper function that gives the function driver the offset into the
> >> allocated memory region to use for mapping a particular PCI address. And then
> >> this helper is then in turn used into a new pci_epc_map() function which does
> >> mem alloc + mapping in one go based on the EPC constraints. That hides all
> > 
> > pci_epc_map() was added only to perform mapping functionality. I'd 
> > prefer it stays that way instead of adding bunch of other things into it.
> 
> I am not proposing to add to it or to modify it. That function can remain the
> basic one for simple cases. But we need better functions for more complex EPF
> functions that need to map potentially large memory areas to random PCI addresses.
> 
> What I am proposing is to have more intelligent helpers using the current simple
> functions: essentially wrapping pci_epc_mem_alloc_addr()+pci_epc_map_addr() with
> pci_epc_map(), and similar for unmap. That would greatly simplify the code of
> EPF drivers that constantly need to map/unmap PCI address to serve IOs/transfers
> as requested by the host/RP side. Developers would still be free to use the
> verbose path if they wish to do so, modulo the mandatory fixes for gracefully
> handling alignment and allocation size, for which we need either to modify
> pci_epc_mem_alloc_addr() or new functions.
> 

I agree with this new API idea. Handling the alignment restrictions in the EPF
core reduces code duplication among the EPF drivers.

- Mani

> Note about the rk3399 EP controller: it has 1MB memory windows that can be used
> to map up to 1MB of PCI address space. This limits comes from the fact that the
> mapping controller uses at most the lower 22 bits from the local CPU address as
> the lower bits for the PCI address. But this also implies that the offset (the
> alignment) into the memory window must be equal to the mask of the PCI address
> to map over the number of bits of PCI address that will change over the range of
> addresses mapped (the number of bits of address changing over the address range
> [PCI_addr .. PCI_addr + mapping_size - 1]).
> 
> Notifying this alignment need to an EPF driver can only be done using an API.
> Cannot do that with epc_features fields.
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Shunsuke Mie]

On 2023/06/02 21:21, Damien Le Moal wrote:
> On 6/2/23 18:42, Shunsuke Mie wrote:
>> Hi Damien,
>>
>> On 2023/06/02 8:43, Damien Le Moal wrote:
>>> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>>>> 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).
>>> That is a really *bad* idea ! Most function drivers should be able to work with
>>> any EP controller hardware. Asking these drivers to support all the alignment
>>> peculiarities of every possible EP controller is impossible.
>>>
>>>> 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.
>>> That is too simplistic and not enough. Example: Rick and I working on an nvme
>>> function driver are facing a lot of issues with the EPC API for mem & mapping
>>> management because we have 0 control over the PCI address that the host will
>>> use. Alignment is all over the place, and the current EPC memory API
>>> restrictions (window size limitations) make it impossible to transparently
>>> handle all cases. We endup with NVMe command failures simply because of the API
>>> limitations.
>> I think so to.
>>
>> I'm also proposing virtio-console function driver[1]. I suppose the
>> virtio function
>> driver and your nvme function driver are the same in that the spec is
>> defined and
>> host side driver must work as is.
>>
>> [1]
>> https://lore.kernel.org/linux-pci/20230427104428.862643-4-mie@igel.co.jp/
>>
>>> And sure, we can modify that driver to better support the EP controller we are
>>> using (rockchip). But we need to support other EP controllers as well. So API
>>> changes are definitely needed. Working on that. That is not easy as the mapping
>>> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>>>
>>> I do have a patch that does something similar as this one, but at a much higher
>>> level with a helper function that gives the function driver the offset into the
>>> allocated memory region to use for mapping a particular PCI address. And then
>>> this helper is then in turn used into a new pci_epc_map() function which does
>>> mem alloc + mapping in one go based on the EPC constraints. That hides all
>>> alignment details to the function drivers, which greatlyu simplyfies the code.
>>> But that is not enough as alignment also implies that we have to deal with
>>> boundaries (due to limited window size) and so sometimes endpu failing a mapping
>>> that is too large because the host used a PCI address close to the boundary.
>>> More work is needed to have pci_epc_map() also hide that with tricks like
>>> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
>>> API changes are also needed.
>> Could you submit the your changes if you can?
>>
>> I'd like to solve the current EPC limitation for the mapping in a better
>> way and avoid doing similar work.
> Will try to cleanup my patches and send an RFC next week. Need to rebase,
> cleanup etc. Not sure I can make it soon as I am busy with other things for 6.5
> right now.
>
> You can have a look at the work in progress here:
>
> https://github.com/damien-lemoal/linux/tree/rockpro64_ep_v21
>
> There are a bunch of epf and epc core patches as well as some rockchip driver
> patches. The first half of the patches on top of Linus 6.3 tag patch are already
> in pci-next.

I'd like to see the repo. Thank you for your cooperation.

Best,

Shunsuke