linux-kernel - Re: [PATCH 10/16] dma-mapping: Add flags to dma_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 May 2021 11:09:36 -0600
From:   Logan Gunthorpe <logang@...tatee.com>
To:     John Hubbard <jhubbard@...dia.com>, linux-kernel@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-block@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-mm@...ck.org,
        iommu@...ts.linux-foundation.org
Cc:     Stephen Bates <sbates@...thlin.com>,
        Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Christian König <christian.koenig@....com>,
        Don Dutile <ddutile@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Minturn Dave B <dave.b.minturn@...el.com>,
        Jason Ekstrand <jason@...kstrand.net>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Xiong Jianxin <jianxin.xiong@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 10/16] dma-mapping: Add flags to dma_map_ops to indicate
 PCI P2PDMA support



On 2021-05-02 6:32 p.m., John Hubbard wrote:
> On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
>> Add a flags member to the dma_map_ops structure with one flag to
>> indicate support for PCI P2PDMA.
>>
>> Also, add a helper to check if a device supports PCI P2PDMA.
>>
>> Signed-off-by: Logan Gunthorpe <logang@...tatee.com>
>> ---
>>   include/linux/dma-map-ops.h |  3 +++
>>   include/linux/dma-mapping.h |  5 +++++
>>   kernel/dma/mapping.c        | 18 ++++++++++++++++++
>>   3 files changed, 26 insertions(+)
>>
>> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
>> index 51872e736e7b..481892822104 100644
>> --- a/include/linux/dma-map-ops.h
>> +++ b/include/linux/dma-map-ops.h
>> @@ -12,6 +12,9 @@
>>   struct cma;
>>   
>>   struct dma_map_ops {
>> +	unsigned int flags;
>> +#define DMA_F_PCI_P2PDMA_SUPPORTED     (1 << 0)
>> +
> 
> Can we move this up and out of the struct member area, so that it looks
> more like this:
> 
> /*
>   * Values for struct dma_map_ops.flags:
>   *
>   * DMA_F_PCI_P2PDMA_SUPPORTED: <documentation here...this is a good place to
>   * explain exactly what this flag is for.>
>   */
> #define DMA_F_PCI_P2PDMA_SUPPORTED     (1 << 0)
> 
> struct dma_map_ops {
> 	unsigned int flags;
> 

Sure, I don't care that much. I was just following the style in nvme.h.

>>   	void *(*alloc)(struct device *dev, size_t size,
>>   			dma_addr_t *dma_handle, gfp_t gfp,
>>   			unsigned long attrs);
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 50b8f586cf59..c31980ecca62 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -146,6 +146,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
>>   		unsigned long attrs);
>>   bool dma_can_mmap(struct device *dev);
>>   int dma_supported(struct device *dev, u64 mask);
>> +bool dma_pci_p2pdma_supported(struct device *dev);
>>   int dma_set_mask(struct device *dev, u64 mask);
>>   int dma_set_coherent_mask(struct device *dev, u64 mask);
>>   u64 dma_get_required_mask(struct device *dev);
>> @@ -247,6 +248,10 @@ static inline int dma_supported(struct device *dev, u64 mask)
>>   {
>>   	return 0;
>>   }
>> +static inline bool dma_pci_p2pdma_supported(struct device *dev)
>> +{
>> +	return 0;
> 
> Should be:
> 	
> 	return false;

Yup, will fix.

>> +}
>>   static inline int dma_set_mask(struct device *dev, u64 mask)
>>   {
>>   	return -EIO;
>> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
>> index 923089c4267b..ce44a0fcc4e5 100644
>> --- a/kernel/dma/mapping.c
>> +++ b/kernel/dma/mapping.c
>> @@ -573,6 +573,24 @@ int dma_supported(struct device *dev, u64 mask)
>>   }
>>   EXPORT_SYMBOL(dma_supported);
>>   
>> +bool dma_pci_p2pdma_supported(struct device *dev)
>> +{
>> +	const struct dma_map_ops *ops = get_dma_ops(dev);
>> +
>> +	/* if ops is not set, dma direct will be used which supports P2PDMA */
>> +	if (!ops)
>> +		return true;
>> +
>> +	/*
>> +	 * Note: dma_ops_bypass is not checked here because P2PDMA should
>> +	 * not be used with dma mapping ops that do not have support even
>> +	 * if the specific device is bypassing them.
>> +	 */
>> +
>> +	return ops->flags & DMA_F_PCI_P2PDMA_SUPPORTED;
> 
> Wow, rather unusual combination of things in order decide this. It feels
> a bit over-complicated to have flags and ops and a bool function all
> dealing with the same 1-bit answer, but there is no caller shown here,
> so I'll have to come back to this after reviewing subsequent patches.

Yeah, I originally had it much simpler, but it confused Ira and it was
clear it had to be written more explicitly and commented better.

Logan