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

From: Robin Murphy <robin.murphy@arm.com>
To: Jason Gunthorpe <jgg@ziepe.ca>, Christoph Hellwig <hch@lst.de>
Cc: Zhu Yanjun <yanjunz@nvidia.com>,
	Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	linux-rdma@vger.kernel.org, linux-pci@vger.kernel.org,
	Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>,
	iommu@lists.linux-foundation.org,
	Bjorn Helgaas <bhelgaas@google.com>,
	Bernard Metzler <bmt@zurich.ibm.com>,
	Logan Gunthorpe <logang@deltatee.com>
Subject: Re: [PATCH 1/6] RMDA/sw: don't allow drivers using dma_virt_ops on highmem configs
Date: Thu, 5 Nov 2020 15:29:58 +0000	[thread overview]
Message-ID: <74729b8d-146f-803a-98a3-e8149bd97e34@arm.com> (raw)
In-Reply-To: <20201105144123.GB4142106@ziepe.ca>

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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu