| Message ID | 1469613157-8942-2-git-send-email-saxenap.ltc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Jul 27, 2016 at 1:52 PM, 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_response" 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 | 41 +++++++++++++++++++++++++++++++++++++++++ > hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > index 777c49c..57df586 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_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for > + details. Why need_response and not "need reply"? btw, I wonder if it would be worth to introduce an enum at this point > * 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,39 @@ 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 responses for certain responses/replies > +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 newly > +introduced "need_response" [Bit 3] flag to any command. This indicates that need reply, you can remove the "newly introduced" (it's not going to be so new after a while) > +the client MUST respond with a Payload VhostUserMsg indicating success or I would put right here for clarity: ...MUST respond with a Payload VhostUserMsg (unless the message has already an explicit reply body)... alternatively, I would forbid using the bit 3 on commands that have already an explicit reply. > +failure. The payload should be set to zero on success or non-zero on failure. > +In other words, response must be in the following format : > + > +------------------------------------ > +| request | flags | size | payload | > +------------------------------------ > + > + * Request: 32-bit type of the request > + * Flags: 32-bit bit field: > + * Size: size of the payload ( see below) > + * Payload : a u64 integer, where a non-zero value indicates a failure. > + > +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. > + > +Note that as per the original vhost-user protocol, the following four messages > +anyway require distinct responses from the vhost-user client process: I don't think we need to repeat this list (already redundant with the list in "Communication" part, and with the message specification, 2 times is enough imho) > + * VHOST_GET_FEATURES > + * VHOST_GET_PROTOCOL_FEATURES > + * VHOST_GET_VRING_BASE > + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) > + > +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or > +need_response bit being set brings no behaviourial change. reply > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 495e09f..0cdb918 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_RESPONSE_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_RESPONSE_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 >
Hi Marc, Thanks, please find my reply inline. On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote: >Hi > >On Wed, Jul 27, 2016 at 1:52 PM, 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_response" 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 | 41 +++++++++++++++++++++++++++++++++++++++++ >> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 73 insertions(+) >> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt >> index 777c49c..57df586 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_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for >> + details. > >Why need_response and not "need reply"? (I’d already pointed this out earlier, but looks like I was possibly not very clear.) Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”. So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing when I reviewed the documentation with this different term. So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion. > >btw, I wonder if it would be worth to introduce an enum at this point > >> * 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,39 @@ 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 responses for certain > >responses/replies If you feel strongly about it, will change it here. > >> +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 newly >> +introduced "need_response" [Bit 3] flag to any command. This indicates that > >need reply, you can remove the "newly introduced" (it's not going to >be so new after a while) * need_reply = no I don’t agree, for reasons cited earlier. * remove the “newly introduced” phrase = agree, will do. > >> +the client MUST respond with a Payload VhostUserMsg indicating success or > >I would put right here for clarity: > >...MUST respond with a Payload VhostUserMsg (unless the message has >already an explicit reply body)... > >alternatively, I would forbid using the bit 3 on commands that have >already an explicit reply. I don’t currently have any code that raises an error for such cases. The implementation silently ignores it. > >> +failure. The payload should be set to zero on success or non-zero on failure. >> +In other words, response must be in the following format : >> + >> +------------------------------------ >> +| request | flags | size | payload | >> +------------------------------------ >> + >> + * Request: 32-bit type of the request >> + * Flags: 32-bit bit field: >> + * Size: size of the payload ( see below) >> + * Payload : a u64 integer, where a non-zero value indicates a failure. >> + >> +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. >> + >> +Note that as per the original vhost-user protocol, the following four messages >> +anyway require distinct responses from the vhost-user client process: > >I don't think we need to repeat this list (already redundant with the >list in "Communication" part, and with the message specification, 2 >times is enough imho) Ok, will remove it for brevity. > >> + * VHOST_GET_FEATURES >> + * VHOST_GET_PROTOCOL_FEATURES >> + * VHOST_GET_VRING_BASE >> + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) >> + >> +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or >> +need_response bit being set brings no behaviourial change. > >Reply Again, I disagree, for reasons cited above. [..snip..] removing the rest. > > > >-- >Marc-André Lureau Thanks once again for the quick review. Let me know if this makes sense, so I’ll quickly spin a cleaned-up patch which includes the tiny documentation changes. Regards, Prerna
On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote: > Hi Marc, > Thanks, please find my reply inline. > > > > > > On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote: > > >Hi > > > >On Wed, Jul 27, 2016 at 1:52 PM, 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_response" 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 | 41 +++++++++++++++++++++++++++++++++++++++++ > >> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++ > >> 2 files changed, 73 insertions(+) > >> > >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > >> index 777c49c..57df586 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_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for > >> + details. > > > >Why need_response and not "need reply"? > > (I’d already pointed this out earlier, but looks like I was possibly not very clear.) > Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”. > So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing when I reviewed the documentation with this different term. > So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion. I don't see confusion, I think I agree with Marc André. > > > >btw, I wonder if it would be worth to introduce an enum at this point > > > >> * 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,39 @@ 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 responses for certain > > > >responses/replies > > If you feel strongly about it, will change it here. > > > > >> +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 newly > >> +introduced "need_response" [Bit 3] flag to any command. This indicates that > > > >need reply, you can remove the "newly introduced" (it's not going to > >be so new after a while) > > * need_reply = no I don’t agree, for reasons cited earlier. > * remove the “newly introduced” phrase = agree, will do. > > > > >> +the client MUST respond with a Payload VhostUserMsg indicating success or > > > >I would put right here for clarity: > > > >...MUST respond with a Payload VhostUserMsg (unless the message has > >already an explicit reply body)... > > > >alternatively, I would forbid using the bit 3 on commands that have > >already an explicit reply. > > I don’t currently have any code that raises an error for such cases. > The implementation silently ignores it. > > > > >> +failure. The payload should be set to zero on success or non-zero on failure. > >> +In other words, response must be in the following format : > >> + > >> +------------------------------------ > >> +| request | flags | size | payload | > >> +------------------------------------ > >> + > >> + * Request: 32-bit type of the request > >> + * Flags: 32-bit bit field: > >> + * Size: size of the payload ( see below) > >> + * Payload : a u64 integer, where a non-zero value indicates a failure. > >> + > >> +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. > >> + > >> +Note that as per the original vhost-user protocol, the following four messages > >> +anyway require distinct responses from the vhost-user client process: > > > >I don't think we need to repeat this list (already redundant with the > >list in "Communication" part, and with the message specification, 2 > >times is enough imho) > > Ok, will remove it for brevity. > > > > >> + * VHOST_GET_FEATURES > >> + * VHOST_GET_PROTOCOL_FEATURES > >> + * VHOST_GET_VRING_BASE > >> + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) > >> + > >> +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or > >> +need_response bit being set brings no behaviourial change. > > > >Reply > > Again, I disagree, for reasons cited above. > > [..snip..] removing the rest. > > > > > > > >-- > >Marc-André Lureau > > Thanks once again for the quick review. > Let me know if this makes sense, so I’ll quickly spin a cleaned-up patch which includes the tiny documentation changes. > > Regards, > Prerna
On 27/07/16 6:58 pm, "Michael S. Tsirkin" <mst@redhat.com> wrote: >On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote: >> Hi Marc, >> Thanks, please find my reply inline. >> >> >> >> >> >> On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote: >> >> >Hi >> > >> >On Wed, Jul 27, 2016 at 1:52 PM, 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_response" 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 | 41 +++++++++++++++++++++++++++++++++++++++++ >> >> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++ >> >> 2 files changed, 73 insertions(+) >> >> >> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt >> >> index 777c49c..57df586 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_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for >> >> + details. >> > >> >Why need_response and not "need reply"? >> >> (I’d already pointed this out earlier, but looks like I was possibly not very clear.) >> Before deciding on the right name for Bit 3, let us see the nomenclature for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from the slave”. >> So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it confusing when I reviewed the documentation with this different term. >> So I chose the name need_response with much deliberation — it conveys the essence of what this flag means to achieve, but without adding to confusion. > >I don't see confusion, I think I agree with Marc André. Allright. Posted a new series with the reworded terminology and updated (more concise) documentation. Regards, Prerna
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt index 777c49c..57df586 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_response 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,39 @@ 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 responses 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 newly +introduced "need_response" [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. +In other words, response must be in the following format : + +------------------------------------ +| request | flags | size | payload | +------------------------------------ + + * Request: 32-bit type of the request + * Flags: 32-bit bit field: + * Size: size of the payload ( see below) + * Payload : a u64 integer, where a non-zero value indicates a failure. + +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. + +Note that as per the original vhost-user protocol, the following four messages +anyway require distinct responses from the vhost-user client process: + * VHOST_GET_FEATURES + * VHOST_GET_PROTOCOL_FEATURES + * VHOST_GET_VRING_BASE + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) + +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or +need_response bit being set brings no behaviourial change. diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 495e09f..0cdb918 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_RESPONSE_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_RESPONSE_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; }