From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CA446A0C47; Tue, 26 Oct 2021 08:53:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 484A940A4B; Tue, 26 Oct 2021 08:53:16 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 242DC4003E for ; Tue, 26 Oct 2021 08:53:15 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635231194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IW/y4XYqfAbIL5H/+Nh329PxwIY1eEtcnrRH7Cpi88c=; b=BsGpWtF1xalcT37758/bFyfbTwNXB/iDEg7s2NMrKvgXBepfUWRQOh29//QFBSeyWLT3Y2 6AzEJPURorLTz21l/L0pwQfKuwA2I9VTDw1cs2dQYxE4jBi78Le2o+jkItXBfVkhBG12Fk W7jFMTU1IsJ0F1F0+NHt6Uwwmx4tNxE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-198-I2NsuNbsOBGdNX3Ko0t8NQ-1; Tue, 26 Oct 2021 02:53:06 -0400 X-MC-Unique: I2NsuNbsOBGdNX3Ko0t8NQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 506D479EE8; Tue, 26 Oct 2021 06:53:05 +0000 (UTC) Received: from [10.39.208.37] (unknown [10.39.208.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 020225C1A1; Tue, 26 Oct 2021 06:52:59 +0000 (UTC) Message-ID: Date: Tue, 26 Oct 2021 08:52:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 To: "Ding, Xuan" , "dev@dpdk.org" , "david.marchand@redhat.com" , "Xia, Chenbo" Cc: "Burakov, Anatoly" References: <20211025203353.147346-1-maxime.coquelin@redhat.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] vhost: fix async DMA map X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/26/21 04:07, Ding, Xuan wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Tuesday, October 26, 2021 4:47 AM >> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo >> ; Ding, Xuan >> 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 >>> --- >>> 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. Ok, then I think we can just remove the async DMA map. >> >> 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? No suggestion, I was just explaining that at the only place where DMA map were done, mapping errors were properly handled and propagated. >> >> 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. I would think we don't care if DMA unmap fails, we can just do the same as what you do for DMA map, i.e. just ignore the error. Thanks to this discussion, I have now more concerns on how it works. I think we have a problem here in case of DMA device hotplug, that device could miss the necessary map entries from Vhost if no VFIO devices were attached at VHST_USER_SET_MEM_TABLE time. How would you handle that case? Regards, Maxime >> >> Thanks, >> Maxime >