> On 24 Jun 2016, at 23:43, Michael S. Tsirkin <m...@redhat.com> wrote: > > On Fri, Jun 24, 2016 at 01:17:07AM -0700, Prerna Saxena wrote: >> From: Prerna Saxena <prerna.sax...@nutanix.com> >> >> The current vhost-user protocol requires the client to send responses to >> only few commands. For the remaining commands, it is impossible for QEMU to >> know the status of the requested operation -- ie, did it succeed at all, and >> if so, at 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) and SET_MEM_TABLE doesn't 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 doesn't know about >> those GPAs > > OK, but there's a simple way to fix the race: submit > another request e.g. GET_FEATURES and wait for a response.
Actually, that doesn't guarantee to fix the problem. I argue it's quite a poor workaround. The protocol doesn't specify that messages need to be processed in series. I could easily see a client application receiving a SET_MEM_TABLE message and dispatching a task to process it asynchronously. The same application could respond to a following GET_FEATURES in line, since the command requires a response, but no extra processing. In summary, qemu could get a response for GET_FEATURES while SET_MEM_TABLE is being processed in the background at the client application. It might seem like this is unlikely to happen, but consider that a multi-threaded application might need to take a lock in order to process SET_MEM_TABLE. For example, waiting for all virtqueues being processed to quiesce. > > >> >> 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. This patch introduces a protocol extension, >> VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU to >> annotate messages to the application that it seeks a response for. The >> application must then respond to qemu by providing a status about the >> requested operation. > > > I would like this structured like this: > > 1. fix races by sending GET_FEATURES and waiting for it We can still add this first if you think it will help. But is it worth it? Asking for a reply to certain commands is clearly a better fix. Thanks, Felipe > 2. add VHOST_USER_PROTOCOL_F_REPLY_ACK > > >> >> Prerna Saxena (1): >> vhost-user : Introduce a new feature, VHOST_USER_PROTOCOL_F_REPLY_ACK >> This feature, if negotiated, forces the remote vhost-user >> process to send a u64 reply containin status code for each >> requested operation. >> Status codes are '0' for success, and non-zero for error. >> >> docs/specs/vhost-user.txt | 36 +++++++++++ >> hw/virtio/vhost-user.c | 153 >> +++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 186 insertions(+), 3 deletions(-) >> >> -- >> 1.8.1.2 >> >
