[dpdk-dev] [PATCH 1/3] eal/vfio: add support for multiple container

[Burakov, Anatoly]

On 09-Mar-18 11:08 PM, Xiao Wang wrote:
> From: Junjie Chen <junjie.j.chen at intel.com>
> 
> 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 <junjie.j.chen at intel.com>
> Signed-off-by: Xiao Wang <xiao.w.wang at intel.com>
> ---

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