Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On Tue, Apr 24, 2018 at 08:47:27AM +0100, Russell King - ARM Linux wrote: > Therefore, the default state for SWIOTLB and hence NEED_SG_DMA_LENGTH > becomes 'y' on ARM, and any defconfig file that does not mention SWIOTLB > explicitly ends up with both these enabled. Indeed, sorry. > It does look a bit weird though - patch 10 arranged stuff so that we > didn't end up with SWIOTLB always enabled, but this patch reintroduces > that with the allowance that the user can disable if so desired. I am not very happy with that patch, but I have a hard time coming up with something saner. Bascially x86_64 and mips/loongson default to SWIOTLB=y but allow to deselect it, powerpc has it optional without any real dependency and defaults to n and everyone just selects it otherwise. I suspect the right thing is to just have it always one for x86_64 and loongson and have a ppc-specific option to enable it on powerpc so that we can always use select statements. I'll do that for the next round.
Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On Tue, Apr 24, 2018 at 08:55:49AM +0200, Christoph Hellwig wrote: > On Tue, Apr 24, 2018 at 12:52:05AM +0100, Russell King - ARM Linux wrote: > > On Mon, Apr 23, 2018 at 07:04:18PM +0200, Christoph Hellwig wrote: > > > This way we have one central definition of it, and user can select it as > > > needed. Note that we also add a second ARCH_HAS_SWIOTLB symbol to > > > indicate the architecture supports swiotlb at all, so that we can still > > > make the usage optional for a few architectures that want this feature > > > to be user selectable. > > > > > > Signed-off-by: Christoph Hellwig > > > > Hmm, this looks like we end up with NEED_SG_DMA_LENGTH=y on ARM by > > default, which probably isn't a good idea - ARM pre-dates the dma_length > > parameter in scatterlists, and I don't think all code is guaranteed to > > do the right thing if this is enabled. > > We shouldn't end up with NEED_SG_DMA_LENGTH=y on ARM by default. Your patch as sent would end up with: ARM selects ARCH_HAS_SWIOTLB SWIOTLB is defaulted to ARCH_HAS_SWIOTLB SWIOTLB selects NEED_SG_DMA_LENGTH due to: @@ -106,6 +106,7 @@ config ARM select REFCOUNT_FULL select RTC_LIB select SYS_SUPPORTS_APM_EMULATION + select ARCH_HAS_SWIOTLB and: +config SWIOTLB + bool "SWIOTLB support" + default ARCH_HAS_SWIOTLB + select NEED_SG_DMA_LENGTH Therefore, the default state for SWIOTLB and hence NEED_SG_DMA_LENGTH becomes 'y' on ARM, and any defconfig file that does not mention SWIOTLB explicitly ends up with both these enabled. > It is only select by ARM_DMA_USE_IOMMU before the patch, and it will > now also be selected by SWIOTLB, which for arm is never used or seleted > directly by anything but xen-swiotlb. See above. > Then again looking at the series there shouldn't be any need to > even select NEED_SG_DMA_LENGTH for swiotlb, as we'll never merge segments, > so I'll fix that up. That would help to avoid any regressions along the lines I've spotted by review. It does look a bit weird though - patch 10 arranged stuff so that we didn't end up with SWIOTLB always enabled, but this patch reintroduces that with the allowance that the user can disable if so desired. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On Tue, Apr 24, 2018 at 12:52:05AM +0100, Russell King - ARM Linux wrote: > On Mon, Apr 23, 2018 at 07:04:18PM +0200, Christoph Hellwig wrote: > > This way we have one central definition of it, and user can select it as > > needed. Note that we also add a second ARCH_HAS_SWIOTLB symbol to > > indicate the architecture supports swiotlb at all, so that we can still > > make the usage optional for a few architectures that want this feature > > to be user selectable. > > > > Signed-off-by: Christoph Hellwig > > Hmm, this looks like we end up with NEED_SG_DMA_LENGTH=y on ARM by > default, which probably isn't a good idea - ARM pre-dates the dma_length > parameter in scatterlists, and I don't think all code is guaranteed to > do the right thing if this is enabled. We shouldn't end up with NEED_SG_DMA_LENGTH=y on ARM by default. It is only select by ARM_DMA_USE_IOMMU before the patch, and it will now also be selected by SWIOTLB, which for arm is never used or seleted directly by anything but xen-swiotlb. Then again looking at the series there shouldn't be any need to even select NEED_SG_DMA_LENGTH for swiotlb, as we'll never merge segments, so I'll fix that up.
Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On Mon, Apr 23, 2018 at 07:04:18PM +0200, Christoph Hellwig wrote: > This way we have one central definition of it, and user can select it as > needed. Note that we also add a second ARCH_HAS_SWIOTLB symbol to > indicate the architecture supports swiotlb at all, so that we can still > make the usage optional for a few architectures that want this feature > to be user selectable. > > Signed-off-by: Christoph Hellwig Hmm, this looks like we end up with NEED_SG_DMA_LENGTH=y on ARM by default, which probably isn't a good idea - ARM pre-dates the dma_length parameter in scatterlists, and I don't think all code is guaranteed to do the right thing if this is enabled. For example, arch/arm/mach-rpc/dma.c doesn't use the dma_length member of struct scatterlist. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On Mon, Apr 23, 2018 at 07:04:18PM +0200, Christoph Hellwig wrote: > This way we have one central definition of it, and user can select it as > needed. Note that we also add a second ARCH_HAS_SWIOTLB symbol to > indicate the architecture supports swiotlb at all, so that we can still > make the usage optional for a few architectures that want this feature > to be user selectable. If I follow this select business this will enable it on ARM and x86 by default. As such: Reviewed-by: Konrad Rzeszutek Wilk Thank you! > > Signed-off-by: Christoph Hellwig > --- > arch/arm/Kconfig| 4 +--- > arch/arm64/Kconfig | 5 ++--- > arch/ia64/Kconfig | 9 + > arch/mips/Kconfig | 3 +++ > arch/mips/cavium-octeon/Kconfig | 5 - > arch/mips/loongson64/Kconfig| 8 > arch/powerpc/Kconfig| 9 - > arch/unicore32/mm/Kconfig | 5 - > arch/x86/Kconfig| 14 +++--- > lib/Kconfig | 15 +++ > 10 files changed, 25 insertions(+), 52 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 90b81a3a28a7..f91f69174630 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -106,6 +106,7 @@ config ARM > select REFCOUNT_FULL > select RTC_LIB > select SYS_SUPPORTS_APM_EMULATION > + select ARCH_HAS_SWIOTLB > # Above selects are sorted alphabetically; please add new ones > # according to that. Thanks. > help > @@ -1773,9 +1774,6 @@ config SECCOMP > and the task is only allowed to execute a few safe syscalls > defined by each seccomp mode. > > -config SWIOTLB > - bool > - > config PARAVIRT > bool "Enable paravirtualization code" > help > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 4d924eb32e7f..056bc7365adf 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -21,6 +21,7 @@ config ARM64 > select ARCH_HAS_SG_CHAIN > select ARCH_HAS_STRICT_KERNEL_RWX > select ARCH_HAS_STRICT_MODULE_RWX > + select ARCH_HAS_SWIOTLB > select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST > select ARCH_HAVE_NMI_SAFE_CMPXCHG > select ARCH_INLINE_READ_LOCK if !PREEMPT > @@ -144,6 +145,7 @@ config ARM64 > select POWER_SUPPLY > select REFCOUNT_FULL > select SPARSE_IRQ > + select SWIOTLB > select SYSCTL_EXCEPTION_TRACE > select THREAD_INFO_IN_TASK > help > @@ -239,9 +241,6 @@ config HAVE_GENERIC_GUP > config SMP > def_bool y > > -config SWIOTLB > - def_bool y > - > config KERNEL_MODE_NEON > def_bool y > > diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig > index 685d557eea48..d396230913e6 100644 > --- a/arch/ia64/Kconfig > +++ b/arch/ia64/Kconfig > @@ -56,6 +56,7 @@ config IA64 > select HAVE_ARCH_AUDITSYSCALL > select NEED_DMA_MAP_STATE > select NEED_SG_DMA_LENGTH > + select ARCH_HAS_SWIOTLB > default y > help > The Itanium Processor Family is Intel's 64-bit successor to > @@ -80,9 +81,6 @@ config MMU > bool > default y > > -config SWIOTLB > - bool > - > config STACKTRACE_SUPPORT > def_bool y > > @@ -139,7 +137,6 @@ config IA64_GENERIC > bool "generic" > select NUMA > select ACPI_NUMA > - select DMA_DIRECT_OPS > select SWIOTLB > select PCI_MSI > help > @@ -160,7 +157,6 @@ config IA64_GENERIC > > config IA64_DIG > bool "DIG-compliant" > - select DMA_DIRECT_OPS > select SWIOTLB > > config IA64_DIG_VTD > @@ -176,7 +172,6 @@ config IA64_HP_ZX1 > > config IA64_HP_ZX1_SWIOTLB > bool "HP-zx1/sx1000 with software I/O TLB" > - select DMA_DIRECT_OPS > select SWIOTLB > help > Build a kernel that runs on HP zx1 and sx1000 systems even when they > @@ -200,7 +195,6 @@ config IA64_SGI_UV > bool "SGI-UV" > select NUMA > select ACPI_NUMA > - select DMA_DIRECT_OPS > select SWIOTLB > help > Selecting this option will optimize the kernel for use on UV based > @@ -211,7 +205,6 @@ config IA64_SGI_UV > > config IA64_HP_SIM > bool "Ski-simulator" > - select DMA_DIRECT_OPS > select SWIOTLB > depends on !PM > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index e10cc5c7be69..b6b4c1e154f8 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -912,6 +912,8 @@ config CAVIUM_OCTEON_SOC > select MIPS_NR_CPU_NR_MAP_1024 > select BUILTIN_DTB > select MTD_COMPLEX_MAPPINGS > + select ARCH_HAS_SWIOTLB > + select SWIOTLB > select SYS_SUPPORTS_RELOCATABLE > help > This option supports all of the Octeon reference boards from Cavium > @@ -1367,6 +1369,7 @@ config CPU_LOONGSON3 > select MIPS_PGD_C0_CONTEXT > select MIPS_L1_CACHE_SHIFT_6 > select GPIOLIB > + select ARCH_HAS_SWIOTLB >
Re: [PATCH 11/12] swiotlb: move the SWIOTLB config symbol to lib/Kconfig
On 04/15/2018 08:29 PM, Christoph Hellwig wrote: > This way we have one central definition of it, and user can select it as > needed. Note that we also add a second ARCH_HAS_SWIOTLB symbol to > indicate the architecture supports swiotlb at all, so that we can still > make the usage optional for a few architectures that want this feature > to be user selectable. > > Signed-off-by: Christoph Hellwig snip > + > +config SWIOTLB > + bool "SWIOTLB support" > + default ARCH_HAS_SWIOTLB > + select DMA_DIRECT_OPS > + select NEED_DMA_MAP_STATE > + select NEED_SG_DMA_LENGTH > + ---help--- > + Support for IO bounce buffering for systems without an IOMMU. > + This allows us to DMA to the full physical address space on > + platforms where the size of a physical address is larger > + than the bus address. If unsure, say Y. > + > config CHECK_SIGNATURE > bool Pulling DMA_DIRECT_OPS config option by default when SWIOTLB is enabled makes sense. This option was also needed to be enabled separately even to use swiotlb_dma_ops.
