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

[Wang, Xiao W]

Hi Anatoly,

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Wednesday, March 14, 2018 8:08 PM
> To: Wang, Xiao W <xiao.w.wang at intel.com>; dev at dpdk.org
> Cc: Wang, Zhihong <zhihong.wang at intel.com>;
> maxime.coquelin at redhat.com; yliu at fridaylinux.org; Liang, Cunming
> <cunming.liang at intel.com>; Xu, Rosen <rosen.xu at intel.com>; Chen, Junjie J
> <junjie.j.chen at intel.com>; Daly, Dan <dan.daly at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3] eal/vfio: add support for multiple
> container
> 
> 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?

The device driver needs to have a separate container to program IOMMU
For the device, with the VM's addr translation table. So driver needs the
Devices be put into new containers, rather than the default one.

> 
> <...>
> 
> 
> > +	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?

This driver are creating new containers for devices, it needs each device
to have its own container, then we can dma_map/ummap for the device
via it's associated container.

BRs,
Xiao

> 
> 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