* [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications
@ 2019-09-11 13:45 Johannes Berg
2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Johannes Berg @ 2019-09-11 13:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S . Tsirkin
Here's a respin of both the docs and the code, hopefully addressing
most of the requests from review. Let me know if I've missed anything
or not done it in the way you thought.
Thanks,
johannes
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 13:45 [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications Johannes Berg @ 2019-09-11 13:45 ` Johannes Berg 2019-09-11 14:07 ` Michael S. Tsirkin 2019-09-11 19:15 ` Dr. David Alan Gilbert 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 2/2] libvhost-user: implement in-band notifications Johannes Berg 2019-09-11 15:01 ` [Qemu-devel] [RFC v2 0/2] vhost-user: " no-reply 2 siblings, 2 replies; 13+ messages in thread From: Johannes Berg @ 2019-09-11 13:45 UTC (permalink / raw) To: qemu-devel; +Cc: Johannes Berg, Michael S . Tsirkin From: Johannes Berg <johannes.berg@intel.com> For good reason, vhost-user is currently built asynchronously, that way better performance can be obtained. However, for certain use cases such as simulation, this is problematic. Consider an event-based simulation in which both the device and CPU have scheduled according to a simulation "calendar". Now, consider the CPU sending I/O to the device, over a vring in the vhost-user protocol. In this case, the CPU must wait for the vring interrupt to have been processed by the device, so that the device is able to put an entry onto the simulation calendar to obtain time to handle the interrupt. Note that this doesn't mean the I/O is actually done at this time, it just means that the handling of it is scheduled before the CPU can continue running. This cannot be done with the asynchronous eventfd based vring kick and call design. Extend the protocol slightly, so that a message can be used for kick and call instead, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is negotiated. This in itself doesn't guarantee synchronisation, but both sides can also negotiate VHOST_USER_PROTOCOL_F_REPLY_ACK and thus get a reply to this message by setting the need_reply flag, and ensure synchronisation this way. To really use it in both directions, VHOST_USER_PROTOCOL_F_SLAVE_REQ is also needed. Since it is used for simulation purposes and too many messages on the socket can lock up the virtual machine, document that this should only be used together with the mentioned features. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- docs/interop/vhost-user.rst | 113 ++++++++++++++++++++++++++++++------ 1 file changed, 96 insertions(+), 17 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 7827b710aa0a..c4396eabf9e9 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -2,6 +2,7 @@ Vhost-user Protocol =================== :Copyright: 2014 Virtual Open Systems Sarl. +:Copyright: 2019 Intel Corporation :Licence: This work is licensed under the terms of the GNU GPL, version 2 or later. See the COPYING file in the top-level directory. @@ -279,6 +280,9 @@ If *master* is unable to send the full message or receives a wrong reply it will close the connection. An optional reconnection mechanism can be implemented. +If *slave* detects some error such as incompatible features, it may also +close the connection. This should only happen in exceptional circumstances. + Any protocol extensions are gated by protocol feature bits, which allows full backwards compatibility on both master and slave. As older slaves don't support negotiating protocol features, a feature @@ -315,7 +319,8 @@ it until ring is started, or after it has been stopped. Client must start ring upon receiving a kick (that is, detecting that file descriptor is readable) on the descriptor specified by -``VHOST_USER_SET_VRING_KICK``, and stop ring upon receiving +``VHOST_USER_SET_VRING_KICK`` (or receiving the in-band message +``VHOST_USER_VRING_KICK`` if negotiated) and stop ring upon receiving ``VHOST_USER_GET_VRING_BASE``. While processing the rings (whether they are enabled or not), client @@ -767,24 +772,48 @@ When reconnecting: #. Resubmit inflight ``DescStatePacked`` entries in order of their counter value +In-band notifications +--------------------- + +In some limited situations (e.g. for simulation) it is desirable to +have the kick, call and error (if used) signals done via in-band +messages instead of asynchronous eventfd notifications. This can be +done by negotiating the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` +protocol feature. + +Note that due to the fact that too many messages on the sockets can +cause the sending application(s) to block, it is not advised to use +this feature unless absolutely necessary. It is also considered an +error to negotiate this feature without also negotiating +``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, +the former is necessary for getting a message channel from the slave +to the master, while the latter needs to be used with the in-band +notification messages to block until they are processed, both to avoid +blocking later and for proper processing (at least in the simulation +use case.) As it has no other way of signalling this error, the slave +should close the connection as a response to a +``VHOST_USER_SET_PROTOCOL_FEATURES`` message that sets the in-band +notifications feature flag without the other two. + Protocol features ----------------- .. code:: c - #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 - #define VHOST_USER_PROTOCOL_F_MTU 4 - #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 - #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 - #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 - #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 - #define VHOST_USER_PROTOCOL_F_CONFIG 9 - #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 - #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 - #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 + #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 + #define VHOST_USER_PROTOCOL_F_MTU 4 + #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 + #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 + #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 + #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 + #define VHOST_USER_PROTOCOL_F_CONFIG 9 + #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 + #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 + #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 + #define VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS 13 Master message types -------------------- @@ -946,7 +975,9 @@ Master message types Bits (0-7) of the payload contain the vring index. Bit 8 is the invalid FD flag. This flag is set when there is no file descriptor in the ancillary data. This signals that polling should be used - instead of waiting for a kick. + instead of waiting for the call. however, if the protocol feature + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated + it instead means the updates should be done using the messages. ``VHOST_USER_SET_VRING_CALL`` :id: 13 @@ -959,7 +990,9 @@ Master message types Bits (0-7) of the payload contain the vring index. Bit 8 is the invalid FD flag. This flag is set when there is no file descriptor in the ancillary data. This signals that polling will be used - instead of waiting for the call. + instead of waiting for the call; however, if the protocol feature + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated + it instead means the updates should be done using the messages. ``VHOST_USER_SET_VRING_ERR`` :id: 14 @@ -971,7 +1004,11 @@ Master message types Bits (0-7) of the payload contain the vring index. Bit 8 is the invalid FD flag. This flag is set when there is no file descriptor - in the ancillary data. + in the ancillary data. If the protocol features + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` and + ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated the invalid + FD flag will be used to indicate that updates should be done using + the ``VHOST_USER_SLAVE_ message. ``VHOST_USER_GET_QUEUE_NUM`` :id: 17 @@ -1190,6 +1227,20 @@ Master message types ancillary data. The GPU protocol is used to inform the master of rendering state and updates. See vhost-user-gpu.rst for details. +``VHOST_USER_VRING_KICK`` + :id: 34 + :equivalent ioctl: N/A + :slave payload: vring state description + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol + feature has been successfully negotiated, this message may be + submitted by the master to indicate that a buffer was added to + the vring instead of signalling it using the vring's event FD or + having the slave rely on polling. + + The state.num field is currently reserved and must be set to 0. + Slave message types ------------------- @@ -1246,6 +1297,34 @@ Slave message types ``VHOST_USER_PROTOCOL_F_HOST_NOTIFIER`` protocol feature has been successfully negotiated. +``VHOST_USER_SLAVE_VRING_CALL`` + :id: 4 + :equivalent ioctl: N/A + :slave payload: vring state description + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol + feature has been successfully negotiated, this message may be + submitted by the slave to indicate that a buffer was used from + the vring instead of signalling this using the vring's kick FD or + having the master relying on polling. + + The state.num field is currently reserved and must be set to 0. + +``VHOST_USER_SLAVE_VRING_ERR`` + :id: 5 + :equivalent ioctl: N/A + :slave payload: vring state description + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol + feature has been successfully negotiated, this message may be + submitted by the slave to indicate that an error occurred on the + specific vring, instead of signalling the error FD set by the + master via ``VHOST_USER_SET_VRING_ERR``. + + The state.num field is currently reserved and must be set to 0. + .. _reply_ack: VHOST_USER_PROTOCOL_F_REPLY_ACK -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg @ 2019-09-11 14:07 ` Michael S. Tsirkin 2019-09-11 15:09 ` Johannes Berg 2019-09-11 19:15 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2019-09-11 14:07 UTC (permalink / raw) To: Johannes Berg; +Cc: qemu-devel, Johannes Berg On Wed, Sep 11, 2019 at 03:45:38PM +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > For good reason, vhost-user is currently built asynchronously, that > way better performance can be obtained. However, for certain use > cases such as simulation, this is problematic. > > Consider an event-based simulation in which both the device and CPU > have scheduled according to a simulation "calendar". Now, consider > the CPU sending I/O to the device, over a vring in the vhost-user > protocol. In this case, the CPU must wait for the vring interrupt > to have been processed by the device, so that the device is able to > put an entry onto the simulation calendar to obtain time to handle > the interrupt. Note that this doesn't mean the I/O is actually done > at this time, it just means that the handling of it is scheduled > before the CPU can continue running. > > This cannot be done with the asynchronous eventfd based vring kick > and call design. > > Extend the protocol slightly, so that a message can be used for kick > and call instead, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is > negotiated. This in itself doesn't guarantee synchronisation, but both > sides can also negotiate VHOST_USER_PROTOCOL_F_REPLY_ACK and thus get > a reply to this message by setting the need_reply flag, and ensure > synchronisation this way. > > To really use it in both directions, VHOST_USER_PROTOCOL_F_SLAVE_REQ > is also needed. > > Since it is used for simulation purposes and too many messages on > the socket can lock up the virtual machine, document that this should > only be used together with the mentioned features. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > docs/interop/vhost-user.rst | 113 ++++++++++++++++++++++++++++++------ > 1 file changed, 96 insertions(+), 17 deletions(-) > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst > index 7827b710aa0a..c4396eabf9e9 100644 > --- a/docs/interop/vhost-user.rst > +++ b/docs/interop/vhost-user.rst > @@ -2,6 +2,7 @@ > Vhost-user Protocol > =================== > :Copyright: 2014 Virtual Open Systems Sarl. > +:Copyright: 2019 Intel Corporation > :Licence: This work is licensed under the terms of the GNU GPL, > version 2 or later. See the COPYING file in the top-level > directory. > @@ -279,6 +280,9 @@ If *master* is unable to send the full message or receives a wrong > reply it will close the connection. An optional reconnection mechanism > can be implemented. > > +If *slave* detects some error such as incompatible features, it may also > +close the connection. This should only happen in exceptional circumstances. > + > Any protocol extensions are gated by protocol feature bits, which > allows full backwards compatibility on both master and slave. As > older slaves don't support negotiating protocol features, a feature > @@ -315,7 +319,8 @@ it until ring is started, or after it has been stopped. > > Client must start ring upon receiving a kick (that is, detecting that > file descriptor is readable) on the descriptor specified by > -``VHOST_USER_SET_VRING_KICK``, and stop ring upon receiving > +``VHOST_USER_SET_VRING_KICK`` (or receiving the in-band message > +``VHOST_USER_VRING_KICK`` if negotiated) and stop ring upon receiving > ``VHOST_USER_GET_VRING_BASE``. > > While processing the rings (whether they are enabled or not), client > @@ -767,24 +772,48 @@ When reconnecting: > #. Resubmit inflight ``DescStatePacked`` entries in order of their > counter value > > +In-band notifications > +--------------------- > + > +In some limited situations (e.g. for simulation) it is desirable to > +have the kick, call and error (if used) signals done via in-band > +messages instead of asynchronous eventfd notifications. This can be > +done by negotiating the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` > +protocol feature. > + > +Note that due to the fact that too many messages on the sockets can > +cause the sending application(s) to block, it is not advised to use > +this feature unless absolutely necessary. It is also considered an > +error to negotiate this feature without also negotiating > +``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, > +the former is necessary for getting a message channel from the slave > +to the master, while the latter needs to be used with the in-band > +notification messages to block until they are processed, both to avoid > +blocking later and for proper processing (at least in the simulation > +use case.) As it has no other way of signalling this error, the slave > +should close the connection as a response to a > +``VHOST_USER_SET_PROTOCOL_FEATURES`` message that sets the in-band > +notifications feature flag without the other two. > + > Protocol features > ----------------- > > .. code:: c > > - #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 > - #define VHOST_USER_PROTOCOL_F_MTU 4 > - #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 > - #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 > - #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 > - #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 > - #define VHOST_USER_PROTOCOL_F_CONFIG 9 > - #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 > - #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 > - #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 > + #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 > + #define VHOST_USER_PROTOCOL_F_MTU 4 > + #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 > + #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 > + #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 > + #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 > + #define VHOST_USER_PROTOCOL_F_CONFIG 9 > + #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 > + #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 > + #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 > + #define VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS 13 INFLIGHT so INBAND? > > Master message types > -------------------- > @@ -946,7 +975,9 @@ Master message types > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > in the ancillary data. This signals that polling should be used > - instead of waiting for a kick. > + instead of waiting for the call. however, if the protocol feature > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated > + it instead means the updates should be done using the messages. > > ``VHOST_USER_SET_VRING_CALL`` > :id: 13 > @@ -959,7 +990,9 @@ Master message types > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > in the ancillary data. This signals that polling will be used > - instead of waiting for the call. > + instead of waiting for the call; however, if the protocol feature > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated > + it instead means the updates should be done using the messages. Hmm I don't like this. I propose that with VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS we just don't allow VHOST_USER_SET_VRING_CALL (if you think it's important to allow them, we can say that we do not require them). But it's important for performance to be able to enable polling. > > ``VHOST_USER_SET_VRING_ERR`` > :id: 14 > @@ -971,7 +1004,11 @@ Master message types > > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > - in the ancillary data. > + in the ancillary data. If the protocol features > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` and > + ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated the invalid > + FD flag will be used to indicate that updates should be done using > + the ``VHOST_USER_SLAVE_ message. > > ``VHOST_USER_GET_QUEUE_NUM`` > :id: 17 > @@ -1190,6 +1227,20 @@ Master message types > ancillary data. The GPU protocol is used to inform the master of > rendering state and updates. See vhost-user-gpu.rst for details. > > +``VHOST_USER_VRING_KICK`` > + :id: 34 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the master to indicate that a buffer was added to > + the vring instead of signalling it using the vring's event FD or event -> kick? fd -> file descriptor > + having the slave rely on polling. i think polling is a separate option and should be there with inband kicks. > + > + The state.num field is currently reserved and must be set to 0. > + > Slave message types > ------------------- > > @@ -1246,6 +1297,34 @@ Slave message types > ``VHOST_USER_PROTOCOL_F_HOST_NOTIFIER`` protocol feature has been > successfully negotiated. > > +``VHOST_USER_SLAVE_VRING_CALL`` > + :id: 4 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the slave to indicate that a buffer was used from > + the vring instead of signalling this using the vring's kick FD or > + having the master relying on polling. call FD? > + > + The state.num field is currently reserved and must be set to 0. > + > +``VHOST_USER_SLAVE_VRING_ERR`` > + :id: 5 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the slave to indicate that an error occurred on the > + specific vring, instead of signalling the error FD set by the > + master via ``VHOST_USER_SET_VRING_ERR``. > + > + The state.num field is currently reserved and must be set to 0. > + > .. _reply_ack: > > VHOST_USER_PROTOCOL_F_REPLY_ACK > -- > 2.20.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 14:07 ` Michael S. Tsirkin @ 2019-09-11 15:09 ` Johannes Berg 2019-09-16 11:40 ` Johannes Berg 0 siblings, 1 reply; 13+ messages in thread From: Johannes Berg @ 2019-09-11 15:09 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Wed, 2019-09-11 at 10:07 -0400, Michael S. Tsirkin wrote: > > > + #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 > > + #define VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS 13 > > INFLIGHT so INBAND? *shrug*, sure > > + instead of waiting for the call; however, if the protocol feature > > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated > > + it instead means the updates should be done using the messages. > > Hmm I don't like this. I propose that with VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS > we just don't allow VHOST_USER_SET_VRING_CALL (if you think it's > important to allow them, we can say that we do not require them). You can't actually skip SET_VRING_CALL, it's necessary to start a vring, so libvhost-user for example calls dev->iface->queue_set_started() only in this case. The docs in the "Starting and stopping rings" section also explain this. > But it's important for performance to be able to enable polling. I don't think if you enable this you care about performance, after all the whole point of it is to get REPLY_ACK for the in-band message. > > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > > + feature has been successfully negotiated, this message may be > > + submitted by the master to indicate that a buffer was added to > > + the vring instead of signalling it using the vring's event FD or > > event -> kick? > fd -> file descriptor Sure. > > + having the slave rely on polling. > > i think polling is a separate option and should be there with inband > kicks. See above. But I guess we could put a flag into bit 9 indicating that you want to use messages instead of polling or a file descriptor, if you prefer. > > +``VHOST_USER_SLAVE_VRING_CALL`` > > + :id: 4 > > + :equivalent ioctl: N/A > > + :slave payload: vring state description > > + :master payload: N/A > > + > > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > > + feature has been successfully negotiated, this message may be > > + submitted by the slave to indicate that a buffer was used from > > + the vring instead of signalling this using the vring's kick FD or > > + having the master relying on polling. > > call FD? I confused this far too many times and thought I got it right finally, but yes, you're right. johannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 15:09 ` Johannes Berg @ 2019-09-16 11:40 ` Johannes Berg 2019-09-16 15:30 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Johannes Berg @ 2019-09-16 11:40 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel Hi Michael, I had just wanted to prepare a resend, but > > Hmm I don't like this. I propose that with VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS > > we just don't allow VHOST_USER_SET_VRING_CALL (if you think it's > > important to allow them, we can say that we do not require them). > > You can't actually skip SET_VRING_CALL, it's necessary to start a vring, > so libvhost-user for example calls dev->iface->queue_set_started() only > in this case. The docs in the "Starting and stopping rings" section also > explain this. [...] > See above. But I guess we could put a flag into bit 9 indicating that > you want to use messages instead of polling or a file descriptor, if you > prefer. Personally, I don't think it matters since right now I can see the in- band notification as being really necessary/useful only for simulation work, and in that case no polling will be doable. If you do think it's important to not make the two mutually exclusive, how would you prefer to have this handled? With a new flag, e.g. in bit 9, indicating "use inband signalling instead of polling or eventfd"? Thanks, johannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-16 11:40 ` Johannes Berg @ 2019-09-16 15:30 ` Michael S. Tsirkin 2019-09-17 12:01 ` Johannes Berg 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2019-09-16 15:30 UTC (permalink / raw) To: Johannes Berg; +Cc: qemu-devel On Mon, Sep 16, 2019 at 01:40:35PM +0200, Johannes Berg wrote: > Hi Michael, > > I had just wanted to prepare a resend, but > > > > Hmm I don't like this. I propose that with VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS > > > we just don't allow VHOST_USER_SET_VRING_CALL (if you think it's > > > important to allow them, we can say that we do not require them). > > > > You can't actually skip SET_VRING_CALL, it's necessary to start a vring, > > so libvhost-user for example calls dev->iface->queue_set_started() only > > in this case. The docs in the "Starting and stopping rings" section also > > explain this. > > [...] > > > See above. But I guess we could put a flag into bit 9 indicating that > > you want to use messages instead of polling or a file descriptor, if you > > prefer. > > Personally, I don't think it matters since right now I can see the in- > band notification as being really necessary/useful only for simulation > work, and in that case no polling will be doable. > > If you do think it's important to not make the two mutually exclusive, > how would you prefer to have this handled? With a new flag, e.g. in bit > 9, indicating "use inband signalling instead of polling or eventfd"? > > Thanks, > johannes So first we really need to fix up Starting and stopping section, explaining that if the FD is invalid, this means ring is immediately started, right? If we want to keep it simple, my proposal is this, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is set then VHOST_USER_SET_VRING_CALL and VHOST_USER_SET_VRING_KICK are not valid. Starting/stopping ring needs to be updated, teaching it that ring is started after it gets a kick. -- MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-16 15:30 ` Michael S. Tsirkin @ 2019-09-17 12:01 ` Johannes Berg 0 siblings, 0 replies; 13+ messages in thread From: Johannes Berg @ 2019-09-17 12:01 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Mon, 2019-09-16 at 11:30 -0400, Michael S. Tsirkin wrote: > > So first we really need to fix up Starting and stopping section, > explaining that if the FD is invalid, this means ring > is immediately started, right? It actually does say that, and ... I even changed it already to say the ring is also started when receiving the new VHOST_USER_VRING_KICK message: Client must start ring upon receiving a kick (that is, detecting that file descriptor is readable) on the descriptor specified by ``VHOST_USER_SET_VRING_KICK`` (or receiving the in-band message ``VHOST_USER_VRING_KICK`` if negotiated) and stop ring upon receiving ``VHOST_USER_GET_VRING_BASE``. > If we want to keep it simple, my proposal is this, if > VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is set then > VHOST_USER_SET_VRING_CALL and VHOST_USER_SET_VRING_KICK are not valid. > Starting/stopping ring needs to be updated, teaching it > that ring is started after it gets a kick. Makes sense, mostly I just need to actually *implement* that. johannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg 2019-09-11 14:07 ` Michael S. Tsirkin @ 2019-09-11 19:15 ` Dr. David Alan Gilbert 2019-09-11 19:18 ` Johannes Berg 1 sibling, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 19:15 UTC (permalink / raw) To: Johannes Berg; +Cc: qemu-devel, Johannes Berg, Michael S . Tsirkin * Johannes Berg (johannes@sipsolutions.net) wrote: > From: Johannes Berg <johannes.berg@intel.com> > > For good reason, vhost-user is currently built asynchronously, that > way better performance can be obtained. However, for certain use > cases such as simulation, this is problematic. > > Consider an event-based simulation in which both the device and CPU > have scheduled according to a simulation "calendar". Now, consider > the CPU sending I/O to the device, over a vring in the vhost-user > protocol. In this case, the CPU must wait for the vring interrupt > to have been processed by the device, so that the device is able to > put an entry onto the simulation calendar to obtain time to handle > the interrupt. Note that this doesn't mean the I/O is actually done > at this time, it just means that the handling of it is scheduled > before the CPU can continue running. > > This cannot be done with the asynchronous eventfd based vring kick > and call design. > > Extend the protocol slightly, so that a message can be used for kick > and call instead, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is > negotiated. This in itself doesn't guarantee synchronisation, but both > sides can also negotiate VHOST_USER_PROTOCOL_F_REPLY_ACK and thus get > a reply to this message by setting the need_reply flag, and ensure > synchronisation this way. I'm confused; if you've already got REPLY_ACK, why do we need anything else? We already require the reply on set_mem_table as part of postcopy. Dave > To really use it in both directions, VHOST_USER_PROTOCOL_F_SLAVE_REQ > is also needed. > > Since it is used for simulation purposes and too many messages on > the socket can lock up the virtual machine, document that this should > only be used together with the mentioned features. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > docs/interop/vhost-user.rst | 113 ++++++++++++++++++++++++++++++------ > 1 file changed, 96 insertions(+), 17 deletions(-) > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst > index 7827b710aa0a..c4396eabf9e9 100644 > --- a/docs/interop/vhost-user.rst > +++ b/docs/interop/vhost-user.rst > @@ -2,6 +2,7 @@ > Vhost-user Protocol > =================== > :Copyright: 2014 Virtual Open Systems Sarl. > +:Copyright: 2019 Intel Corporation > :Licence: This work is licensed under the terms of the GNU GPL, > version 2 or later. See the COPYING file in the top-level > directory. > @@ -279,6 +280,9 @@ If *master* is unable to send the full message or receives a wrong > reply it will close the connection. An optional reconnection mechanism > can be implemented. > > +If *slave* detects some error such as incompatible features, it may also > +close the connection. This should only happen in exceptional circumstances. > + > Any protocol extensions are gated by protocol feature bits, which > allows full backwards compatibility on both master and slave. As > older slaves don't support negotiating protocol features, a feature > @@ -315,7 +319,8 @@ it until ring is started, or after it has been stopped. > > Client must start ring upon receiving a kick (that is, detecting that > file descriptor is readable) on the descriptor specified by > -``VHOST_USER_SET_VRING_KICK``, and stop ring upon receiving > +``VHOST_USER_SET_VRING_KICK`` (or receiving the in-band message > +``VHOST_USER_VRING_KICK`` if negotiated) and stop ring upon receiving > ``VHOST_USER_GET_VRING_BASE``. > > While processing the rings (whether they are enabled or not), client > @@ -767,24 +772,48 @@ When reconnecting: > #. Resubmit inflight ``DescStatePacked`` entries in order of their > counter value > > +In-band notifications > +--------------------- > + > +In some limited situations (e.g. for simulation) it is desirable to > +have the kick, call and error (if used) signals done via in-band > +messages instead of asynchronous eventfd notifications. This can be > +done by negotiating the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` > +protocol feature. > + > +Note that due to the fact that too many messages on the sockets can > +cause the sending application(s) to block, it is not advised to use > +this feature unless absolutely necessary. It is also considered an > +error to negotiate this feature without also negotiating > +``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, > +the former is necessary for getting a message channel from the slave > +to the master, while the latter needs to be used with the in-band > +notification messages to block until they are processed, both to avoid > +blocking later and for proper processing (at least in the simulation > +use case.) As it has no other way of signalling this error, the slave > +should close the connection as a response to a > +``VHOST_USER_SET_PROTOCOL_FEATURES`` message that sets the in-band > +notifications feature flag without the other two. > + > Protocol features > ----------------- > > .. code:: c > > - #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 > - #define VHOST_USER_PROTOCOL_F_MTU 4 > - #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 > - #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 > - #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 > - #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 > - #define VHOST_USER_PROTOCOL_F_CONFIG 9 > - #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 > - #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 > - #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 > + #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 > + #define VHOST_USER_PROTOCOL_F_MTU 4 > + #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 > + #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 > + #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 > + #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 > + #define VHOST_USER_PROTOCOL_F_CONFIG 9 > + #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10 > + #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11 > + #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 > + #define VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS 13 > > Master message types > -------------------- > @@ -946,7 +975,9 @@ Master message types > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > in the ancillary data. This signals that polling should be used > - instead of waiting for a kick. > + instead of waiting for the call. however, if the protocol feature > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated > + it instead means the updates should be done using the messages. > > ``VHOST_USER_SET_VRING_CALL`` > :id: 13 > @@ -959,7 +990,9 @@ Master message types > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > in the ancillary data. This signals that polling will be used > - instead of waiting for the call. > + instead of waiting for the call; however, if the protocol feature > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` has been negotiated > + it instead means the updates should be done using the messages. > > ``VHOST_USER_SET_VRING_ERR`` > :id: 14 > @@ -971,7 +1004,11 @@ Master message types > > Bits (0-7) of the payload contain the vring index. Bit 8 is the > invalid FD flag. This flag is set when there is no file descriptor > - in the ancillary data. > + in the ancillary data. If the protocol features > + ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` and > + ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated the invalid > + FD flag will be used to indicate that updates should be done using > + the ``VHOST_USER_SLAVE_ message. > > ``VHOST_USER_GET_QUEUE_NUM`` > :id: 17 > @@ -1190,6 +1227,20 @@ Master message types > ancillary data. The GPU protocol is used to inform the master of > rendering state and updates. See vhost-user-gpu.rst for details. > > +``VHOST_USER_VRING_KICK`` > + :id: 34 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the master to indicate that a buffer was added to > + the vring instead of signalling it using the vring's event FD or > + having the slave rely on polling. > + > + The state.num field is currently reserved and must be set to 0. > + > Slave message types > ------------------- > > @@ -1246,6 +1297,34 @@ Slave message types > ``VHOST_USER_PROTOCOL_F_HOST_NOTIFIER`` protocol feature has been > successfully negotiated. > > +``VHOST_USER_SLAVE_VRING_CALL`` > + :id: 4 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the slave to indicate that a buffer was used from > + the vring instead of signalling this using the vring's kick FD or > + having the master relying on polling. > + > + The state.num field is currently reserved and must be set to 0. > + > +``VHOST_USER_SLAVE_VRING_ERR`` > + :id: 5 > + :equivalent ioctl: N/A > + :slave payload: vring state description > + :master payload: N/A > + > + When the ``VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS`` protocol > + feature has been successfully negotiated, this message may be > + submitted by the slave to indicate that an error occurred on the > + specific vring, instead of signalling the error FD set by the > + master via ``VHOST_USER_SET_VRING_ERR``. > + > + The state.num field is currently reserved and must be set to 0. > + > .. _reply_ack: > > VHOST_USER_PROTOCOL_F_REPLY_ACK > -- > 2.20.1 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 19:15 ` Dr. David Alan Gilbert @ 2019-09-11 19:18 ` Johannes Berg 2019-09-12 8:09 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 13+ messages in thread From: Johannes Berg @ 2019-09-11 19:18 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: qemu-devel, Michael S . Tsirkin On Wed, 2019-09-11 at 20:15 +0100, Dr. David Alan Gilbert wrote: > > Extend the protocol slightly, so that a message can be used for kick > > and call instead, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is > > negotiated. This in itself doesn't guarantee synchronisation, but both > > sides can also negotiate VHOST_USER_PROTOCOL_F_REPLY_ACK and thus get > > a reply to this message by setting the need_reply flag, and ensure > > synchronisation this way. > > I'm confused; if you've already got REPLY_ACK, why do we need anything > else? We already require the reply on set_mem_table as part of > postcopy. Hmm? How's this related to set_mem_table? For simulation purposes, I need the kick and call (and error perhaps though it's not really used by anyone now it seems) to be synchronous messages instead of asynchronous event FD pushes. But I think enough words have been expended on explaining it already, if I may kindly ask you to read the discussions with Stefan and Michael here: https://lore.kernel.org/qemu-devel/20190902121233.13382-1-johannes@sipsolutions.net/ Thanks, johannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-11 19:18 ` Johannes Berg @ 2019-09-12 8:09 ` Dr. David Alan Gilbert 2019-09-12 8:13 ` Johannes Berg 0 siblings, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-12 8:09 UTC (permalink / raw) To: Johannes Berg; +Cc: qemu-devel, Michael S . Tsirkin * Johannes Berg (johannes@sipsolutions.net) wrote: > On Wed, 2019-09-11 at 20:15 +0100, Dr. David Alan Gilbert wrote: > > > > Extend the protocol slightly, so that a message can be used for kick > > > and call instead, if VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS is > > > negotiated. This in itself doesn't guarantee synchronisation, but both > > > sides can also negotiate VHOST_USER_PROTOCOL_F_REPLY_ACK and thus get > > > a reply to this message by setting the need_reply flag, and ensure > > > synchronisation this way. > > > > I'm confused; if you've already got REPLY_ACK, why do we need anything > > else? We already require the reply on set_mem_table as part of > > postcopy. > > Hmm? How's this related to set_mem_table? > > For simulation purposes, I need the kick and call (and error perhaps > though it's not really used by anyone now it seems) to be synchronous > messages instead of asynchronous event FD pushes. > > But I think enough words have been expended on explaining it already, if > I may kindly ask you to read the discussions with Stefan and Michael > here: > > https://lore.kernel.org/qemu-devel/20190902121233.13382-1-johannes@sipsolutions.net/ Ah OK. You're actually using the same trick of using REPLY_ACK/need_reply to make it synchronous that set_mem_table does; that makes sense - but then new calls are getting it to actually process some data/commands on the ring itself? Dave > Thanks, > johannes > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages 2019-09-12 8:09 ` Dr. David Alan Gilbert @ 2019-09-12 8:13 ` Johannes Berg 0 siblings, 0 replies; 13+ messages in thread From: Johannes Berg @ 2019-09-12 8:13 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: qemu-devel, Michael S . Tsirkin On Thu, 2019-09-12 at 09:09 +0100, Dr. David Alan Gilbert wrote: > > You're actually using the same trick of using > REPLY_ACK/need_reply to make it synchronous that set_mem_table does; I don't think it's really the same - though arguably I could have spec'ed the inband signal to *require* an ACK. The way I did it relies on the REPLY_ACK extension. SET_MEM_TABLE actually specifies a 3-way handshake, qemu->device, device->qemu, qemu->device. > that makes sense - but then new calls are getting it to actually process > some data/commands on the ring itself? No, the calls (or more specifically the REPLY_ACK to them) are really in simulation to only acknowledge the interrupt (kick/call) signal has been received and accounted for on the simulation calendar, the actual processing happens when the calendar event is scheduled. johannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC v2 2/2] libvhost-user: implement in-band notifications 2019-09-11 13:45 [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications Johannes Berg 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg @ 2019-09-11 13:45 ` Johannes Berg 2019-09-11 15:01 ` [Qemu-devel] [RFC v2 0/2] vhost-user: " no-reply 2 siblings, 0 replies; 13+ messages in thread From: Johannes Berg @ 2019-09-11 13:45 UTC (permalink / raw) To: qemu-devel; +Cc: Johannes Berg, Michael S . Tsirkin From: Johannes Berg <johannes.berg@intel.com> Add support for VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS, but as it's not desired by default, don't enable it unless the device implementation opts in by returning it from its protocol features callback. Note that I updated vu_set_vring_err_exec(), but didn't add any sending of the VHOST_USER_SLAVE_VRING_ERR message as there's no write to the err_fd today either. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- contrib/libvhost-user/libvhost-user.c | 94 ++++++++++++++++++++++++--- contrib/libvhost-user/libvhost-user.h | 4 ++ 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index f1677da21201..6367ddeb17fb 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -136,6 +136,7 @@ vu_request_to_string(unsigned int req) REQ(VHOST_USER_GET_INFLIGHT_FD), REQ(VHOST_USER_SET_INFLIGHT_FD), REQ(VHOST_USER_GPU_SET_SOCKET), + REQ(VHOST_USER_VRING_KICK), REQ(VHOST_USER_MAX), }; #undef REQ @@ -163,7 +164,10 @@ vu_panic(VuDev *dev, const char *msg, ...) dev->panic(dev, buf); free(buf); - /* FIXME: find a way to call virtio_error? */ + /* + * FIXME: + * find a way to call virtio_error, or perhaps close the connection? + */ } /* Translate guest physical address to our virtual address. */ @@ -920,6 +924,7 @@ static bool vu_check_queue_msg_file(VuDev *dev, VhostUserMsg *vmsg) { int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK; + bool nofd = vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK; if (index >= dev->max_queues) { vmsg_close_fds(vmsg); @@ -927,8 +932,12 @@ vu_check_queue_msg_file(VuDev *dev, VhostUserMsg *vmsg) return false; } - if (vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK || - vmsg->fd_num != 1) { + if (nofd) { + vmsg_close_fds(vmsg); + return true; + } + + if (vmsg->fd_num != 1) { vmsg_close_fds(vmsg); vu_panic(dev, "Invalid fds in request: %d", vmsg->request); return false; @@ -1025,6 +1034,7 @@ static bool vu_set_vring_kick_exec(VuDev *dev, VhostUserMsg *vmsg) { int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK; + bool nofd = vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK; DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64); @@ -1038,8 +1048,8 @@ vu_set_vring_kick_exec(VuDev *dev, VhostUserMsg *vmsg) dev->vq[index].kick_fd = -1; } - dev->vq[index].kick_fd = vmsg->fds[0]; - DPRINT("Got kick_fd: %d for vq: %d\n", vmsg->fds[0], index); + dev->vq[index].kick_fd = nofd ? -1 : vmsg->fds[0]; + DPRINT("Got kick_fd: %d for vq: %d\n", dev->vq[index].kick_fd, index); dev->vq[index].started = true; if (dev->iface->queue_set_started) { @@ -1116,6 +1126,7 @@ static bool vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg) { int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK; + bool nofd = vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK; DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64); @@ -1128,14 +1139,14 @@ vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg) dev->vq[index].call_fd = -1; } - dev->vq[index].call_fd = vmsg->fds[0]; + dev->vq[index].call_fd = nofd ? -1 : vmsg->fds[0]; /* in case of I/O hang after reconnecting */ - if (eventfd_write(vmsg->fds[0], 1)) { + if (dev->vq[index].call_fd != -1 && eventfd_write(vmsg->fds[0], 1)) { return -1; } - DPRINT("Got call_fd: %d for vq: %d\n", vmsg->fds[0], index); + DPRINT("Got call_fd: %d for vq: %d\n", dev->vq[index].call_fd, index); return false; } @@ -1144,6 +1155,7 @@ static bool vu_set_vring_err_exec(VuDev *dev, VhostUserMsg *vmsg) { int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK; + bool nofd = vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK; DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64); @@ -1156,7 +1168,7 @@ vu_set_vring_err_exec(VuDev *dev, VhostUserMsg *vmsg) dev->vq[index].err_fd = -1; } - dev->vq[index].err_fd = vmsg->fds[0]; + dev->vq[index].err_fd = nofd ? -1 : vmsg->fds[0]; return false; } @@ -1164,6 +1176,14 @@ vu_set_vring_err_exec(VuDev *dev, VhostUserMsg *vmsg) static bool vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) { + /* + * Note that we support, but intentionally do not set, + * VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS. This means that + * a device implementation can return it in its callback + * (get_protocol_features) if it wants to use this for + * simulation, but it is otherwise not desirable (if even + * implemented by the master.) + */ uint64_t features = 1ULL << VHOST_USER_PROTOCOL_F_MQ | 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | @@ -1196,6 +1216,25 @@ vu_set_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) dev->protocol_features = vmsg->payload.u64; + if (vu_has_protocol_feature(dev, + VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS) && + (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ) || + !vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { + /* + * The use case for using messages for kick/call is simulation, to make + * the kick and call synchronous. To actually get that behaviour, both + * of the other features are required. + * Theoretically, one could use only kick messages, or do them without + * having F_REPLY_ACK, but too many (possibly pending) messages on the + * socket will eventually cause the master to hang, to avoid this in + * scenarios where not desired enforce that the settings are in a way + * that actually enables the simulation case. + */ + vu_panic(dev, + "F_IN_BAND_NOTIFICATIONS requires F_SLAVE_REQ && F_REPLY_ACK"); + return false; + } + if (dev->iface->set_protocol_features) { dev->iface->set_protocol_features(dev, features); } @@ -1456,6 +1495,25 @@ vu_set_inflight_fd(VuDev *dev, VhostUserMsg *vmsg) return false; } +static bool +vu_handle_vring_kick(VuDev *dev, VhostUserMsg *vmsg) +{ + unsigned int index = vmsg->payload.state.index; + + if (index >= dev->max_queues) { + vu_panic(dev, "Invalid queue index: %u", index); + return false; + } + + DPRINT("Got kick message: handler:%p idx:%d\n", + dev->vq[index].handler, index); + if (dev->vq[index].handler) { + dev->vq[index].handler(dev, index); + } + + return false; +} + static bool vu_process_message(VuDev *dev, VhostUserMsg *vmsg) { @@ -1538,6 +1596,8 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg) return vu_get_inflight_fd(dev, vmsg); case VHOST_USER_SET_INFLIGHT_FD: return vu_set_inflight_fd(dev, vmsg); + case VHOST_USER_VRING_KICK: + return vu_handle_vring_kick(dev, vmsg); default: vmsg_close_fds(vmsg); vu_panic(dev, "Unhandled request: %d", vmsg->request); @@ -2010,6 +2070,22 @@ vu_queue_notify(VuDev *dev, VuVirtq *vq) return; } + if (vu_has_protocol_feature(dev, + VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS) && + vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ)) { + VhostUserMsg vmsg = { + .request = VHOST_USER_SLAVE_VRING_CALL, + .flags = VHOST_USER_VERSION, + .size = sizeof(vmsg.payload.state), + .payload.state = { + .index = vq - dev->vq, + }, + }; + + vu_message_write(dev, dev->slave_fd, &vmsg); + return; + } + if (eventfd_write(vq->call_fd, 1) < 0) { vu_panic(dev, "Error writing eventfd: %s", strerror(errno)); } diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h index 46b600799b2e..a22dc4733849 100644 --- a/contrib/libvhost-user/libvhost-user.h +++ b/contrib/libvhost-user/libvhost-user.h @@ -53,6 +53,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD = 10, VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, + VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS = 13, VHOST_USER_PROTOCOL_F_MAX }; @@ -94,6 +95,7 @@ typedef enum VhostUserRequest { VHOST_USER_GET_INFLIGHT_FD = 31, VHOST_USER_SET_INFLIGHT_FD = 32, VHOST_USER_GPU_SET_SOCKET = 33, + VHOST_USER_VRING_KICK = 34, VHOST_USER_MAX } VhostUserRequest; @@ -102,6 +104,8 @@ typedef enum VhostUserSlaveRequest { VHOST_USER_SLAVE_IOTLB_MSG = 1, VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2, VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3, + VHOST_USER_SLAVE_VRING_CALL = 4, + VHOST_USER_SLAVE_VRING_ERR = 5, VHOST_USER_SLAVE_MAX } VhostUserSlaveRequest; -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications 2019-09-11 13:45 [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications Johannes Berg 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 2/2] libvhost-user: implement in-band notifications Johannes Berg @ 2019-09-11 15:01 ` no-reply 2 siblings, 0 replies; 13+ messages in thread From: no-reply @ 2019-09-11 15:01 UTC (permalink / raw) To: johannes; +Cc: qemu-devel, mst Patchew URL: https://patchew.org/QEMU/20190911134539.25650-1-johannes@sipsolutions.net/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications Message-id: 20190911134539.25650-1-johannes@sipsolutions.net Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Switched to a new branch 'test' 9f34f61 libvhost-user: implement in-band notifications 8f4b013 docs: vhost-user: add in-band kick/call messages === OUTPUT BEGIN === 1/2 Checking commit 8f4b0138608c (docs: vhost-user: add in-band kick/call messages) 2/2 Checking commit 9f34f61c2044 (libvhost-user: implement in-band notifications) ERROR: code indent should never use tabs #183: FILE: contrib/libvhost-user/libvhost-user.c:1508: +^I dev->vq[index].handler, index);$ total: 1 errors, 0 warnings, 211 lines checked Patch 2/2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190911134539.25650-1-johannes@sipsolutions.net/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-09-17 12:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-11 13:45 [Qemu-devel] [RFC v2 0/2] vhost-user: in-band notifications Johannes Berg 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages Johannes Berg 2019-09-11 14:07 ` Michael S. Tsirkin 2019-09-11 15:09 ` Johannes Berg 2019-09-16 11:40 ` Johannes Berg 2019-09-16 15:30 ` Michael S. Tsirkin 2019-09-17 12:01 ` Johannes Berg 2019-09-11 19:15 ` Dr. David Alan Gilbert 2019-09-11 19:18 ` Johannes Berg 2019-09-12 8:09 ` Dr. David Alan Gilbert 2019-09-12 8:13 ` Johannes Berg 2019-09-11 13:45 ` [Qemu-devel] [RFC v2 2/2] libvhost-user: implement in-band notifications Johannes Berg 2019-09-11 15:01 ` [Qemu-devel] [RFC v2 0/2] vhost-user: " no-reply
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).