diff mbox series

[1/6] RMDA/sw: don't allow drivers using dma_virt_ops on highmem configs

Message ID 20201105074205.1690638-2-hch@lst.de (mailing list archive)
State Superseded
Delegated to: Jason Gunthorpe
Headers show
Series [1/6] RMDA/sw: don't allow drivers using dma_virt_ops on highmem configs | expand

Commit Message

Christoph Hellwig Nov. 5, 2020, 7:42 a.m. UTC 
dma_virt_ops requires that all pages have a kernel virtual address.
Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on !HIGHMEM
and a large enough dma_addr_t, and make all three driver depend on the
new symbol.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/Kconfig           | 6 ++++++
 drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
 drivers/infiniband/sw/rxe/Kconfig    | 2 +-
 drivers/infiniband/sw/siw/Kconfig    | 1 +
 4 files changed, 10 insertions(+), 2 deletions(-)

Comments

Robin Murphy Nov. 5, 2020, 12:15 p.m. UTC | #1 
On 2020-11-05 07:42, Christoph Hellwig wrote:
> dma_virt_ops requires that all pages have a kernel virtual address.
> Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on !HIGHMEM
> and a large enough dma_addr_t, and make all three driver depend on the
> new symbol.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/infiniband/Kconfig           | 6 ++++++
>   drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
>   drivers/infiniband/sw/rxe/Kconfig    | 2 +-
>   drivers/infiniband/sw/siw/Kconfig    | 1 +
>   4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 32a51432ec4f73..81acaf5fb5be67 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -73,6 +73,12 @@ config INFINIBAND_ADDR_TRANS_CONFIGFS
>   	  This allows the user to config the default GID type that the CM
>   	  uses for each device, when initiaing new connections.
>   
> +config INFINIBAND_VIRT_DMA
> +	bool
> +	default y
> +	depends on !HIGHMEM
> +	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT

Isn't that effectively always true now since 4965a68780c5? I had a quick 
try of manually overriding CONFIG_ARCH_DMA_ADDR_T_64BIT in my .config, 
and the build just forces it back to "=y".

Robin.

> +
>   if INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
>   source "drivers/infiniband/hw/mthca/Kconfig"
>   source "drivers/infiniband/hw/qib/Kconfig"
> diff --git a/drivers/infiniband/sw/rdmavt/Kconfig b/drivers/infiniband/sw/rdmavt/Kconfig
> index 9ef5f5ce1ff6b0..c8e268082952b0 100644
> --- a/drivers/infiniband/sw/rdmavt/Kconfig
> +++ b/drivers/infiniband/sw/rdmavt/Kconfig
> @@ -1,7 +1,8 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   config INFINIBAND_RDMAVT
>   	tristate "RDMA verbs transport library"
> -	depends on X86_64 && ARCH_DMA_ADDR_T_64BIT
> +	depends on INFINIBAND_VIRT_DMA
> +	depends on X86_64
>   	depends on PCI
>   	select DMA_VIRT_OPS
>   	help
> diff --git a/drivers/infiniband/sw/rxe/Kconfig b/drivers/infiniband/sw/rxe/Kconfig
> index a0c6c7dfc1814f..8810bfa680495a 100644
> --- a/drivers/infiniband/sw/rxe/Kconfig
> +++ b/drivers/infiniband/sw/rxe/Kconfig
> @@ -2,7 +2,7 @@
>   config RDMA_RXE
>   	tristate "Software RDMA over Ethernet (RoCE) driver"
>   	depends on INET && PCI && INFINIBAND
> -	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
> +	depends on INFINIBAND_VIRT_DMA
>   	select NET_UDP_TUNNEL
>   	select CRYPTO_CRC32
>   	select DMA_VIRT_OPS
> diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig
> index b622fc62f2cd6d..3450ba5081df51 100644
> --- a/drivers/infiniband/sw/siw/Kconfig
> +++ b/drivers/infiniband/sw/siw/Kconfig
> @@ -1,6 +1,7 @@
>   config RDMA_SIW
>   	tristate "Software RDMA over TCP/IP (iWARP) driver"
>   	depends on INET && INFINIBAND && LIBCRC32C
> +	depends on INFINIBAND_VIRT_DMA
>   	select DMA_VIRT_OPS
>   	help
>   	This driver implements the iWARP RDMA transport over
>
Jason Gunthorpe Nov. 5, 2020, 2:41 p.m. UTC | #2 
On Thu, Nov 05, 2020 at 08:42:00AM +0100, Christoph Hellwig wrote:
> dma_virt_ops requires that all pages have a kernel virtual address.
> Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on !HIGHMEM
> and a large enough dma_addr_t, and make all three driver depend on the
> new symbol.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>  drivers/infiniband/Kconfig           | 6 ++++++
>  drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
>  drivers/infiniband/sw/rxe/Kconfig    | 2 +-
>  drivers/infiniband/sw/siw/Kconfig    | 1 +
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 32a51432ec4f73..81acaf5fb5be67 100644
> +++ b/drivers/infiniband/Kconfig
> @@ -73,6 +73,12 @@ config INFINIBAND_ADDR_TRANS_CONFIGFS
>  	  This allows the user to config the default GID type that the CM
>  	  uses for each device, when initiaing new connections.
>  
> +config INFINIBAND_VIRT_DMA
> +	bool
> +	default y

