On 2022-06-15 17:12, Logan Gunthorpe wrote:
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 a bus address.
The new bit will only be used when CONFIG_PCI_P2PDMA is set; this means
PCI P2PDMA will require CONFIG_64BIT. 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@xxxxxxxxxxxx>
Reviewed-by: Chaitanya Kulkarni <kch@xxxxxxxxxx>
---
drivers/pci/Kconfig | 5 +++++
include/linux/scatterlist.h | 44 ++++++++++++++++++++++++++++++++++++-
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 133c73207782..5cc7cba1941f 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -164,6 +164,11 @@ config PCI_PASID
config PCI_P2PDMA
bool "PCI peer-to-peer transfer support"
depends on ZONE_DEVICE
+ #
+ # The need for the scatterlist DMA bus address flag means PCI P2PDMA
+ # requires 64bit
+ #
+ depends on 64BIT
select GENERIC_ALLOCATOR
help
Enableѕ drivers to do PCI peer-to-peer transactions to and from
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 7ff9d6386c12..6561ca8aead8 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -64,12 +64,24 @@ 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
+ * bus address when doing P2PDMA.
+ */
+#ifdef CONFIG_PCI_P2PDMA
+#define SG_DMA_BUS_ADDRESS 0x04UL
+static_assert(__alignof__(struct page) >= 8);
+#else
+#define SG_DMA_BUS_ADDRESS 0x00UL
+#endif
+
/*
* 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.
* Those low bits are there for everyone! (thanks mason :-)
*/
-#define SG_PAGE_LINK_MASK (SG_CHAIN | SG_END)
+#define SG_PAGE_LINK_MASK (SG_CHAIN | SG_END | SG_DMA_BUS_ADDRESS)
static inline unsigned int __sg_flags(struct scatterlist *sg)
{
@@ -91,6 +103,11 @@ static inline bool sg_is_last(struct scatterlist *sg)
return __sg_flags(sg) & SG_END;
}
+static inline bool sg_is_dma_bus_address(struct scatterlist *sg)
+{
+ return __sg_flags(sg) & SG_DMA_BUS_ADDRESS;
+}
+
/**
* sg_assign_page - Assign a given page to an SG entry
* @sg: SG entry
@@ -245,6 +262,31 @@ static inline void sg_unmark_end(struct scatterlist *sg)
sg->page_link &= ~SG_END;
}
+/**
+ * sg_dma_mark_bus address - Mark the scatterlist entry as a bus address
+ * @sg: SG entryScatterlist
entryScatterlist?
+ *
+ * Description:
+ * Marks the passed in sg entry to indicate that the dma_address is
+ * a bus address and doesn't need to be unmapped.
+ **/
+static inline void sg_dma_mark_bus_address(struct scatterlist *sg)
+{
+ sg->page_link |= SG_DMA_BUS_ADDRESS;
+}
+
+/**
+ * sg_unmark_pci_p2pdma - Unmark the scatterlist entry as a bus address
+ * @sg: SG entryScatterlist
+ *
+ * Description:
+ * Clears the bus address mark.
+ **/
+static inline void sg_dma_unmark_bus_address(struct scatterlist *sg)
+{
+ sg->page_link &= ~SG_DMA_BUS_ADDRESS;
+}
Does this serve any useful purpose? If a page is determined to be device
memory, it's not going to suddenly stop being device memory, and if the
underlying sg is recycled to point elsewhere then sg_assign_page() will
still (correctly) clear this flag anyway. Trying to reason about this
beyond superficial API symmetry - i.e. why exactly would a caller need
to call it, and what would the implications be of failing to do so -
seems to lead straight to confusion.
In fact I'd be inclined to have sg_assign_page() be responsible for
setting the flag automatically as well, and thus not need
sg_dma_mark_bus_address() either, however I can see the argument for
doing it this way round to not entangle the APIs too much, so I don't
have any great objection to that.
Thanks,
Robin.
+
/**
* sg_phys - Return physical address of an sg entry
* @sg: SG entry
