Hi On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <saxenap....@gmail.com> wrote: > From: Prerna Saxena <prerna.sax...@nutanix.com> > > This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK. > > If negotiated, client applications should send a u64 payload in > response to any message that contains the "need_reply" bit set > on the message flags. Setting the payload to "zero" indicates the > command finished successfully. Likewise, setting it to "non-zero" > indicates an error. > > Currently implemented only for SET_MEM_TABLE. > > Signed-off-by: Prerna Saxena <prerna.sax...@nutanix.com> > --- > docs/specs/vhost-user.txt | 26 ++++++++++++++++++++++++++ > hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > index 777c49c..54b5c8f 100644 > --- a/docs/specs/vhost-user.txt > +++ b/docs/specs/vhost-user.txt > @@ -37,6 +37,8 @@ consists of 3 header fields and a payload: > * Flags: 32-bit bit field: > - Lower 2 bits are the version (currently 0x01) > - Bit 2 is the reply flag - needs to be sent on each reply from the slave > + - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for > + details. > * Size - 32-bit size of the payload > > > @@ -126,6 +128,8 @@ the ones that do: > * VHOST_GET_VRING_BASE > * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) > > +[ Also see the section on REPLY_ACK protocol extension. ] > + > There are several messages that the master sends with file descriptors passed > in the ancillary data: > > @@ -254,6 +258,7 @@ Protocol features > #define VHOST_USER_PROTOCOL_F_MQ 0 > #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1 > #define VHOST_USER_PROTOCOL_F_RARP 2 > +#define VHOST_USER_PROTOCOL_F_REPLY_ACK 3 > > Message types > ------------- > @@ -464,3 +469,24 @@ Message types > is present in VHOST_USER_GET_PROTOCOL_FEATURES. > The first 6 bytes of the payload contain the mac address of the guest > to > allow the vhost user backend to construct and broadcast the fake RARP. > + > +VHOST_USER_PROTOCOL_F_REPLY_ACK: > +------------------------------- > +The original vhost-user specification only demands replies for certain > +commands. This differs from the vhost protocol implementation where commands > +are sent over an ioctl() call and block until the client has completed. > + > +With this protocol extension negotiated, the sender (QEMU) can set the > +"need_reply" [Bit 3] flag to any command. This indicates that > +the client MUST respond with a Payload VhostUserMsg indicating success or > +failure. The payload should be set to zero on success or non-zero on failure. > +(Unless the message already has an explicit reply body)
Unless/unless (for consistency, the rest of the document doesn't use Upper-case inside parentheses) > + > +This indicates to QEMU that the requested operation has deterministically > +been met or not. Today, QEMU is expected to terminate the main vhost-user > +loop upon receiving such errors. In future, qemu could be taught to be more > +resilient for selective requests. > + > +For the message types that already solicit a reply from the client, the > +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set > brings > +no behaviourial change. (See the 'Communication' section for details.) See/see > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 495e09f..86e7ae0 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -31,6 +31,7 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_MQ = 0, > VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1, > VHOST_USER_PROTOCOL_F_RARP = 2, > + VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, > > VHOST_USER_PROTOCOL_F_MAX > }; > @@ -84,6 +85,7 @@ typedef struct VhostUserMsg { > > #define VHOST_USER_VERSION_MASK (0x3) > #define VHOST_USER_REPLY_MASK (0x1<<2) > +#define VHOST_USER_NEED_REPLY_MASK (0x1 << 3) You could align it, and use the same style as the line above > uint32_t flags; > uint32_t size; /* the following payload size */ > union { > @@ -158,6 +160,25 @@ fail: > return -1; > } > > +static int process_message_reply(struct vhost_dev *dev, > + VhostUserRequest request) bad indentation > +{ > + VhostUserMsg msg; > + > + if (vhost_user_read(dev, &msg) < 0) { > + return 0; return -1 > + } > + > + if (msg.request != request) { > + error_report("Received unexpected msg type." > + "Expected %d received %d", > + request, msg.request); bad indentation > + return -1; > + } > + > + return msg.payload.u64 ? -1 : 0; > +} > + > static bool vhost_user_one_time_request(VhostUserRequest request) > { > switch (request) { > @@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev > *dev, > int fds[VHOST_MEMORY_MAX_NREGIONS]; > int i, fd; > size_t fd_num = 0; > + bool reply_supported = virtio_has_feature(dev->protocol_features, > + VHOST_USER_PROTOCOL_F_REPLY_ACK); bad indentation > + > VhostUserMsg msg = { > .request = VHOST_USER_SET_MEM_TABLE, > .flags = VHOST_USER_VERSION, > }; > > + if (reply_supported) { > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > + } > + > for (i = 0; i < dev->mem->nregions; ++i) { > struct vhost_memory_region *reg = dev->mem->regions + i; > ram_addr_t offset; > @@ -277,6 +305,10 @@ static int vhost_user_set_mem_table(struct vhost_dev > *dev, > > vhost_user_write(dev, &msg, fds, fd_num); > > + if (reply_supported) { > + return process_message_reply(dev, msg.request); > + } > + > return 0; > } > > -- > 1.8.1.2 > Other than that, looks good to me, with those fixes Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> -- Marc-André Lureau