Re: [dpdk-dev] [PATCH] vhost: fix async DMA map

["Ding, Xuan" <xuan.ding@intel.com>]

Hi Maxime,

>-----Original Message-----
>From: Maxime Coquelin <maxime.coquelin@redhat.com>
>Sent: Tuesday, October 26, 2021 4:47 AM
>To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
><chenbo.xia@intel.com>; Ding, Xuan <xuan.ding@intel.com>
>Subject: Re: [PATCH] vhost: fix async DMA map
>
>Hi Xuan,
>
>On 10/25/21 22:33, Maxime Coquelin wrote:
>> This patch fixes possible NULL-pointer dereferencing
>> reported by Coverity and also fixes NUMA reallocation
>> of the async DMA map.
>>
>> Fixes: 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
>>
>> Coverity issue: 373655
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/vhost_user.c | 45 +++++++++++++++++++-----------------------
>>   1 file changed, 20 insertions(+), 25 deletions(-)
>>
>
>I posted this patch to fix the issue reported by Coverity and also other
>issue on NUMA realloc that I found at the same time. But I wonder
>whether all this async_map_status is needed.

Thanks for your fix! I can help to review and test the patch later.

I add the async_map_status in v2 for compatibility. Some DMA device,
like DSA, may use kernel idxd driver only. If there is no device bound to
DPDK vfio and kernel vfio module is modprobed to ensure rte_vfio_is_enabled() is true,
we will unavoidably do DMA map/unmap and it will fail.

Therefore, the dma_map_status here is used to filter this situation by preventing
unnecessary DMA unmap.

>
>Indeed, if the only place where we DMA map is in
>vhost_user_mmap_region(). If it fails, the error is propagated, the mem
>table are freed and NACK is replied to the master. IOW, the device will
>be in an unusable state.

I agree with you, this is the place I consider right to do DMA map
because we also do SW mapping here, any suggestions?

>
>Removing the async DMA map will simplify a lot the code, do you agree to
>remove it or there is something I missed?

See above. Indeed, it adds a lot of code. But we can't know the driver for
each device in vhost lib, or we can only restrict the user to bind some devices
to DPDK vfio if async logic needed.

>
>Thanks,
>Maxime