Oh, I haven't seen this kconfig trick with default before..

> +	depends on !HIGHMEM
> +	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
> +
>  if INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
>  source "drivers/infiniband/hw/mthca/Kconfig"
>  source "drivers/infiniband/hw/qib/Kconfig"
> diff --git a/drivers/infiniband/sw/rdmavt/Kconfig b/drivers/infiniband/sw/rdmavt/Kconfig
> index 9ef5f5ce1ff6b0..c8e268082952b0 100644
> +++ b/drivers/infiniband/sw/rdmavt/Kconfig
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config INFINIBAND_RDMAVT
>  	tristate "RDMA verbs transport library"
> -	depends on X86_64 && ARCH_DMA_ADDR_T_64BIT
> +	depends on INFINIBAND_VIRT_DMA

Usually I would expect a non-menu item to be used with select not
'depends on' - is the use of default avoiding that?

This looks nice

Jason
Robin Murphy Nov. 5, 2020, 3:29 p.m. UTC | #3 
On 2020-11-05 14:41, Jason Gunthorpe wrote:
> On Thu, Nov 05, 2020 at 08:42:00AM +0100, Christoph Hellwig wrote:
>> dma_virt_ops requires that all pages have a kernel virtual address.
>> Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on !HIGHMEM
>> and a large enough dma_addr_t, and make all three driver depend on the
>> new symbol.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>   drivers/infiniband/Kconfig           | 6 ++++++
>>   drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
>>   drivers/infiniband/sw/rxe/Kconfig    | 2 +-
>>   drivers/infiniband/sw/siw/Kconfig    | 1 +
>>   4 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
>> index 32a51432ec4f73..81acaf5fb5be67 100644
>> +++ b/drivers/infiniband/Kconfig
>> @@ -73,6 +73,12 @@ config INFINIBAND_ADDR_TRANS_CONFIGFS
>>   	  This allows the user to config the default GID type that the CM
>>   	  uses for each device, when initiaing new connections.
>>   
>> +config INFINIBAND_VIRT_DMA
>> +	bool
>> +	default y
> 
> Oh, I haven't seen this kconfig trick with default before..

It's commonly done using the "def_bool" shorthand. I fact, I think 
simply "def_bool !HIGHMEM" would suffice for the fundamental definition 
here.

>> +	depends on !HIGHMEM
>> +	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
>> +
>>   if INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
>>   source "drivers/infiniband/hw/mthca/Kconfig"
>>   source "drivers/infiniband/hw/qib/Kconfig"
>> diff --git a/drivers/infiniband/sw/rdmavt/Kconfig b/drivers/infiniband/sw/rdmavt/Kconfig
>> index 9ef5f5ce1ff6b0..c8e268082952b0 100644
>> +++ b/drivers/infiniband/sw/rdmavt/Kconfig
>> @@ -1,7 +1,8 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   config INFINIBAND_RDMAVT
>>   	tristate "RDMA verbs transport library"
>> -	depends on X86_64 && ARCH_DMA_ADDR_T_64BIT
>> +	depends on INFINIBAND_VIRT_DMA
> 
> Usually I would expect a non-menu item to be used with select not
> 'depends on' - is the use of default avoiding that?

A select wouldn't make any sense here - if the user chooses to enable 
the subsystem it can't automatically pull in "the absence of highmem" 
from the arch code; there's still a literal dependency on certain 
conditions being met for the option to be available. The intermediate 
config symbol just abstracts that set of conditions.

