Re: [PATCH v3 01/20] lib/scatterlist: add flag for indicating P2PDMA segments in an SGL
From: Jason Gunthorpe <jgg@ziepe.ca>
Date: 2021-09-28 18:33:17
Also in:
linux-iommu, linux-mm, linux-nvme, linux-pci, lkml
On Thu, Sep 16, 2021 at 05:40:41PM -0600, Logan Gunthorpe wrote:
quoted hunk ↗ jump to hunk
Make use of the third free LSB in scatterlist's page_link on 64bit systems. The extra bit will be used by dma_[un]map_sg_p2pdma() to determine when a given SGL segments dma_address points to a PCI bus address. dma_unmap_sg_p2pdma() will need to perform different cleanup when a segment is marked as P2PDMA. Using this bit requires adding an additional dependency on CONFIG_64BIT to CONFIG_PCI_P2PDMA. This should be acceptable as the majority of P2PDMA use cases are restricted to newer root complexes and roughly require the extra address space for memory BARs used in the transactions. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: John Hubbard <jhubbard@nvidia.com> drivers/pci/Kconfig | 2 +- include/linux/scatterlist.h | 50 ++++++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 5 deletions(-)diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 0c473d75e625..90b4bddb3300 100644 +++ b/drivers/pci/Kconfig@@ -163,7 +163,7 @@ config PCI_PASID config PCI_P2PDMA bool "PCI peer-to-peer transfer support" - depends on ZONE_DEVICE + depends on ZONE_DEVICE && 64BIT
Perhaps a comment to explain what the 64bit is doing?
quoted hunk ↗ jump to hunk
select GENERIC_ALLOCATOR help Enableѕ drivers to do PCI peer-to-peer transactions to and fromdiff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 266754a55327..e62b1cf6386f 100644 +++ b/include/linux/scatterlist.h@@ -64,6 +64,21 @@ struct sg_append_table { #define SG_CHAIN 0x01UL #define SG_END 0x02UL +/* + * bit 2 is the third free bit in the page_link on 64bit systems which + * is used by dma_unmap_sg() to determine if the dma_address is a PCI + * bus address when doing P2PDMA. + * Note: CONFIG_PCI_P2PDMA depends on CONFIG_64BIT because of this. + */ + +#ifdef CONFIG_PCI_P2PDMA +#define SG_DMA_PCI_P2PDMA 0x04UL
Add a static_assert(__alignof__(void *) == 8); ?
quoted hunk ↗ jump to hunk
+#else +#define SG_DMA_PCI_P2PDMA 0x00UL +#endif + +#define SG_PAGE_LINK_MASK (SG_CHAIN | SG_END | SG_DMA_PCI_P2PDMA) + /* * We overload the LSB of the page pointer to indicate whether it's * a valid sg entry, or whether it points to the start of a new scatterlist.@@ -72,7 +87,9 @@ struct sg_append_table { #define sg_is_chain(sg) ((sg)->page_link & SG_CHAIN) #define sg_is_last(sg) ((sg)->page_link & SG_END) #define sg_chain_ptr(sg) \ - ((struct scatterlist *) ((sg)->page_link & ~(SG_CHAIN | SG_END))) + ((struct scatterlist *)((sg)->page_link & ~SG_PAGE_LINK_MASK)) + +#define sg_is_dma_pci_p2pdma(sg) ((sg)->page_link & SG_DMA_PCI_P2PDMA)
I've been encouraging people to use static inlines more..
static inline unsigned int __sg_flags(struct scatterlist *sg)
{
return sg->page_link & SG_PAGE_LINK_MASK;
}
static inline bool sg_is_chain(struct scatterlist *sg)
{
return __sg_flags(sg) & SG_CHAIN;
}
static inline bool sg_is_last(struct scatterlist *sg)
{
return __sg_flags(sg) & SG_END;
}
static inline bool sg_is_dma_pci_p2pdma(struct scatterlist *sg)
{
return __sg_flags(sg) & SG_DMA_PCI_P2PDMA;
}
quoted hunk ↗ jump to hunk
/** * sg_assign_page - Assign a given page to an SG entry@@ -86,13 +103,13 @@ struct sg_append_table { **/ static inline void sg_assign_page(struct scatterlist *sg, struct page *page) { - unsigned long page_link = sg->page_link & (SG_CHAIN | SG_END); + unsigned long page_link = sg->page_link & SG_PAGE_LINK_MASK;
I think this should just be '& SG_END', sg_assign_page() doesn't look like it should ever be used on a sg_chain entry, so this is just trying to preserve the end stamp. Anyway, this looks OK Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason