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 = {
