Re: [PATCH V4 3/5] vDPA: introduce vDPA bus

[Jason Wang]

On 2020/2/20 äå11:14, Jason Gunthorpe wrote:
> On Thu, Feb 20, 2020 at 02:11:39PM +0800, Jason Wang wrote:
> > vDPA device is a device that uses a datapath which complies with the
> > virtio specifications with vendor specific control path. vDPA devices
> > can be both physically located on the hardware or emulated by
> > software. vDPA hardware devices are usually implemented through PCIE
> > with the following types:
> >
> > - PF (Physical Function) - A single Physical Function
> > - VF (Virtual Function) - Device that supports single root I/O
> >    virtualization (SR-IOV). Its Virtual Function (VF) represents a
> >    virtualized instance of the device that can be assigned to different
> >    partitions
> > - ADI (Assignable Device Interface) and its equivalents - With
> >    technologies such as Intel Scalable IOV, a virtual device (VDEV)
> >    composed by host OS utilizing one or more ADIs. Or its equivalent
> >    like SF (Sub function) from Mellanox.
> >
> >  From a driver's perspective, depends on how and where the DMA
> > translation is done, vDPA devices are split into two types:
> >
> > - Platform specific DMA translation - From the driver's perspective,
> >    the device can be used on a platform where device access to data in
> >    memory is limited and/or translated. An example is a PCIE vDPA whose
> >    DMA request was tagged via a bus (e.g PCIE) specific way. DMA
> >    translation and protection are done at PCIE bus IOMMU level.
> > - Device specific DMA translation - The device implements DMA
> >    isolation and protection through its own logic. An example is a vDPA
> >    device which uses on-chip IOMMU.
> >
> > To hide the differences and complexity of the above types for a vDPA
> > device/IOMMU options and in order to present a generic virtio device
> > to the upper layer, a device agnostic framework is required.
> >
> > This patch introduces a software vDPA bus which abstracts the
> > common attributes of vDPA device, vDPA bus driver and the
> > communication method (vdpa_config_ops) between the vDPA device
> > abstraction and the vDPA bus driver. This allows multiple types of
> > drivers to be used for vDPA device like the virtio_vdpa and vhost_vdpa
> > driver to operate on the bus and allow vDPA device could be used by
> > either kernel virtio driver or userspace vhost drivers as:
> >
> >     virtio drivers  vhost drivers
> >            |             |
> >      [virtio bus]   [vhost uAPI]
> >            |             |
> >     virtio device   vhost device
> >     virtio_vdpa drv vhost_vdpa drv
> >               \       /
> >              [vDPA bus]
> >                   |
> >              vDPA device
> >              hardware drv
> >                   |
> >              [hardware bus]
> >                   |
> >              vDPA hardware
> I still don't like this strange complexity, vhost should have been
> layered on top of the virtio device instead of adding an extra bus
> just for vdpa.


We've considered such method and I think why we choose a bus is:

- vDPA device was originally named as "vhost Datapath Acceleration" 
which means the datapath complies virtio specification but not control 
path. This means the device should behave like vhost. And in order to 
support vhost, vDPA device requires more function than virtio. E.g the 
ability to query the device state (virtqueue indices, counters etc) and 
track dirty pages. This mean even a pure virtio hardware may not work 
for vhost. That's why a multi inheritance is used for a new type of vDPA 
device.

- As we've already discussed, virtio bus is designed for kernel driver 
and a brunches of devices, drivers or even buses have been implemented 
around that. It requires a major refactoring not only with the virtio 
bus but also with the drivers and devices to make it behave more like a 
vhost. Abstract vDPA as a kind of transport for virtio greatly simplify 
the work and have almost zero impact on the exist virtio core. VOP 
(vop_bus) use similar design.


> However, I don't see any technical problems with this patch now.


Thanks, your review is greatly appreciated.


> Thanks,
> Jason