| Message ID | 20210408170123.8788-9-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
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. > > 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. > Hard to properly review this patch by itself, because it doesn't show any callers of the new routine. If you end up shuffling patches and/or refactoring for other reasons, it would be nice if the next version of the series included a caller here. In particular, the new pci_p2pdma_map_state concept is something I want to double-check, to see if it hits any common pitfalls. I'm sure it doesn't, but still. :) Meanwhile, I'll keep working through the series, and come back to this one when I have seen the callers. thanks,
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. > + * @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? > + * > + * 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. > + > + 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? > + * @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? thanks,
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
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. + * @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. + * + * 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; + } + + 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 + * @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. + */ +void pci_p2pdma_map_bus_segment(struct scatterlist *pg_sg, + struct scatterlist *dma_sg) +{ + struct pci_p2pdma_pagemap *pgmap = to_p2p_pgmap(sg_page(pg_sg)->pgmap); + + dma_sg->dma_address = sg_phys(pg_sg) + pgmap->bus_offset; + sg_dma_len(dma_sg) = pg_sg->length; + sg_mark_pci_p2pdma(dma_sg); +} + /** * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store * to enable p2pdma diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h index a06072ac3a52..49e7679403cf 100644 --- a/include/linux/pci-p2pdma.h +++ b/include/linux/pci-p2pdma.h @@ -13,6 +13,12 @@ #include <linux/pci.h> +struct pci_p2pdma_map_state { + struct dev_pagemap *pgmap; + int map; + u64 bus_off; +}; + struct block_device; struct scatterlist; @@ -43,6 +49,11 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); +int pci_p2pdma_map_segment(struct pci_p2pdma_map_state *state, + struct device *dev, struct scatterlist *sg, + unsigned long dma_attrs); +void pci_p2pdma_map_bus_segment(struct scatterlist *pg_sg, + struct scatterlist *dma_sg); int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev, bool *use_p2pdma); ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev, @@ -109,6 +120,16 @@ static inline void pci_p2pdma_unmap_sg_attrs(struct device *dev, unsigned long attrs) { } +static inline int pci_p2pdma_map_segment(struct pci_p2pdma_map_state *state, + struct device *dev, struct scatterlist *sg, + unsigned long dma_attrs) +{ + return 0; +} +static inline void pci_p2pdma_map_bus_segment(struct scatterlist *pg_sg, + struct scatterlist *dma_sg) +{ +} static inline int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev, bool *use_p2pdma) {
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. 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(+)