| Message ID | a1ef6eeab8b64fac817b9734da4a056f05a68d01.1687859323.git.petr.tesarik.ext@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow dynamic allocation of software IO TLB bounce buffers | expand |
On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote: > +/** > + * is_swiotlb_active() - check if the software IO TLB is initialized > + * @dev: Device to check, or %NULL for the default IO TLB. > + */ > bool is_swiotlb_active(struct device *dev) > { > - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > + struct io_tlb_mem *mem = dev > + ? dev->dma_io_tlb_mem > + : &io_tlb_default_mem; That's impossible to read and maintain over time, sorry. Please use real "if () else" lines, so that it can be maintained over time. thanks, greg k-h
On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote: > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote: >> +/** >> + * is_swiotlb_active() - check if the software IO TLB is initialized >> + * @dev: Device to check, or %NULL for the default IO TLB. >> + */ >> bool is_swiotlb_active(struct device *dev) >> { >> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; >> + struct io_tlb_mem *mem = dev >> + ? dev->dma_io_tlb_mem >> + : &io_tlb_default_mem; > > That's impossible to read and maintain over time, sorry. > > Please use real "if () else" lines, so that it can be maintained over > time. Moreover, it makes for a horrible interface anyway. If there's a need for a non-specific "is SWIOTLB present at all?" check unrelated to any particular device (which arguably still smells of poking into implementation details...), please encapsulate it in its own distinct helper like, say, is_swiotlb_present(void). However, the more I think about it, the more I doubt that logic like octeon_pci_setup() can continue to work properly at all if SWIOTLB allocation becomes dynamic... :/ Thanks, Robin.
On Tue, 27 Jun 2023 11:55:00 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote: > > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote: > >> +/** > >> + * is_swiotlb_active() - check if the software IO TLB is initialized > >> + * @dev: Device to check, or %NULL for the default IO TLB. > >> + */ > >> bool is_swiotlb_active(struct device *dev) > >> { > >> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > >> + struct io_tlb_mem *mem = dev > >> + ? dev->dma_io_tlb_mem > >> + : &io_tlb_default_mem; > > > > That's impossible to read and maintain over time, sorry. > > > > Please use real "if () else" lines, so that it can be maintained over > > time. > > Moreover, it makes for a horrible interface anyway. If there's a need > for a non-specific "is SWIOTLB present at all?" check unrelated to any > particular device (which arguably still smells of poking into > implementation details...), please encapsulate it in its own distinct > helper like, say, is_swiotlb_present(void). > > However, the more I think about it, the more I doubt that logic like > octeon_pci_setup() can continue to work properly at all if SWIOTLB > allocation becomes dynamic... :/ Good, so I'm not alone. I don't know enough of the Octeon hardware to understand how much magic is behind these PCI BARs and why one of them should be (sometimes) programmed the way it is. OTOH it doesn't seem to me that this platform forces DMA through SWIOTLB. At least all calls to swiotlb_init() under arch/mips take this form: swiotlb_init(true, SWIOTLB_VERBOSE); This makes me believe that this PCI BAR setup is merely an optimization. However, if nobody has a clear answer, a fallback solution is to stay on the safe side and add a flag to struct io_tlb_mem whether SWIOTLB can grow dynamically. The helper function would then set this flag and make sure that on this Octeon platform, the SWIOTLB stays restricted to the default pool. Hopefully, Thomas Bogendoerfer can shed some light on that code. Petr T
Oops, originally sent only to Robin. Restoring the recipient list here... On Tue, 27 Jun 2023 11:55:00 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote: > > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote: > >> +/** > >> + * is_swiotlb_active() - check if the software IO TLB is initialized > >> + * @dev: Device to check, or %NULL for the default IO TLB. > >> + */ > >> bool is_swiotlb_active(struct device *dev) > >> { > >> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > >> + struct io_tlb_mem *mem = dev > >> + ? dev->dma_io_tlb_mem > >> + : &io_tlb_default_mem; > > > > That's impossible to read and maintain over time, sorry. > > > > Please use real "if () else" lines, so that it can be maintained over > > time. > > Moreover, it makes for a horrible interface anyway. If there's a need > for a non-specific "is SWIOTLB present at all?" check unrelated to any > particular device (which arguably still smells of poking into > implementation details...), please encapsulate it in its own distinct > helper like, say, is_swiotlb_present(void). I'm sorry for writing two replies, but I realized too late that this part is unrelated to the MIPS Octeon platform. Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen needs bounce buffers for the PCI frontend driver, but if there is no other reason to have a SWIOTLB, the system does not set up one at boot. Yeah, they should probably do things differently. At least this code in arch/x86/kernel/pci-dma.c is fishy: /* XXX: this switches the dma ops under live devices! */ dma_ops = &xen_swiotlb_dma_ops; However, I don't think it's up to me to fix that... To sum it up, I can certainly provide a separate function instead of overloading the is_swiotlb_active() API. Thanks for the suggestion! Petr T
On Tue, Jun 27, 2023 at 01:30:06PM +0200, Petr Tesařík wrote: > Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen > needs bounce buffers for the PCI frontend driver, but if there is no > other reason to have a SWIOTLB, the system does not set up one at boot. Please take a look at my "unexport swiotlb_active v2" series that unfortunately missed the 6.5 merge window waiting for reviews.
On Tue, 27 Jun 2023 17:48:02 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Tue, Jun 27, 2023 at 01:30:06PM +0200, Petr Tesařík wrote: > > Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen > > needs bounce buffers for the PCI frontend driver, but if there is no > > other reason to have a SWIOTLB, the system does not set up one at boot. > > Please take a look at my "unexport swiotlb_active v2" series that > unfortunately missed the 6.5 merge window waiting for reviews. I noticed it, but it seems I missed the part which completely removes pci_xen_swiotlb_init_late(). Then we're left only with a reference from xen_mm_init() in arch/arm/xen/mm.c, and I believe this one can also be solved differently. Petr T
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index 3d826c0b5fee..529aabc1be38 100644 --- a/arch/arm/xen/mm.c +++ b/arch/arm/xen/mm.c @@ -125,7 +125,7 @@ static int __init xen_mm_init(void) return 0; /* we can work with the default swiotlb */ - if (!io_tlb_default_mem.nslabs) { + if (!is_swiotlb_active(NULL)) { rc = swiotlb_init_late(swiotlb_size_or_default(), xen_swiotlb_gfp(), NULL); if (rc < 0) diff --git a/arch/mips/pci/pci-octeon.c b/arch/mips/pci/pci-octeon.c index e457a18cbdc5..c5c4c1f7d5e4 100644 --- a/arch/mips/pci/pci-octeon.c +++ b/arch/mips/pci/pci-octeon.c @@ -664,7 +664,7 @@ static int __init octeon_pci_setup(void) /* BAR1 movable regions contiguous to cover the swiotlb */ octeon_bar1_pci_phys = - io_tlb_default_mem.start & ~((1ull << 22) - 1); + default_swiotlb_start() & ~((1ull << 22) - 1); for (index = 0; index < 32; index++) { union cvmx_pci_bar1_indexx bar1_index; diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c index de6be0a3965e..33e960672837 100644 --- a/arch/x86/kernel/pci-dma.c +++ b/arch/x86/kernel/pci-dma.c @@ -90,7 +90,7 @@ int pci_xen_swiotlb_init_late(void) return 0; /* we can work with the default swiotlb */ - if (!io_tlb_default_mem.nslabs) { + if (!is_swiotlb_active(NULL)) { int rc = swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL, xen_swiotlb_fixup); if (rc < 0) diff --git a/drivers/base/core.c b/drivers/base/core.c index 3dff5037943e..46d1d78c5beb 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3108,9 +3108,7 @@ 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 + swiotlb_dev_init(dev); } EXPORT_SYMBOL_GPL(device_initialize); diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 67aa74d20162..946bd56f0ac5 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -381,7 +381,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct scatterlist *sgl, static int xen_swiotlb_dma_supported(struct device *hwdev, u64 mask) { - return xen_phys_to_dma(hwdev, io_tlb_default_mem.end - 1) <= mask; + return xen_phys_to_dma(hwdev, default_swiotlb_limit()) <= mask; } const struct dma_map_ops xen_swiotlb_dma_ops = { diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 7af2673b47ba..b38045be532d 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -112,7 +112,6 @@ struct io_tlb_mem { atomic_long_t used_hiwater; #endif }; -extern struct io_tlb_mem io_tlb_default_mem; static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { @@ -130,13 +129,19 @@ static inline bool is_swiotlb_force_bounce(struct device *dev) void swiotlb_init(bool addressing_limited, unsigned int flags); void __init swiotlb_exit(void); +void swiotlb_dev_init(struct device *dev); size_t swiotlb_max_mapping_size(struct device *dev); bool is_swiotlb_active(struct device *dev); void __init swiotlb_adjust_size(unsigned long size); +phys_addr_t default_swiotlb_start(void); +phys_addr_t default_swiotlb_limit(void); #else static inline void swiotlb_init(bool addressing_limited, unsigned int flags) { } +static inline void swiotlb_dev_init(struct device *dev) +{ +} static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { return false; @@ -161,6 +166,16 @@ static inline bool is_swiotlb_active(struct device *dev) static inline void swiotlb_adjust_size(unsigned long size) { } + +static inline phys_addr_t default_swiotlb_start(void) +{ + return 0; +} + +static inline phys_addr_t default_swiotlb_limit(void) +{ + return 0; +} #endif /* CONFIG_SWIOTLB */ extern void swiotlb_print_info(void); diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 079df5ad38d0..63d318059f27 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -71,7 +71,7 @@ struct io_tlb_slot { static bool swiotlb_force_bounce; static bool swiotlb_force_disable; -struct io_tlb_mem io_tlb_default_mem; +static struct io_tlb_mem io_tlb_default_mem; static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT; static unsigned long default_nareas; @@ -486,6 +486,15 @@ void __init swiotlb_exit(void) memset(mem, 0, sizeof(*mem)); } +/** + * swiotlb_dev_init() - initialize swiotlb fields in &struct device + * @dev: Device to be initialized. + */ +void swiotlb_dev_init(struct device *dev) +{ + dev->dma_io_tlb_mem = &io_tlb_default_mem; +} + /* * Return the offset into a iotlb slot required to keep the device happy. */ @@ -939,14 +948,40 @@ size_t swiotlb_max_mapping_size(struct device *dev) return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE - min_align; } +/** + * is_swiotlb_active() - check if the software IO TLB is initialized + * @dev: Device to check, or %NULL for the default IO TLB. + */ bool is_swiotlb_active(struct device *dev) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev + ? dev->dma_io_tlb_mem + : &io_tlb_default_mem; return mem && mem->nslabs; } EXPORT_SYMBOL_GPL(is_swiotlb_active); +/** + * default_swiotlb_start() - get the start of the default SWIOTLB + * + * Get the lowest physical address used by the default software IO TLB pool. + */ +phys_addr_t default_swiotlb_start(void) +{ + return io_tlb_default_mem.start; +} + +/** + * default_swiotlb_limit() - get the highest address in the default SWIOTLB + * + * Get the highest physical address used by the default software IO TLB pool. + */ +phys_addr_t default_swiotlb_limit(void) +{ + return io_tlb_default_mem.end - 1; +} + #ifdef CONFIG_DEBUG_FS static int io_tlb_used_get(void *data, u64 *val)