Robin.
Christoph Hellwig Nov. 5, 2020, 5 p.m. UTC | #4 
On Thu, Nov 05, 2020 at 12:15:46PM +0000, Robin Murphy wrote:
> On 2020-11-05 07:42, Christoph Hellwig wrote:
>> dma_virt_ops requires that all pages have a kernel virtual address.
>> Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on !HIGHMEM
>> and a large enough dma_addr_t, and make all three driver depend on the
>> new symbol.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/infiniband/Kconfig           | 6 ++++++
>>   drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
>>   drivers/infiniband/sw/rxe/Kconfig    | 2 +-
>>   drivers/infiniband/sw/siw/Kconfig    | 1 +
>>   4 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
>> index 32a51432ec4f73..81acaf5fb5be67 100644
>> --- a/drivers/infiniband/Kconfig
>> +++ b/drivers/infiniband/Kconfig
>> @@ -73,6 +73,12 @@ config INFINIBAND_ADDR_TRANS_CONFIGFS
>>   	  This allows the user to config the default GID type that the CM
>>   	  uses for each device, when initiaing new connections.
>>   +config INFINIBAND_VIRT_DMA
>> +	bool
>> +	default y
>> +	depends on !HIGHMEM
>> +	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
>
> Isn't that effectively always true now since 4965a68780c5? I had a quick 
> try of manually overriding CONFIG_ARCH_DMA_ADDR_T_64BIT in my .config, and 
> the build just forces it back to "=y".

True.  The guy who did the commit should have really told me about it :)
Christoph Hellwig Nov. 5, 2020, 5:03 p.m. UTC | #5 
On Thu, Nov 05, 2020 at 03:29:58PM +0000, Robin Murphy wrote:
> It's commonly done using the "def_bool" shorthand. I fact, I think simply 
> "def_bool !HIGHMEM" would suffice for the fundamental definition here.

Indeed, I'll switch it over.
Bernard Metzler Nov. 5, 2020, 8:32 p.m. UTC | #6 
-----"Christoph Hellwig" <hch@lst.de> wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: "Christoph Hellwig" <hch@lst.de>
>Date: 11/05/2020 08:46AM
>Cc: "Bjorn Helgaas" <bhelgaas@google.com>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Zhu Yanjun" <yanjunz@nvidia.com>, "Logan
>Gunthorpe" <logang@deltatee.com>, "Dennis Dalessandro"
><dennis.dalessandro@cornelisnetworks.com>, "Mike Marciniszyn"
><mike.marciniszyn@cornelisnetworks.com>, linux-rdma@vger.kernel.org,
>linux-pci@vger.kernel.org, iommu@lists.linux-foundation.org
>Subject: [EXTERNAL] [PATCH 1/6] RMDA/sw: don't allow drivers using
>dma_virt_ops on highmem configs
>
>dma_virt_ops requires that all pages have a kernel virtual address.
>Introduce a INFINIBAND_VIRT_DMA Kconfig symbol that depends on
>!HIGHMEM
>and a large enough dma_addr_t, and make all three driver depend on
>the
>new symbol.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> drivers/infiniband/Kconfig           | 6 ++++++
> drivers/infiniband/sw/rdmavt/Kconfig | 3 ++-
> drivers/infiniband/sw/rxe/Kconfig    | 2 +-
> drivers/infiniband/sw/siw/Kconfig    | 1 +
> 4 files changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
>index 32a51432ec4f73..81acaf5fb5be67 100644
>--- a/drivers/infiniband/Kconfig
>+++ b/drivers/infiniband/Kconfig
>@@ -73,6 +73,12 @@ config INFINIBAND_ADDR_TRANS_CONFIGFS
> 	  This allows the user to config the default GID type that the CM
> 	  uses for each device, when initiaing new connections.
> 
>+config INFINIBAND_VIRT_DMA
>+	bool
>+	default y
>+	depends on !HIGHMEM
>+	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
>+
> if INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
> source "drivers/infiniband/hw/mthca/Kconfig"
> source "drivers/infiniband/hw/qib/Kconfig"
>diff --git a/drivers/infiniband/sw/rdmavt/Kconfig
>b/drivers/infiniband/sw/rdmavt/Kconfig
>index 9ef5f5ce1ff6b0..c8e268082952b0 100644
>--- a/drivers/infiniband/sw/rdmavt/Kconfig
>+++ b/drivers/infiniband/sw/rdmavt/Kconfig
>@@ -1,7 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0-only
> config INFINIBAND_RDMAVT
> 	tristate "RDMA verbs transport library"
>-	depends on X86_64 && ARCH_DMA_ADDR_T_64BIT
>+	depends on INFINIBAND_VIRT_DMA
>+	depends on X86_64
> 	depends on PCI
> 	select DMA_VIRT_OPS
> 	help
>diff --git a/drivers/infiniband/sw/rxe/Kconfig
>b/drivers/infiniband/sw/rxe/Kconfig
>index a0c6c7dfc1814f..8810bfa680495a 100644
>--- a/drivers/infiniband/sw/rxe/Kconfig
>+++ b/drivers/infiniband/sw/rxe/Kconfig
>@@ -2,7 +2,7 @@
> config RDMA_RXE
> 	tristate "Software RDMA over Ethernet (RoCE) driver"
> 	depends on INET && PCI && INFINIBAND
>-	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
>+	depends on INFINIBAND_VIRT_DMA
> 	select NET_UDP_TUNNEL
> 	select CRYPTO_CRC32
> 	select DMA_VIRT_OPS
>diff --git a/drivers/infiniband/sw/siw/Kconfig
>b/drivers/infiniband/sw/siw/Kconfig
>index b622fc62f2cd6d..3450ba5081df51 100644
>--- a/drivers/infiniband/sw/siw/Kconfig
>+++ b/drivers/infiniband/sw/siw/Kconfig
>@@ -1,6 +1,7 @@
> config RDMA_SIW
> 	tristate "Software RDMA over TCP/IP (iWARP) driver"
> 	depends on INET && INFINIBAND && LIBCRC32C
>+	depends on INFINIBAND_VIRT_DMA
> 	select DMA_VIRT_OPS
> 	help
> 	This driver implements the iWARP RDMA transport over
>-- 
>2.28.0
>
>


