Re: [PATCH] NTB: ntb_perf: Fix address err in perf_copy_chunk

[Logan Gunthorpe]

On 2019-11-11 12:51 a.m., Jiasen Lin wrote:
> 
> 
> On 2019/11/9 1:04, Logan Gunthorpe wrote:
>>
>>
>> On 2019-11-06 8:38 p.m., Jiasen Lin wrote:
>>> peer->outbuf is a virtual address which is get by ioremap, it can not
>>> be converted to a physical address by virt_to_page and page_to_phys.
>>> This conversion will result in DMA error, because the destination address
>>> which is converted by page_to_phys is invalid.
>>
>> Hmm, yes, ntb_perf is obviously wrong. I never noticed this, how did
>> this ever work?
>>
> 
> The default value of use_dma which is used to enable DMA engine to 
> measure NTB performance is zero, in other words, DMA engine is not used 
> by default. Thus, olny memcpy_toio is called in perf_copy_chunk and not 
> trigger this bug.
> 
> If we install driver with specified dma-enabled flag like this:
> insmod ntb_perf.ko use_dma=1, this issue will be triggered.

I've definitely tested with use_dma=1 in the past. But it looks like it
was broken by this problematic commit and I must have never personally
run the DMA tests after it:

5648e56d03fa ("NTB: ntb_perf: Add full multi-port NTB API support")

So it's probably worth adding a fixes tag to your patch.

>>> We Save the physical address in perf_setup_peer_mw, it is MMIO address
>>> of NTB BARx. Then fill the destination address of DMA descriptor with
>>> this physical address to guarantee that the address of memory write
>>> requests fall in memory window of NBT BARx.
>>
>> Using the physical address directly is also wrong and will not work in
>> the presence of an IOMMU. You should use dma_map_resource() for this.
>> See what was done in ntb_transport[1].
>>
> 
> Yes, my mistake. I will modify the code according to your suggestion and 
> test it on AMD and HYGON platforms with the IOMMU enabled. Maybe the 
> following patches are relied on, when IOMMU is enabled on AMD and HYGON 
> plartforms.
> 
> https://lore.kernel.org/patchwork/cover/1143155/
> https://lore.kernel.org/patchwork/patch/1143156/
> https://lore.kernel.org/patchwork/patch/1143157/

Thanks!

Logan