lkml.org 
[lkml]   [2023]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware map/unmaping
    From
    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

    \
     
     \ /
      Last update: 2023-06-02 11:43    [W:3.207 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site