lkml.org 
[lkml]   [2021]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 08/16] PCI/P2PDMA: Introduce helpers for dma_map_sg implementations


    On 2021-05-02 6:50 p.m., John Hubbard wrote:
    > On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
    >> Add pci_p2pdma_map_segment() as a helper for simple dma_map_sg()
    >> implementations. It takes an scatterlist segment that must point to a
    >> pci_p2pdma struct page and will map it if the mapping requires a bus
    >> address.
    >>
    >> The return value indicates whether the mapping required a bus address
    >> or whether the caller still needs to map the segment normally. If the
    >> segment should not be mapped, -EREMOTEIO is returned.
    >>
    >> This helper uses a state structure to track the changes to the
    >> pgmap across calls and avoid needing to lookup into the xarray for
    >> every page.
    >>
    >
    > OK, coming back to this patch, after seeing how it is used later in
    > the series...
    >
    >> Also add pci_p2pdma_map_bus_segment() which is useful for IOMMU
    >> dma_map_sg() implementations where the sg segment containing the page
    >> differs from the sg segment containing the DMA address.
    >>
    >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
    >> ---
    >> drivers/pci/p2pdma.c | 65 ++++++++++++++++++++++++++++++++++++++
    >> include/linux/pci-p2pdma.h | 21 ++++++++++++
    >> 2 files changed, 86 insertions(+)
    >>
    >> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
    >> index 38c93f57a941..44ad7664e875 100644
    >> --- a/drivers/pci/p2pdma.c
    >> +++ b/drivers/pci/p2pdma.c
    >> @@ -923,6 +923,71 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
    >> }
    >> EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
    >>
    >> +/**
    >> + * pci_p2pdma_map_segment - map an sg segment determining the mapping type
    >> + * @state: State structure that should be declared on the stack outside of
    >> + * the for_each_sg() loop and initialized to zero.
    >
    > Silly fine point for the docs here: it doesn't actually have to be on
    > the stack, so I don't think you need to write that constraint in the
    > documentation. It just has be be somehow allocated and zeroed.

    Yeah, that's true, but there's really no reason it would ever not be
    allocated on the stack.

    >
    >> + * @dev: DMA device that's doing the mapping operation
    >> + * @sg: scatterlist segment to map
    >> + * @attrs: dma mapping attributes
    >> + *
    >> + * This is a helper to be used by non-iommu dma_map_sg() implementations where
    >> + * the sg segment is the same for the page_link and the dma_address.
    >> + *
    >> + * Attempt to map a single segment in an SGL with the PCI bus address.
    >> + * The segment must point to a PCI P2PDMA page and thus must be
    >> + * wrapped in a is_pci_p2pdma_page(sg_page(sg)) check.
    >
    > Should this be backed up with actual checks in the function, that
    > the prerequisites are met?

    I think that would be unnecessary. All callers are going to call this
    inside an is_pci_p2pdma_page() check, otherwise it would slow down the
    fast path.

    >> + *
    >> + * Returns 1 if the segment was mapped, 0 if the segment should be mapped
    >> + * directly (or through the IOMMU) and -EREMOTEIO if the segment should not
    >> + * be mapped at all.
    >> + */
    >> +int pci_p2pdma_map_segment(struct pci_p2pdma_map_state *state,
    >> + struct device *dev, struct scatterlist *sg,
    >> + unsigned long dma_attrs)
    >> +{
    >> + if (state->pgmap != sg_page(sg)->pgmap) {
    >> + state->pgmap = sg_page(sg)->pgmap;
    >> + state->map = pci_p2pdma_map_type(state->pgmap, dev, dma_attrs);
    >> + state->bus_off = to_p2p_pgmap(state->pgmap)->bus_offset;
    >> + }
    >
    > I'll quote myself from patch 9, because I had a comment there that actually
    > was meant for this patch:
    >
    > Is it worth putting this stuff on the caller's stack? I mean, is there a
    > noticeable performance improvement from caching the state? Because if
    > it's invisible, then simplicity is better. I suspect you're right, and
    > that it *is* worth it, but it's good to know for real.

    Yeah, I responded to this in another email.

    >
    >> +
    >> + switch (state->map) {
    >> + case PCI_P2PDMA_MAP_BUS_ADDR:
    >> + sg->dma_address = sg_phys(sg) + state->bus_off;
    >> + sg_dma_len(sg) = sg->length;
    >> + sg_mark_pci_p2pdma(sg);
    >> + return 1;
    >> + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE:
    >> + return 0;
    >> + default:
    >> + return -EREMOTEIO;
    >> + }
    >> +}
    >> +
    >> +/**
    >> + * pci_p2pdma_map_bus_segment - map an sg segment pre determined to
    >> + * be mapped with PCI_P2PDMA_MAP_BUS_ADDR
    >
    > Or:
    >
    > * pci_p2pdma_map_bus_segment - map an SG segment that is already known
    > * to be mapped with PCI_P2PDMA_MAP_BUS_ADDR
    >
    > Also, should that prerequisite be backed up with checks in the function?

    No, this function only really exists for the needs of iommu_dma_map_sg().

    >> + * @pg_sg: scatterlist segment with the page to map
    >> + * @dma_sg: scatterlist segment to assign a dma address to
    >> + *
    >> + * This is a helper for iommu dma_map_sg() implementations when the
    >> + * segment for the dma address differs from the segment containing the
    >> + * source page.
    >> + *
    >> + * pci_p2pdma_map_type() must have already been called on the pg_sg and
    >> + * returned PCI_P2PDMA_MAP_BUS_ADDR.
    >
    > Another prerequisite, so same question: do you think that the code should
    > also check that this prerequisite is met?

    Again, no, simply because it's this way because of what's required by
    iommu_dma.

    Logan

    \
     
     \ /
      Last update: 2021-05-03 19:16    [W:4.559 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site