On Thu, Sep 16, 2021 at 05:40:41PM -0600, 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 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@xxxxxxxxxxxx>
> Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
> 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?
> 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 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);
?
> +#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;
}
> /**
> * 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@xxxxxxxxxx>
Jason
