From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B14A9C43334 for ; Wed, 29 Jun 2022 18:02:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 266C38E001F; Wed, 29 Jun 2022 14:02:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 215A08E0015; Wed, 29 Jun 2022 14:02:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0DDEB8E001F; Wed, 29 Jun 2022 14:02:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id EC38D8E0015 for ; Wed, 29 Jun 2022 14:02:44 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id BDC9535386 for ; Wed, 29 Jun 2022 18:02:44 +0000 (UTC) X-FDA: 79632043848.14.4043EA6 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf16.hostedemail.com (Postfix) with ESMTP id 8FDAF18004A for ; Wed, 29 Jun 2022 18:02:43 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E21051480; Wed, 29 Jun 2022 11:02:42 -0700 (PDT) Received: from [10.57.85.71] (unknown [10.57.85.71]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 050C53F5A1; Wed, 29 Jun 2022 11:02:37 -0700 (PDT) Message-ID: Date: Wed, 29 Jun 2022 19:02:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v7 01/21] lib/scatterlist: add flag for indicating P2PDMA segments in an SGL Content-Language: en-GB To: Logan Gunthorpe , linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, linux-pci@vger.kernel.org, linux-mm@kvack.org, iommu@lists.linux-foundation.org Cc: Stephen Bates , Christoph Hellwig , Dan Williams , Jason Gunthorpe , =?UTF-8?Q?Christian_K=c3=b6nig?= , John Hubbard , Don Dutile , Matthew Wilcox , Daniel Vetter , Minturn Dave B , Jason Ekstrand , Dave Hansen , Xiong Jianxin , Bjorn Helgaas , Ira Weiny , Martin Oliveira , Chaitanya Kulkarni , Ralph Campbell , Chaitanya Kulkarni References: <20220615161233.17527-1-logang@deltatee.com> <20220615161233.17527-2-logang@deltatee.com> From: Robin Murphy In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1656525764; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VJ5giCnL9gW0Wv51Z6KrKnRSp+wk2wjV+WILMkU4Itc=; b=oX7Il4ur7Dg4D/3mB1bqBxi4ADMrX8dYDXqz0CEtUPGpcc3setYX/FRDKjwiW5aVKdPNgT kz81li7GljD5e+2BDsdW20o1QidwQHMRxrlFBfjHphKtN0mtn0sxYmcccTLRSEj8Jy94eB M/sGnKYvLSaNc2YwLhveCEFAqcmMD/4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1656525764; a=rsa-sha256; cv=none; b=jQDDlKkaPbV0UrrYpCUNRH+bd4ZvLvzhLkRelu2QlegLtHyAUAmyDfhQrJNV/kiESvxyjS yItR73rXS/Kt7z05qABfH4WkKrC+k5WjNBCZQi5uHKJpq1GV+pKM5nHSqXsNIHgossWllR ly5n/vShLKkFVMNn8J8ec275o/qz6kQ= X-Stat-Signature: 8eaet6dut4xm7u6n1iziyhgijywk7gm1 X-Rspamd-Queue-Id: 8FDAF18004A X-Rspam-User: Authentication-Results: imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com X-Rspamd-Server: rspam12 X-HE-Tag: 1656525763-778341 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2022-06-29 16:39, Logan Gunthorpe wrote: > > > > On 2022-06-29 03:05, Robin Murphy wrote: >> 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 >>> Reviewed-by: Chaitanya Kulkarni >>> --- >>>   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. > > Yes, I think you misunderstand what this is for. The SG_DMA_BUS_ADDDRESS > flag doesn't mark the segment for the page, but for the dma address. It > cannot be set in sg_assign_page() seeing it's not a property of the page > but a property of the dma_address in the sgl. > > It's not meant for use by regular SG users, it's only meant for use > inside DMA mapping implementations. The purpose is to know whether a > given dma_address in the SGL is a bus address or regular memory because > the two different types must be unmapped differently. We can't rely on > the page because, as you know, many dma_map_sg() the dma_address entry > in the sgl does not map to the same memory as the page. Or to put it > another way: is_pci_p2pdma_page(sg->page) does not imply that > sg->dma_address points to a bus address. > > Does that make sense? Ah, you're quite right, in trying to take in the whole series at once first thing in the morning I did fail to properly grasp that detail, so indeed the sg_assign_page() thing couldn't possibly work, but as I said that's fine anyway. I still think the lifecycle management is a bit off though - equivalently, a bus address doesn't stop being a bus address, so it would seem appropriate to update this flag appropriately whenever sg_dma_address() is assigned to, and not when it isn't. Thanks, Robin.