Re: [PATCH 06/16] lib/scatterlist: Add flag for indicating P2PDMA segments in an SGL

John Hubbard
2021-05-02T22:34:30Z

On 4/8/21 10:01 AM, 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.

Totally agree with the CONFIG_64BIT call.

Also, I have failed to find anything wrong with this patch. :)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
--
John Hubbard
NVIDIA

>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
> drivers/pci/Kconfig | 2 +-
> include/linux/scatterlist.h | 49 ++++++++++++++++++++++++++++++++++---
> 2 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..90b4bddb3300 100644
> --- a/drivers/pci/Kconfig
> +++ 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
> 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 6f70572b2938..5525d3ebf36f 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -58,6 +58,21 @@ struct sg_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_PCI_P2PDMA 0x04UL
> +#else
> +#define SG_PCI_P2PDMA 0x00UL
> +#endif
> +
> +#define SG_PAGE_LINK_MASK (SG_CHAIN | SG_END | SG_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.
> @@ -65,8 +80,9 @@ struct sg_table {
> */
> #define sg_is_chain(sg) ((sg)->page_link & SG_CHAIN)
> #define sg_is_last(sg) ((sg)->page_link & SG_END)
> +#define sg_is_pci_p2pdma(sg) ((sg)->page_link & SG_PCI_P2PDMA)
> #define sg_chain_ptr(sg) \
> - ((struct scatterlist *) ((sg)->page_link & ~(SG_CHAIN | SG_END)))
> + ((struct scatterlist *) ((sg)->page_link & ~SG_PAGE_LINK_MASK))
>
> /**
> * sg_assign_page - Assign a given page to an SG entry
> @@ -80,13 +96,13 @@ struct sg_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;
>
> /*
> * In order for the low bit stealing approach to work, pages
> * must be aligned at a 32-bit boundary as a minimum.
> */
> - BUG_ON((unsigned long) page & (SG_CHAIN | SG_END));
> + BUG_ON((unsigned long) page & SG_PAGE_LINK_MASK);
> #ifdef CONFIG_DEBUG_SG
> BUG_ON(sg_is_chain(sg));
> #endif
> @@ -120,7 +136,7 @@ static inline struct page *sg_page(struct scatterlist *sg)
> #ifdef CONFIG_DEBUG_SG
> BUG_ON(sg_is_chain(sg));
> #endif
> - return (struct page *)((sg)->page_link & ~(SG_CHAIN | SG_END));
> + return (struct page *)((sg)->page_link & ~SG_PAGE_LINK_MASK);
> }
>
> /**
> @@ -222,6 +238,31 @@ static inline void sg_unmark_end(struct scatterlist *sg)
> sg->page_link &= ~SG_END;
> }
>
> +/**
> + * sg_mark_pci_p2pdma - Mark the scatterlist entry for PCI p2pdma
> + * @sg: SG entryScatterlist
> + *
> + * Description:
> + * Marks the passed in sg entry to indicate that the dma_address is
> + * a PCI bus address.
> + **/
> +static inline void sg_mark_pci_p2pdma(struct scatterlist *sg)
> +{
> + sg->page_link |= SG_PCI_P2PDMA;
> +}
> +
> +/**
> + * sg_unmark_pci_p2pdma - Unmark the scatterlist entry for PCI p2pdma
> + * @sg: SG entryScatterlist
> + *
> + * Description:
> + * Clears the PCI P2PDMA mark
> + **/
> +static inline void sg_unmark_pci_p2pdma(struct scatterlist *sg)
> +{
> + sg->page_link &= ~SG_PCI_P2PDMA;
> +}
> +
> /**
> * sg_phys - Return physical address of an sg entry
> * @sg: SG entry
>