Hi Marc-André, thanks for the comments.
On 12/20/2016 12:43 AM, Marc-André Lureau wrote:
Hi Wei,
On Mon, Dec 19, 2016 at 7:00 AM Wei Wang <address@hidden
<mailto:address@hidden>> wrote:
This patch series implements vhost-pci, which is a point-to-point
based inter-vm
communication solution. The QEMU side implementation includes the
vhost-user
extension, vhost-pci device emulation and management. The current
device part
implementation is based on virtio 1.0, but it can be easily
upgraded to support
the upcoming virtio 1.1.
The current QEMU implementation supports the polling mode driver
on both sides
to receive packets. More features, such as interrupt support, live
migration
support, protected memory accesses will be added later.
I highly appreciate the effort you put in splitting the patch series
and commenting each, although some are probably superfluous.
I will see if I can combine some of the unnecessary splits in the next
version.
High level question, why do you need to create device dynamically? I
would rather have the following qemu setup:
-chardev socket,id=chr,path=.. -device vhost-pci-net,chardev=chr
This would also avoid some global state (vp_slave etc)
With the above commands, the slave QEMU will create and plug in the
vhost-pci-net device at the QEMU booting time. I think this won't
work, because at the slave QEMU booting time, the master, in most
cases, hasn't connected to the slave to transmit the info (e.g. memory
info) that's required for the device setup - for example, the device
bar can't be constructed without the memory info passed from the
master, and if the device is created and plugged in the VM without a
bar at the beginning, I think we don't have another chance to add a
bar on the fly when the device is already driven in the guest. That's
the reason that I get a global vp_slave to manage (dynamically
create/destroy a device when requested by the master) the vhost-pci
device.
Regarding the protocol changes to support slave request: I tried to
explained that before, apprently I didn't manage to. It is not enough
to support bidirectionnal communications to simply add chardev
frontend handlers. Until now, qemu's code expects an immediate reply
after a request. With your protocol change, it must now also consider
that the slave may send a request before the master request reaches
the slave handler. So all req/reply write()/read() must now handle in
between requests from slave to be race-free (master can read back a
request when it expects a reply). That's not really trivial change,
that's why I proposed to have a secondary channel for slave->master
communications in the past. Not only would this be easier to
implement, but the protocol documentation would also be simpler, the
cost is simply 1 additional unix socket (that I proposed to setup and
pass with ancilliary data on the main channel).
I don't disagree with the second channel method. That implementation
hasn't been in the upstream QEMU yet, are you planning to get it in
first, so that we can upstream vhost-pci based on that?
RESEND change: Fixed some coding style issue
there are some spelling to be fixed, and perhaps some
variables/fields to rename (asyn -> async, crq -> ctrlq?) That can
be addressed in a detailed review.
Sure, I will fix them.