| Message ID | 7f64111986f4f361a2deb4a1a1b6f588e63a851b.1689261692.git.petr.tesarik.ext@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow dynamic allocation of software IO TLB bounce buffers | expand |
Hi Petr, On 13/7/23 17:23, Petr Tesarik wrote: > From: Petr Tesarik <petr.tesarik.ext@huawei.com> > > SWIOTLB implementation details should not be exposed to the rest of the > kernel. This will allow to make changes to the implementation without > modifying non-swiotlb code. > > To avoid breaking existing users, provide helper functions for the few > required fields. > > As a bonus, using a helper function to initialize struct device allows to > get rid of an #ifdef in driver core. > > Signed-off-by: Petr Tesarik <petr.tesarik.ext@huawei.com> > --- > arch/arm/xen/mm.c | 2 +- > arch/mips/pci/pci-octeon.c | 2 +- > arch/x86/kernel/pci-dma.c | 2 +- > drivers/base/core.c | 4 +--- > drivers/xen/swiotlb-xen.c | 2 +- > include/linux/swiotlb.h | 25 +++++++++++++++++++++++- > kernel/dma/swiotlb.c | 39 +++++++++++++++++++++++++++++++++++++- > 7 files changed, 67 insertions(+), 9 deletions(-) > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > index 4e52cd5e0bdc..07216af59e93 100644 > --- a/include/linux/swiotlb.h > +++ b/include/linux/swiotlb.h > @@ -110,7 +110,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) > { > @@ -128,13 +127,22 @@ 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_allocated(void); > 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); Usually we use start/end, base/limit, low[est]/high[est] tuples. Possibly clearer to rename, regardless: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 2b83e3ad9dca..873b077d7e37 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -958,6 +975,26 @@ bool is_swiotlb_active(struct device *dev) > } > 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. (note you describe lowest/highest). > + */ > +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)
On Mon, 17 Jul 2023 08:06:07 +0200 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Hi Petr, > > On 13/7/23 17:23, Petr Tesarik wrote: > > From: Petr Tesarik <petr.tesarik.ext@huawei.com> > > > > SWIOTLB implementation details should not be exposed to the rest of the > > kernel. This will allow to make changes to the implementation without > > modifying non-swiotlb code. > > > > To avoid breaking existing users, provide helper functions for the few > > required fields. > > > > As a bonus, using a helper function to initialize struct device allows to > > get rid of an #ifdef in driver core. > > > > Signed-off-by: Petr Tesarik <petr.tesarik.ext@huawei.com> > > --- > > arch/arm/xen/mm.c | 2 +- > > arch/mips/pci/pci-octeon.c | 2 +- > > arch/x86/kernel/pci-dma.c | 2 +- > > drivers/base/core.c | 4 +--- > > drivers/xen/swiotlb-xen.c | 2 +- > > include/linux/swiotlb.h | 25 +++++++++++++++++++++++- > > kernel/dma/swiotlb.c | 39 +++++++++++++++++++++++++++++++++++++- > > 7 files changed, 67 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > > index 4e52cd5e0bdc..07216af59e93 100644 > > --- a/include/linux/swiotlb.h > > +++ b/include/linux/swiotlb.h > > @@ -110,7 +110,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) > > { > > @@ -128,13 +127,22 @@ 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_allocated(void); > > 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); > > Usually we use start/end, base/limit, low[est]/high[est] tuples. I'm no big fan of start/end, because the "end" sometimes means "highest within range" and sometimes "one past range", being responsible for an impressive amount of off-by-one errors. But I agree. When I decided against "end", I should have also replaced "start" with "base". Well, this patch series will certainly see a v5, so I'll change it there. Thanks for the suggestion! Petr T > Possibly clearer to rename, regardless: > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > > index 2b83e3ad9dca..873b077d7e37 100644 > > --- a/kernel/dma/swiotlb.c > > +++ b/kernel/dma/swiotlb.c > > > > @@ -958,6 +975,26 @@ bool is_swiotlb_active(struct device *dev) > > } > > 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. > > (note you describe lowest/highest). > > > + */ > > +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) >
On Thu, Jul 13, 2023 at 05:23:12PM +0200, Petr Tesarik wrote: > From: Petr Tesarik <petr.tesarik.ext@huawei.com> > > SWIOTLB implementation details should not be exposed to the rest of the > kernel. This will allow to make changes to the implementation without > modifying non-swiotlb code. > > To avoid breaking existing users, provide helper functions for the few > required fields. > > As a bonus, using a helper function to initialize struct device allows to > get rid of an #ifdef in driver core. > > Signed-off-by: Petr Tesarik <petr.tesarik.ext@huawei.com> > --- > arch/arm/xen/mm.c | 2 +- > arch/mips/pci/pci-octeon.c | 2 +- > arch/x86/kernel/pci-dma.c | 2 +- > drivers/base/core.c | 4 +--- > drivers/xen/swiotlb-xen.c | 2 +- > include/linux/swiotlb.h | 25 +++++++++++++++++++++++- > kernel/dma/swiotlb.c | 39 +++++++++++++++++++++++++++++++++++++- > 7 files changed, 67 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > index 3d826c0b5fee..0f32c14eb786 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_allocated()) { > rc = swiotlb_init_late(swiotlb_size_or_default(), > xen_swiotlb_gfp(), NULL); > if (rc < 0) I'd much rather move the already initialized check into swiotlb_init_late, which is a much cleaer interface. > /* we can work with the default swiotlb */ > - if (!io_tlb_default_mem.nslabs) { > + if (!is_swiotlb_allocated()) { > int rc = swiotlb_init_late(swiotlb_size_or_default(), > GFP_KERNEL, xen_swiotlb_fixup); > if (rc < 0) .. and would take care of this one as well. > +bool is_swiotlb_allocated(void) > +{ > + return !!io_tlb_default_mem.nslabs; Nit: no need for the !!, we can rely on the implicit promotion to bool. But with the suggestion above the need for this helper should go away anyway.
On Thu, 20 Jul 2023 08:37:44 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Thu, Jul 13, 2023 at 05:23:12PM +0200, Petr Tesarik wrote: > > From: Petr Tesarik <petr.tesarik.ext@huawei.com> > > > > SWIOTLB implementation details should not be exposed to the rest of the > > kernel. This will allow to make changes to the implementation without > > modifying non-swiotlb code. > > > > To avoid breaking existing users, provide helper functions for the few > > required fields. > > > > As a bonus, using a helper function to initialize struct device allows to > > get rid of an #ifdef in driver core. > > > > Signed-off-by: Petr Tesarik <petr.tesarik.ext@huawei.com> > > --- > > arch/arm/xen/mm.c | 2 +- > > arch/mips/pci/pci-octeon.c | 2 +- > > arch/x86/kernel/pci-dma.c | 2 +- > > drivers/base/core.c | 4 +--- > > drivers/xen/swiotlb-xen.c | 2 +- > > include/linux/swiotlb.h | 25 +++++++++++++++++++++++- > > kernel/dma/swiotlb.c | 39 +++++++++++++++++++++++++++++++++++++- > > 7 files changed, 67 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > > index 3d826c0b5fee..0f32c14eb786 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_allocated()) { > > rc = swiotlb_init_late(swiotlb_size_or_default(), > > xen_swiotlb_gfp(), NULL); > > if (rc < 0) > > I'd much rather move the already initialized check into > swiotlb_init_late, which is a much cleaer interface. > > > /* we can work with the default swiotlb */ > > - if (!io_tlb_default_mem.nslabs) { > > + if (!is_swiotlb_allocated()) { > > int rc = swiotlb_init_late(swiotlb_size_or_default(), > > GFP_KERNEL, xen_swiotlb_fixup); > > if (rc < 0) > > .. and would take care of this one as well. Oh, you're right! These are the only two places that look at io_tlb_default_mem.nslabs, and all they need is to avoid double initialization. Makes perfect sense to move it inside swiotlb_init_late(). > > +bool is_swiotlb_allocated(void) > > +{ > > + return !!io_tlb_default_mem.nslabs; > > Nit: no need for the !!, we can rely on the implicit promotion to > bool. But with the suggestion above the need for this helper > should go away anyway. Eh, yes. I initially declared the return type as int and then forgot to change the return statement. But as you say, the whole function will go away entirely. Petr T
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index 3d826c0b5fee..0f32c14eb786 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_allocated()) { 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..08c6ffc3550f 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_allocated()) { 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 4e52cd5e0bdc..07216af59e93 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -110,7 +110,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) { @@ -128,13 +127,22 @@ 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_allocated(void); 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; @@ -151,6 +159,11 @@ static inline size_t swiotlb_max_mapping_size(struct device *dev) return SIZE_MAX; } +static inline bool is_swiotlb_allocated(void) +{ + return false; +} + static inline bool is_swiotlb_active(struct device *dev) { return false; @@ -159,6 +172,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 2b83e3ad9dca..873b077d7e37 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. */ @@ -950,6 +959,14 @@ size_t swiotlb_max_mapping_size(struct device *dev) return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE - min_align; } +/** + * is_swiotlb_allocated() - check if the default software IO TLB is initialized + */ +bool is_swiotlb_allocated(void) +{ + return !!io_tlb_default_mem.nslabs; +} + bool is_swiotlb_active(struct device *dev) { struct io_tlb_mem *mem = dev->dma_io_tlb_mem; @@ -958,6 +975,26 @@ bool is_swiotlb_active(struct device *dev) } 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)