| Message ID | 1469689643-11556-2-git-send-email-saxenap.ltc@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena <saxenap.ltc@gmail.com> wrote: > From: Prerna Saxena <prerna.saxena@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.saxena@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.lureau@redhat.com>
On 07/28/2016 01:07 AM, Prerna Saxena wrote: > From: Prerna Saxena <prerna.saxena@nutanix.com> > > This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK. > > + > +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) Rather than make this parenthetical, I would go with: The payload should be set to zero on success or non-zero on failure, unless the message already has an explicit reply body. > + > +This indicates to QEMU that the requested operation has deterministically > +been met or not. Today, QEMU is expected to terminate the main vhost-user Reads awkwardly; maybe: The response payload gives QEMU a deterministic indication of the result of the command. > +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.) s/behaviourial/behavioural/ (or if the document widely favors US spelling, behavioral)
On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote: >On 07/28/2016 01:07 AM, Prerna Saxena wrote: >> From: Prerna Saxena <prerna.saxena@nutanix.com> >> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK. >> > >> + >> +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) > >Rather than make this parenthetical, I would go with: > >The payload should be set to zero on success or non-zero on failure, >unless the message already has an explicit reply body. Hi Eric, Thank you for taking a look, but I think you possibly missed the latest patchset posted last night. This had already been incorporated in v6 that I’d posted last night before your message. See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html > >> + >> +This indicates to QEMU that the requested operation has deterministically >> +been met or not. Today, QEMU is expected to terminate the main vhost-user > >Reads awkwardly; maybe: > >The response payload gives QEMU a deterministic indication of the result >of the command. Hmm, it is more of personal taste, so I’ll refrain from commenting either way. > >> +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.) > >s/behaviourial/behavioural/ (or if the document widely favors US >spelling, behavioral) The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-) May I request the maintainers to please correct this tiny spelling typo as this is checked in? Regards, Prerna
On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote: > > > > > > On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote: > > >On 07/28/2016 01:07 AM, Prerna Saxena wrote: > >> From: Prerna Saxena <prerna.saxena@nutanix.com> > >> > >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK. > >> > > > >> + > >> +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) > > > >Rather than make this parenthetical, I would go with: > > > >The payload should be set to zero on success or non-zero on failure, > >unless the message already has an explicit reply body. > > Hi Eric, > Thank you for taking a look, but I think you possibly missed the latest patchset posted last night. > This had already been incorporated in v6 that I’d posted last night before your message. > See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html > > > > > >> + > >> +This indicates to QEMU that the requested operation has deterministically > >> +been met or not. Today, QEMU is expected to terminate the main vhost-user > > > >Reads awkwardly; maybe: > > > >The response payload gives QEMU a deterministic indication of the result > >of the command. > > Hmm, it is more of personal taste, so I’ll refrain from commenting either way. I prefer Eric's form too. "that ... or not" isn't very clear. > > > >> +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.) > > > >s/behaviourial/behavioural/ (or if the document widely favors US > >spelling, behavioral) > > > The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-) > May I request the maintainers to please correct this tiny spelling typo as this is checked in? > > Regards, > Prerna Probably easier to post v7 with above minor things.
On 04/08/16 9:41 am, "Michael S. Tsirkin" <mst@redhat.com> wrote: >On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote: >> >> >> >> >> >> On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote: >> >> >On 07/28/2016 01:07 AM, Prerna Saxena wrote: >> >> From: Prerna Saxena <prerna.saxena@nutanix.com> >> >> >> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK. >> >> >> > >> >> + >> >> +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) >> > >> >Rather than make this parenthetical, I would go with: >> > >> >The payload should be set to zero on success or non-zero on failure, >> >unless the message already has an explicit reply body. >> >> Hi Eric, >> Thank you for taking a look, but I think you possibly missed the latest patchset posted last night. >> This had already been incorporated in v6 that I’d posted last night before your message. >> See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html >> >> >> > >> >> + >> >> +This indicates to QEMU that the requested operation has deterministically >> >> +been met or not. Today, QEMU is expected to terminate the main vhost-user >> > >> >Reads awkwardly; maybe: >> > >> >The response payload gives QEMU a deterministic indication of the result >> >of the command. >> >> Hmm, it is more of personal taste, so I’ll refrain from commenting either way. > >I prefer Eric's form too. "that ... or not" isn't very clear. Done. > >> > >> >> +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.) >> > >> >s/behaviourial/behavioural/ (or if the document widely favors US >> >spelling, behavioral) >> >> >> The last 3 iterations of this patchset have only seen review comments focussed on documentation suggestions and indentation of code, but nothing on the idea/code itself. This gives me hope that the patch is possibly close to merging within 2.7 timeframe :-) >> May I request the maintainers to please correct this tiny spelling typo as this is checked in? >> >> Regards, >> Prerna > >Probably easier to post v7 with above minor things. Posted a v7 which incorporates all suggestions made by Eric. https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01027.html Regards,
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) + +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.) 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) 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) +{ + VhostUserMsg msg; + + if (vhost_user_read(dev, &msg) < 0) { + return 0; + } + + if (msg.request != request) { + error_report("Received unexpected msg type." + "Expected %d received %d", + request, msg.request); + 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); + 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; }