Hi Marc-Andre, Thank you for taking a look.
On 03/07/16 5:17 pm, "Marc-André Lureau" <marcandre.lur...@gmail.com> wrote: >Hi > >On Fri, Jul 1, 2016 at 11:46 AM, Prerna Saxena <saxenap....@gmail.com> 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. >> >> 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. >> > >Overall, that looks good to me. > >Why do we have both "response" and "reply" which basically means the >same thing, right? I would rather stick with "reply". Allright, will rename this function to process_message_reply(). > >I am not convinced the first patch is needed, imho it is a >workaround/hack, the solution is given with the patch 2 only. Great, I’ll post a v3 with just Patch2. Regards, Prerna > >> 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 >> > > > >-- >Marc-André Lureau >
