From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2018 05:08:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,470,1515484800"; d="scan'208";a="25613352" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga006.jf.intel.com with ESMTP; 14 Mar 2018 05:08:26 -0700 To: Xiao Wang , dev@dpdk.org 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 References: <20180309230809.63361-1-xiao.w.wang@intel.com> <20180309230809.63361-2-xiao.w.wang@intel.com> From: "Burakov, Anatoly" Message-ID: <009ea590-d174-c153-7190-83b240b2053a@intel.com> Date: Wed, 14 Mar 2018 12:08:25 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180309230809.63361-2-xiao.w.wang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/3] eal/vfio: add support for multiple container X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Mar 2018 12:08:31 -0000 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