Re: [Qemu-devel] [PATCH 0/1] vhost-user: Add a protocol extension for client responses to vhost commands.

[Michael S. Tsirkin]

On Sun, Jun 26, 2016 at 02:48:09AM +0000, Prerna Saxena wrote:
> 
> 
> 
> 
> 
> On 26/06/16 8:15 am, "Michael S. Tsirkin" <address@hidden> wrote:
> 
> >On Sat, Jun 25, 2016 at 03:13:54AM +0000, Prerna Saxena wrote:
> >> 
> >> 
> >> 
> >> 
> >> 
> >> On 25/06/16 4:43 am, "Michael S. Tsirkin" <address@hidden> wrote:
> >> 
> >> >On Fri, Jun 24, 2016 at 05:39:31PM +0000, Prerna Saxena wrote:
> >> >> 
> >> >> 
> >> >> On 24/06/16 9:15 pm, "Felipe Franciosi" <address@hidden> wrote:
> >> >> 
> >> >> >We talked to MST on IRC a while back and he brainstormed the idea of 
> >> >> >doing this per-message.
> >> >> >(I even recall proposing to call this feature REPLY_ALL and he 
> >> >> >suggested REPLY_ANY due to that.)
> >> >> >
> >> >> >I agree with doing it per message, as the protocol itself should be 
> >> >> >flexible in that sense.
> >> >> >(Even if qemu today will probably want to ask for a reply in all 
> >> >> >messages.)
> >> >> 
> >> >> In fact, the current implementation does exactly this. If 
> >> >> VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, the current QEMU patch 
> >> >> sets the NEED_RESPONSE flag bit for all outgoing messages — basically 
> >> >> enforcing the vhost-user application to respond to all messages.
> >> >
> >> >
> >> >This seems unnecessary. Let's only do that for messages that actually
> >> >need to be synchronous.
> >> 
> >> It would be nice to distinguish the vhost-user protocol itself from its 
> >> QEMU implementation.
> >> The protocol should, in theory, have provision for an implementation (such 
> >> as QEMU’s vhost-user implementation) to seek response for _any_ command. 
> >> However, we can choose to be selective in our QEMU implementation and just 
> >> have limited commands currently send a response, such as SET_MEM_TABLE. 
> >> 
> >> In other words, we will still require the NEED_RESPONSE flag bit defined, 
> >> but we can just set it to 1 it for SET_MEM_TABLE command in our QEMU 
> >> implementation. All other vhost-user commands are sent from QEMU setting 
> >> this to 0, so the application does not send an ack.
> >> 
> >> Michael, Does that correctly summarize what you were meaning to suggest 
> >> here ?
> >> 
> >> Regards,
> >> Prerna
> >
> >Exactly.
> 
> Thanks for your response. I will rework and send out a patch to that end.
> 
> Regards,
> Prerna

And if the feature is not supported, work around that
by sending e.g. GET_FEATURES afterwards.

> >
> >> 
> >> >
> >> >> >
> >> >> >On 24/06/2016, 14:59, "Qemu-devel on behalf of Marc-André Lureau" 
> >> >> ><address@hidden on behalf of address@hidden> wrote:
> >> >> >
> >> >> >Hi
> >> >> >
> >> >> >On Fri, Jun 24, 2016 at 10:17 AM, Prerna Saxena <address@hidden> wrote:
> >> >> >> From: Prerna Saxena <address@hidden>
> >> >> >>
> >> >> >> 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
> >> >> >>
> >> >> >> 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 like the idea, as I encountered a similar issue in my
> >> >> >"vhost-user-gpu" development (which I worked around by sending a dump
> >> >> >GET_FEATURES.. to sync things). But I question the need to have a flag
> >> >> >per message. I think if the protocol feature is negociated, all
> >> >> >messages should have a reply. Why do you want it to be per-message?
> >> >> >
> >> >> >thanks
> >> >> >
> >> >> >-- 
> >> >> >Marc-André Lureau
> >> >> >
> >> >> >
> >> >> >