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

[John Hubbard <jhubbard@nvidia.com>]

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
>