On Fri, Jul 01, 2016 at 02:46:20AM -0700, Prerna Saxena wrote: > From: Prerna Saxena <prerna.sax...@nutanix.com> > > The current vhost-user protocol requires the client to send responses to only > a > few commands. For the remaining commands, it is impossible for QEMU to know > the > status of the requested operation -- ie, did it succeed? If so, by what time? > > This is inconvenient, and can also lead to races. As an example: > > (1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net > application). > Note that SET_MEM_TABLE does not require a reply according to the spec. > (2) Qemu commits the memory to the guest. > (3) Guest issues an I/O operation over a new memory region which was > configured on (1). > (4) The application hasn't yet remapped the memory, but it sees the I/O > request. > (5) The application cannot satisfy the request because it does not know about > those GPAs. > > Note that the kernel implementation does not suffer from this limitation > since messages are sent via an ioctl(). The ioctl() blocks until the backend > (eg. vhost-net) completes the command and returns (with an error code). > > Changing the behaviour of current vhost-user commands would break existing > applications. > To work around this race, Patch 1 adds a get_features command to be sent > before returning from set_mem_table. While this is not a complete fix, it > will help client applications that strictly process messages in order. > > The second patch introduces a protocol extension, > VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to > request a response to any message by setting the newly introduced > "need_response" flag. The application must then respond to qemu by providing > a status about the requested operation.
OK this all looks very reasonable (and I do like patch 1 too) but there's one source of waste here: we do not need to synchronize when we set up device the first time when hdev->memory_changed is false. I think we should test that and skip synch in both patches unless hdev->memory_changed is set. > Changelog: > --------- > Changes since v1: > Patch 1 : Ask for get_features before returning from set_mem_table(new). > Patch 2 : * Improve documentation. > * Abstract out commonly used operations in the form of a function, > process_message_response(). Also implement this only for SET_MEM_TABLE. > > Prerna Saxena (2): > vhost-user: Attempt to prevent a race on set_mem_table. > vhost-user : Introduce a new feature VHOST_USER_PROTOCOL_F_REPLY_ACK. > > docs/specs/vhost-user.txt | 40 ++++++++++++ > hw/virtio/vhost-user.c | 157 > ++++++++++++++++++++++++++++++++-------------- > 2 files changed, 150 insertions(+), 47 deletions(-) > > -- > 1.8.1.2
