Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp4724857pxv; Tue, 6 Jul 2021 07:44:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzHNq1syzE//j7z8xss6rakEGF1BNG5l0Mm8dQlwp4jN1pqtSEHpzsiwUbYkoEjVmYPbgZM X-Received: by 2002:a17:906:31d4:: with SMTP id f20mr18319351ejf.383.1625582668491; Tue, 06 Jul 2021 07:44:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1625582668; cv=none; d=google.com; s=arc-20160816; b=uiA/hWc5h/DCPy6G38uFr6BksdMoKQZR0fqugV88khmaxtacBFkL1qj9DxpLHuB+cN pR411X0D0iDhO8s6wrhyTHr6pIcrt03i2onmVGBIlz7PYpj9j2lILcqUsfzEwFY6Z/KH Qmx0znnd53m4mNiP42BriK7ukRfLVQpW50tAFwlE65WcsNCYlcRHQhx/gWqR1ZdVKr0w 9geLFHq1uqMLmqF6LlnGQGE5C4MaLN3o3823bUKmJCRTrLHyE14kFZVFtc9I2opiDmMo sf8j1CxLZ9SMzqrDSuzYtF1GWcn7R9CTiXZiAfWuDIUos01fqDh9fLqygbAiz4fMYTti tjTQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=O4dORczB/lRqfhJdDnrpv6zVHBc5kMZJ0QwoUFeAn8U=; b=qe5kf3XYnvY7ArbEeNVhB8t0z7To5rOvlDEvFJZ56ENTL6luUYk4Zn0DWfJX8uv2uH ZO0mbKvyigzCSsyNLonhM9lCaDOu9yeGVpvYP3mqT4zn+Y5riIfT88utWOddVZdzPn/5 JJmWxdtHiNkFER1zydGfgjZlTYSKpwV8TyDY9EZkp+hXVYkQegqG7TtaOMoJp7zn1Yak 6iwpUNB6imWZhLInjT3iYuCZ+kytfuRq71ms04UCI3lHj8FHrVOFDLvk0YgQTATycXEP 12gSThMZ7SDKv6+ocxz76pQh9iJjPRY0NJLjoPy19skVP0T2CsFRPEqPKwe8/KuPrJYi Cg/g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id f25si14025203edu.543.2021.07.06.07.44.05; Tue, 06 Jul 2021 07:44:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232529AbhGFOm4 (ORCPT + 99 others); Tue, 6 Jul 2021 10:42:56 -0400 Received: from foss.arm.com ([217.140.110.172]:43808 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232527AbhGFOmx (ORCPT ); Tue, 6 Jul 2021 10:42:53 -0400 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 6E5E031B; Tue, 6 Jul 2021 07:01:18 -0700 (PDT) Received: from [10.57.40.45] (unknown [10.57.40.45]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 461163F73B; Tue, 6 Jul 2021 07:01:11 -0700 (PDT) Subject: Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing To: Will Deacon , Christoph Hellwig Cc: heikki.krogerus@linux.intel.com, thomas.hellstrom@linux.intel.com, peterz@infradead.org, benh@kernel.crashing.org, joonas.lahtinen@linux.intel.com, dri-devel@lists.freedesktop.org, chris@chris-wilson.co.uk, grant.likely@arm.com, paulus@samba.org, Frank Rowand , mingo@kernel.org, Stefano Stabellini , Saravana Kannan , mpe@ellerman.id.au, "Rafael J . Wysocki" , Bartosz Golaszewski , bskeggs@redhat.com, linux-pci@vger.kernel.org, xen-devel@lists.xenproject.org, Thierry Reding , intel-gfx@lists.freedesktop.org, matthew.auld@intel.com, linux-devicetree , Jianxiong Gao , Daniel Vetter , Konrad Rzeszutek Wilk , maarten.lankhorst@linux.intel.com, airlied@linux.ie, Dan Williams , linuxppc-dev@lists.ozlabs.org, jani.nikula@linux.intel.com, Nathan Chancellor , Rob Herring , rodrigo.vivi@intel.com, Bjorn Helgaas , Claire Chang , boris.ostrovsky@oracle.com, Andy Shevchenko , jgross@suse.com, Nicolas Boichat , Greg KH , Randy Dunlap , Qian Cai , lkml , "list@263.net:IOMMU DRIVERS" , Jim Quinlan , xypron.glpk@gmx.de, Tom Lendacky , bauerman@linux.ibm.com References: <20210630114348.GA8383@willie-the-truck> <20210701074045.GA9436@willie-the-truck> <20210702135856.GB11132@willie-the-truck> <0f7bd903-e309-94a0-21d7-f0e8e9546018@arm.com> <20210705190352.GA19461@willie-the-truck> <20210706044848.GA13640@lst.de> <20210706132422.GA20327@willie-the-truck> From: Robin Murphy Message-ID: Date: Tue, 6 Jul 2021 15:01:04 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210706132422.GA20327@willie-the-truck> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-07-06 14:24, Will Deacon wrote: > On Tue, Jul 06, 2021 at 06:48:48AM +0200, Christoph Hellwig wrote: >> On Mon, Jul 05, 2021 at 08:03:52PM +0100, Will Deacon wrote: >>> So at this point, the AMD IOMMU driver does: >>> >>> swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; >>> >>> where 'swiotlb' is a global variable indicating whether or not swiotlb >>> is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which >>> will call swiotlb_exit() if 'swiotlb' is false. >>> >>> Now, that used to work fine, because swiotlb_exit() clears >>> 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I >>> think that all the devices which have successfully probed beforehand will >>> have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' >>> field. >> >> Yeah. I don't think we can do that anymore, and I also think it is >> a bad idea to start with. > > I've had a crack at reworking things along the following lines: > > - io_tlb_default_mem now lives in the BSS, the flexible array member > is now a pointer and that part is allocated dynamically (downside of > this is an extra indirection to get at the slots). > > - io_tlb_default_mem.nslabs tells you whether the thing is valid > > - swiotlb_exit() frees the slots array and clears the rest of the > structure to 0. I also extended it to free the actual slabs, but I'm > not sure why it wasn't doing that before. > > So a non-NULL dev->dma_io_tlb_mem should always be valid to follow. FWIW I was pondering the question of whether to do something along those lines or just scrap the default assignment entirely, so since I hadn't got round to saying that I've gone ahead and hacked up the alternative (similarly untested) for comparison :) TBH I'm still not sure which one I prefer... Robin. ----->8----- diff --git a/drivers/base/core.c b/drivers/base/core.c index ea5b85354526..394abf184c1a 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2847,9 +2847,6 @@ void device_initialize(struct device *dev) defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL) dev->dma_coherent = dma_default_coherent; #endif -#ifdef CONFIG_SWIOTLB - dev->dma_io_tlb_mem = io_tlb_default_mem; -#endif } EXPORT_SYMBOL_GPL(device_initialize); diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 39284ff2a6cd..620f16d89a98 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -107,16 +107,21 @@ struct io_tlb_mem { }; extern struct io_tlb_mem *io_tlb_default_mem; +static inline struct io_tlb_mem *dev_iotlb_mem(struct device *dev) +{ + return dev->dma_io_tlb_mem ?: io_tlb_default_mem; +} + static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); return mem && paddr >= mem->start && paddr < mem->end; } static inline bool is_swiotlb_force_bounce(struct device *dev) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); return mem && mem->force_bounce; } @@ -167,7 +172,7 @@ bool swiotlb_free(struct device *dev, struct page *page, size_t size); static inline bool is_swiotlb_for_alloc(struct device *dev) { - return dev->dma_io_tlb_mem->for_alloc; + return dev_iotlb_mem(dev)->for_alloc; } #else static inline struct page *swiotlb_alloc(struct device *dev, size_t size) diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index b7f76bca89bf..f4942149f87d 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -359,7 +359,7 @@ static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size, enum dma_data_direction dir) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT; phys_addr_t orig_addr = mem->slots[index].orig_addr; size_t alloc_size = mem->slots[index].alloc_size; @@ -440,7 +440,7 @@ static unsigned int wrap_index(struct io_tlb_mem *mem, unsigned int index) static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, size_t alloc_size) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned long boundary_mask = dma_get_seg_boundary(dev); dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, mem->start) & boundary_mask; @@ -522,7 +522,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr, size_t mapping_size, size_t alloc_size, enum dma_data_direction dir, unsigned long attrs) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned int offset = swiotlb_align_offset(dev, orig_addr); unsigned int i; int index; @@ -565,7 +565,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr, static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned long flags; unsigned int offset = swiotlb_align_offset(dev, tlb_addr); int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT; @@ -682,7 +682,7 @@ size_t swiotlb_max_mapping_size(struct device *dev) bool is_swiotlb_active(struct device *dev) { - return dev->dma_io_tlb_mem != NULL; + return dev_iotlb_mem(dev) != NULL; } EXPORT_SYMBOL_GPL(is_swiotlb_active); @@ -729,7 +729,7 @@ static void rmem_swiotlb_debugfs_init(struct reserved_mem *rmem) struct page *swiotlb_alloc(struct device *dev, size_t size) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); phys_addr_t tlb_addr; int index; @@ -792,7 +792,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem, static void rmem_swiotlb_device_release(struct reserved_mem *rmem, struct device *dev) { - dev->dma_io_tlb_mem = io_tlb_default_mem; + dev->dma_io_tlb_mem = NULL; } static const struct reserved_mem_ops rmem_swiotlb_ops = {