Re: [PATCH v7 01/21] lib/scatterlist: add flag for indicating P2PDMA segments in an SGL

Robin Murphy
2022-06-29T09:05:57Z

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@deltatee.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
> 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