The complete patch set works for siw. Tested with siw as
nvmef target.

Tested-by: Bernard Metzler <bmt@zurich.ibm.com>
diff mbox series

Patch

diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index 32a51432ec4f73..81acaf5fb5be67 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -73,6 +73,12 @@  config INFINIBAND_ADDR_TRANS_CONFIGFS
 	  This allows the user to config the default GID type that the CM
 	  uses for each device, when initiaing new connections.
 
+config INFINIBAND_VIRT_DMA
+	bool
+	default y
+	depends on !HIGHMEM
+	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
+
 if INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
 source "drivers/infiniband/hw/mthca/Kconfig"
 source "drivers/infiniband/hw/qib/Kconfig"
diff --git a/drivers/infiniband/sw/rdmavt/Kconfig b/drivers/infiniband/sw/rdmavt/Kconfig
index 9ef5f5ce1ff6b0..c8e268082952b0 100644
--- a/drivers/infiniband/sw/rdmavt/Kconfig
+++ b/drivers/infiniband/sw/rdmavt/Kconfig
@@ -1,7 +1,8 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 config INFINIBAND_RDMAVT
 	tristate "RDMA verbs transport library"
-	depends on X86_64 && ARCH_DMA_ADDR_T_64BIT
+	depends on INFINIBAND_VIRT_DMA
+	depends on X86_64
 	depends on PCI
 	select DMA_VIRT_OPS
 	help
diff --git a/drivers/infiniband/sw/rxe/Kconfig b/drivers/infiniband/sw/rxe/Kconfig
index a0c6c7dfc1814f..8810bfa680495a 100644
--- a/drivers/infiniband/sw/rxe/Kconfig
+++ b/drivers/infiniband/sw/rxe/Kconfig
@@ -2,7 +2,7 @@ 
 config RDMA_RXE
 	tristate "Software RDMA over Ethernet (RoCE) driver"
 	depends on INET && PCI && INFINIBAND
-	depends on !64BIT || ARCH_DMA_ADDR_T_64BIT
+	depends on INFINIBAND_VIRT_DMA
 	select NET_UDP_TUNNEL
 	select CRYPTO_CRC32
 	select DMA_VIRT_OPS
diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig
index b622fc62f2cd6d..3450ba5081df51 100644
--- a/drivers/infiniband/sw/siw/Kconfig
+++ b/drivers/infiniband/sw/siw/Kconfig
@@ -1,6 +1,7 @@ 
 config RDMA_SIW
 	tristate "Software RDMA over TCP/IP (iWARP) driver"
 	depends on INET && INFINIBAND && LIBCRC32C
+	depends on INFINIBAND_VIRT_DMA
 	select DMA_VIRT_OPS
 	help
 	This driver implements the iWARP RDMA transport over