From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Burakov, Anatoly" Subject: Re: [PATCH 1/3] eal/vfio: add support for multiple container Date: Wed, 14 Mar 2018 12:08:25 +0000 Message-ID: <009ea590-d174-c153-7190-83b240b2053a@intel.com> References: <20180309230809.63361-1-xiao.w.wang@intel.com> <20180309230809.63361-2-xiao.w.wang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: zhihong.wang@intel.com, maxime.coquelin@redhat.com, yliu@fridaylinux.org, cunming.liang@intel.com, rosen.xu@intel.com, junjie.j.chen@intel.com, dan.daly@intel.com To: Xiao Wang , dev@dpdk.org Return-path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 794D4728B for ; Wed, 14 Mar 2018 13:08:29 +0100 (CET) In-Reply-To: <20180309230809.63361-2-xiao.w.wang@intel.com> Content-Language: en-US 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 09-Mar-18 11:08 PM, Xiao Wang wrote: > From: Junjie Chen > > Currently eal vfio framework binds vfio group fd to the default > container fd, while in some cases, e.g. vDPA (vhost data path > acceleration), we want to set vfio group to a new container and > program DMA mapping via this new container, so this patch adds > APIs to support multiple container. > > Signed-off-by: Junjie Chen > Signed-off-by: Xiao Wang > --- I'm not going to get into virtual vs. real device debate, but i do have some issues with VFIO side of things. I'm not completely convinced this change is needed in the first place. If the device driver manages its own groups anyway, it knows which VFIO groups belong to it, so it can add/remove them without putting them into separate containers. What is the purpose of keeping them in a separate container as opposed to just keeping track of group id's? <...> > + vfio_cfg->vfio_container_fd = vfio_get_container_fd(); > + > + if (vfio_cfg->vfio_container_fd < 0) > + return -1; > + > + return vfio_cfg->vfio_container_fd; > +} Please correct me if i'm wrong, but this patch appears to be mistitled. You're not really creating multiple containers, you're just partitioning existing one. Do we really need to open/store/close container fd's separately, if all we have is a single container anyway? The semantics of this are also weird in multiprocess. When secondary process requests a container, we always create a new one, send it over IPC and close it afterwards. It seems to be oblivious that you may have several container fd's, and does not know which one you are asking for. We know it's all the same container, but that's clearly not what the code appears to be doing. -- Thanks, Anatoly