Re: [PATCH v1 00/24] vfio-user client

["Cédric Le Goater" <clg@redhat.com>]

On 11/9/22 00:13, John Johnson wrote:
> 
> Hello,
> 
> This is the 6th revision of the vfio-user client implementation.
> It is the first patch series (the previous revisions were RFCs)
> 
> First of all, thank you for your time reviewing the RFC versions.
> 
> The vfio-user framework consists of 3 parts:
>   1) The VFIO user protocol specification.
>   2) A client - the VFIO device in QEMU that encapsulates VFIO messages
>      and sends them to the server.
>   3) A server - a remote process that emulates a device.
> 
> This patchset implements parts 1 and 2.
> 
> The libvfio-user project (https://github.com/nutanix/libvfio-user)
> can be used by a remote process to handle the protocol to implement the third part.
> We also have upstreamed a patch series that implement a server using QEMU.
> 

Thanks for this contribution,


This is a large and complex framework which looks quite mature. QEMU
would need implementations of remote devices under contrib/, provided
as examples for the QEMU crowd. These are important to show possible
use cases and also, they could possibly be run for non regression tests
done by maintainers and distros. e1000e is quite popular, it would
be a good target. It could be a simpler device to start with, but we
would need to cover the basic features, INTx, MSI, DMA, etc. when time
permits. There are samples under libvfio-user and I wonder how we
could leverage them.

Unit tests under /tests would be (really) good to have. These would be
run by CI. Yes, this is a lot of work :/ but very important for QEMU
stability.

The "socket" name property is not the preferred way in QEMU to define
a remote peer to contact. Is there a possibility to use the chardev
interface in some simple way ? I am thinking about the command line
interface and the integration with libvirt.

More code should be isolated under the *user.c file, even if that
means exporting definitions of routines which are local today. I don't
think the #ifdef CONFIG_VIO_USER in the vfio files are something we
will want to merge.

Some renaming to be done, like Kern -> Kernel, etc. nothing major.

I am not convinced that the macros hiding the VFIO backend IO ops are
useful. I even recall some places where the vfio-user implemented
handler could be called directly without calling the top routine first.
Anyhow, it would be better to be explicit, the macros don't add much.

There is a lot of code duplication for the IOMMU support. Did you
consider implementing a VFIOGroup class to share the common behaviour ?
May be too early, but this is certainly something to keep in mind.

The msg recycling feature looks nice but isn't it an optimization ?

More globally speaking, for what kind of crazy setup this feature could
help us with ? I was wondering if you had tried to implement a remote
device or bridge in another QEMU process, for instance.

Thanks,

C.



> Contributors:
> 
> John G Johnson <john.g.johnson@oracle.com>
> John Levon <john.levon@nutanix.com>
> Thanos Makatos <thanos.makatos@nutanix.com>
> Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Jagannathan Raman <jag.raman@oracle.com>
> 
> John Johnson (23):
>    vfio-user: add VFIO base abstract class
>    vfio-user: add container IO ops vector
>    vfio-user: add region cache
>    vfio-user: add device IO ops vector
>    vfio-user: Define type vfio_user_pci_dev_info
>    vfio-user: connect vfio proxy to remote server
>    vfio-user: define socket receive functions
>    vfio-user: define socket send functions
>    vfio-user: get device info
>    vfio-user: get region info
>    vfio-user: region read/write
>    vfio-user: pci_user_realize PCI setup
>    vfio-user: get and set IRQs
>    vfio-user: forward msix BAR accesses to server
>    vfio-user: proxy container connect/disconnect
>    vfio-user: dma map/unmap operations
>    vfio-user: add dma_unmap_all
>    vfio-user: secure DMA support
>    vfio-user: dma read/write operations
>    vfio-user: pci reset
>    vfio-user: add 'x-msg-timeout' option that specifies msg wait times
>    vfio-user: add coalesced posted writes
>    vfio-user: add trace points
> 
> Thanos Makatos (1):
>    vfio-user: introduce vfio-user protocol specification
> 
>   MAINTAINERS                    |   11 +
>   docs/devel/index-internals.rst |    1 +
>   docs/devel/vfio-user.rst       | 1522 ++++++++++++++++++++++++++++++++
>   hw/vfio/Kconfig                |   10 +
>   hw/vfio/ap.c                   |    1 +
>   hw/vfio/ccw.c                  |    7 +-
>   hw/vfio/common.c               |  648 ++++++++++++--
>   hw/vfio/igd.c                  |   23 +-
>   hw/vfio/meson.build            |    1 +
>   hw/vfio/migration.c            |    2 -
>   hw/vfio/pci-quirks.c           |   19 +-
>   hw/vfio/pci.c                  |  926 ++++++++++++++-----
>   hw/vfio/pci.h                  |   29 +-
>   hw/vfio/platform.c             |    2 +
>   hw/vfio/trace-events           |   15 +
>   hw/vfio/user-protocol.h        |  244 +++++
>   hw/vfio/user.c                 | 1904 ++++++++++++++++++++++++++++++++++++++++
>   hw/vfio/user.h                 |  112 +++
>   include/hw/vfio/vfio-common.h  |   82 ++
>   19 files changed, 5230 insertions(+), 329 deletions(-)
>   create mode 100644 docs/devel/vfio-user.rst
>   create mode 100644 hw/vfio/user-protocol.h
>   create mode 100644 hw/vfio/user.c
>   create mode 100644 hw/vfio/user.h
>