| Message ID | 20230113090350.1103494-2-mie@igel.co.jp |
|---|---|
| State | New |
| Headers | show |
| Series | Deal with alignment restriction on EP side | expand |
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 >
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
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);
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);
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
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); >
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.
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.
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 >
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
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);
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(-)