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

[John Johnson]

> On Dec 16, 2022, at 3:31 AM, Cédric Le Goater <clg@redhat.com> wrote:
> 
> 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,
> 

        Thank you for taking the time to review it

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

        A QEMU server patch is already upstream, which allows QEMU to
act as a vfio-user device server.  We’ve tested with some disk devices
(mpt, lsi895)


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

        We have an avocado test using an mpt disk controller that didn’t
get pushed with the server changes since the client isn’t upstream.  I’ll
add it to this patch series.


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

        I was asked to look at using the socket command line parser,
but its visitor function doesn’t work with device property list options,
so the compromise was to use a SocketAddress as the argument to
vfio_user_connect_dev, so vfio-user can be changed without much work if
the visitor changes.


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

        I have a patch series that renames the struct to your preference
and removes the virtual function macros.  It will be out shortly.


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

        I did refactor the IOMMU group code so vfio-user and the kernel
version can share common.c routines.  I hope this helps.


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

        It was done more as a place to hold bug catching code than as
an optimization.  e.g., If I misplace a msg, I can usually find it on
free list.


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

        We arrived at this setup from a KVM forum in 2019, where several
groups wanted to do device emulation outside the VMM for security and
perfomance reasons.  We merged a couple projects together into vfio-user.

        Thanks again for the feedback.
                                                                JJ