diff mbox

[RFC,1/2] spec/vhost-user: Introduce secondary channel for slave initiated requests

Message ID 20170411101002.28451-2-maxime.coquelin@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Coquelin April 11, 2017, 10:10 a.m. UTC
This vhost-user specification update aims at enabling the
slave to send requests to the master using a dedicated socket
created by the master.

It can be used for example when the slave implements a device
IOTLB to send cache miss requests to the master.

The message types list is updated with an "Initiator" field to
indicate for each type whether the master and/or slave can
initiate the request.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 docs/specs/vhost-user.txt | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Marc-André Lureau April 11, 2017, 1:06 p.m. UTC | #1
Hi

On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> This vhost-user specification update aims at enabling the
> slave to send requests to the master using a dedicated socket
> created by the master.
>
> It can be used for example when the slave implements a device
> IOTLB to send cache miss requests to the master.
>
> The message types list is updated with an "Initiator" field to
> indicate for each type whether the master and/or slave can
> initiate the request.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>

This is very similar to a patch I proposed for shutdown slave initiated
requests:
https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html


> ---
>  docs/specs/vhost-user.txt | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 036890f..b365047 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -139,6 +139,7 @@ in the ancillary data:
>   * VHOST_USER_SET_VRING_KICK
>   * VHOST_USER_SET_VRING_CALL
>   * VHOST_USER_SET_VRING_ERR
> + * VHOST_USER_SET_SLAVE_REQ_FD
>
>
I like "slave-req-fd" better than "slave-fd"


>  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.
> @@ -150,6 +151,11 @@ As older slaves don't support negotiating protocol
> features,
>  a feature bit was dedicated for this purpose:
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +If the slave supports VHOST_USER_PROTOCOL_F_SLAVE_REQ protocol feature,
> the
> +master may create a secondary Unix domain socket and send its file
> descriptor
> +to the slave using VHOST_USER_SET_SLAVE_REQ_FD request. This new channel
> enables
> +the slave to send message requests and master to send message replies.
> +
>  Starting and stopping rings
>  ----------------------
>  Client must only process each ring when it is started.
> @@ -260,6 +266,7 @@ Protocol features
>  #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
>
>  Message types
>  -------------
> @@ -268,6 +275,7 @@ Message types
>
>        Id: 1
>        Equivalent ioctl: VHOST_GET_FEATURES
> +      Initiator: Master
>        Master payload: N/A
>        Slave payload: u64
>
> @@ -279,6 +287,7 @@ Message types
>
>        Id: 2
>        Ioctl: VHOST_SET_FEATURES
> +      Initiator: Master
>        Master payload: u64
>
>        Enable features in the underlying vhost implementation using a
> bitmask.
> @@ -289,6 +298,7 @@ Message types
>
>        Id: 15
>        Equivalent ioctl: VHOST_GET_FEATURES
> +      Initiator: Master
>        Master payload: N/A
>        Slave payload: u64
>
> @@ -302,6 +312,7 @@ Message types
>
>        Id: 16
>        Ioctl: VHOST_SET_FEATURES
> +      Initiator: Master
>        Master payload: u64
>
>        Enable protocol features in the underlying vhost implementation.
> @@ -314,6 +325,7 @@ Message types
>
>        Id: 3
>        Equivalent ioctl: VHOST_SET_OWNER
> +      Initiator: Master
>        Master payload: N/A
>
>        Issued when a new connection is established. It sets the current
> Master
> @@ -323,6 +335,7 @@ Message types
>   * VHOST_USER_RESET_OWNER
>
>        Id: 4
> +      Initiator: Master
>        Master payload: N/A
>
>        This is no longer used. Used to be sent to request disabling
> @@ -335,6 +348,7 @@ Message types
>
>        Id: 5
>        Equivalent ioctl: VHOST_SET_MEM_TABLE
> +      Initiator: Master
>        Master payload: memory regions description
>
>        Sets the memory map regions on the slave so it can translate the
> vring
> @@ -346,6 +360,7 @@ Message types
>
>        Id: 6
>        Equivalent ioctl: VHOST_SET_LOG_BASE
> +      Initiator: Master
>        Master payload: u64
>        Slave payload: N/A
>
> @@ -360,6 +375,7 @@ Message types
>
>        Id: 7
>        Equivalent ioctl: VHOST_SET_LOG_FD
> +      Initiator: Master
>        Master payload: N/A
>
>        Sets the logging file descriptor, which is passed as ancillary data.
> @@ -368,6 +384,7 @@ Message types
>
>        Id: 8
>        Equivalent ioctl: VHOST_SET_VRING_NUM
> +      Initiator: Master
>        Master payload: vring state description
>
>        Set the size of the queue.
> @@ -376,6 +393,7 @@ Message types
>
>        Id: 9
>        Equivalent ioctl: VHOST_SET_VRING_ADDR
> +      Initiator: Master
>        Master payload: vring address description
>        Slave payload: N/A
>
> @@ -385,6 +403,7 @@ Message types
>
>        Id: 10
>        Equivalent ioctl: VHOST_SET_VRING_BASE
> +      Initiator: Master
>        Master payload: vring state description
>
>        Sets the base offset in the available vring.
> @@ -393,6 +412,7 @@ Message types
>
>        Id: 11
>        Equivalent ioctl: VHOST_USER_GET_VRING_BASE
> +      Initiator: Master
>        Master payload: vring state description
>        Slave payload: vring state description
>
> @@ -402,6 +422,7 @@ Message types
>
>        Id: 12
>        Equivalent ioctl: VHOST_SET_VRING_KICK
> +      Initiator: Master
>        Master payload: u64
>
>        Set the event file descriptor for adding buffers to the vring. It
> @@ -415,6 +436,7 @@ Message types
>
>        Id: 13
>        Equivalent ioctl: VHOST_SET_VRING_CALL
> +      Initiator: Master
>        Master payload: u64
>
>        Set the event file descriptor to signal when buffers are used. It
> @@ -428,6 +450,7 @@ Message types
>
>        Id: 14
>        Equivalent ioctl: VHOST_SET_VRING_ERR
> +      Initiator: Master
>        Master payload: u64
>
>        Set the event file descriptor to signal when error occurs. It
> @@ -440,6 +463,7 @@ Message types
>
>        Id: 17
>        Equivalent ioctl: N/A
> +      Initiator: Master
>        Master payload: N/A
>        Slave payload: u64
>
> @@ -451,6 +475,7 @@ Message types
>
>        Id: 18
>        Equivalent ioctl: N/A
> +      Initiator: Master
>        Master payload: vring state description
>
>        Signal slave to enable or disable corresponding vring.
> @@ -461,6 +486,7 @@ Message types
>
>        Id: 19
>        Equivalent ioctl: N/A
> +      Initiator: Master
>        Master payload: u64
>
>        Ask vhost user backend to broadcast a fake RARP to notify the
> migration
> @@ -475,6 +501,7 @@ Message types
>
>        Id: 20
>        Equivalent ioctl: N/A
> +      Initiator: Master
>        Master payload: u64
>
>        Set host MTU value exposed to the guest.
> @@ -486,6 +513,17 @@ Message types
>        If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must respond
>        with zero in case the specified MTU is valid, or non-zero otherwise.
>
> + * VHOST_USER_SET_SLAVE_REQ_FD
> +
> +      Id: 21
> +      Equivalent ioctl: N/A
> +      Initiator: Master
> +
> +      Set the socket file descriptor for slave initiated requests.
> +      This request should be sent only when VHOST_USER_F_PROTOCOL_FEATURES
> +      has been negotiated, and protocol feature bit
> VHOST_USER_PROTOCOL_F_SLAVE_REQ
> +      bit is present in VHOST_USER_GET_PROTOCOL_FEATURES.
> +
>

looks good to me


>  VHOST_USER_PROTOCOL_F_REPLY_ACK:
>  -------------------------------
>  The original vhost-user specification only demands replies for certain
> --
> 2.9.3
>
>
> --
Marc-André Lureau
Maxime Coquelin April 11, 2017, 1:53 p.m. UTC | #2
Hi Marc-André,

On 04/11/2017 03:06 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin
> <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
>
>     This vhost-user specification update aims at enabling the
>     slave to send requests to the master using a dedicated socket
>     created by the master.
>
>     It can be used for example when the slave implements a device
>     IOTLB to send cache miss requests to the master.
>
>     The message types list is updated with an "Initiator" field to
>     indicate for each type whether the master and/or slave can
>     initiate the request.
>
>     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com
>     <mailto:maxime.coquelin@redhat.com>>
>
>
> This is very similar to a patch I proposed for shutdown slave initiated
> requests:
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html

Indeed, thanks for pointing this out, I wasn't aware of your series.

I find your proposal of having dedicated messages types
(VHOST_USER_SLAVE_*) cleaner.

Are you ok if I handover your patch, and replace
VHOST_USER_SET_SLAVE_FD to VHOST_USER_SET_SLAVE_REQ_FD?

>
>
>     ---
>      docs/specs/vhost-user.txt | 38 ++++++++++++++++++++++++++++++++++++++
>      1 file changed, 38 insertions(+)
>
>     diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>     index 036890f..b365047 100644
>     --- a/docs/specs/vhost-user.txt
>     +++ b/docs/specs/vhost-user.txt
>     @@ -139,6 +139,7 @@ in the ancillary data:
>       * VHOST_USER_SET_VRING_KICK
>       * VHOST_USER_SET_VRING_CALL
>       * VHOST_USER_SET_VRING_ERR
>     + * VHOST_USER_SET_SLAVE_REQ_FD
>
>
> I like "slave-req-fd" better than "slave-fd"
>
>
>      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.
>     @@ -150,6 +151,11 @@ As older slaves don't support negotiating
>     protocol features,
>      a feature bit was dedicated for this purpose:
>      #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
>     +If the slave supports VHOST_USER_PROTOCOL_F_SLAVE_REQ protocol
>     feature, the
>     +master may create a secondary Unix domain socket and send its file
>     descriptor
>     +to the slave using VHOST_USER_SET_SLAVE_REQ_FD request. This new
>     channel enables
>     +the slave to send message requests and master to send message replies.
>     +
>      Starting and stopping rings
>      ----------------------
>      Client must only process each ring when it is started.
>     @@ -260,6 +266,7 @@ Protocol features
>      #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
>
>      Message types
>      -------------
>     @@ -268,6 +275,7 @@ Message types
>
>            Id: 1
>            Equivalent ioctl: VHOST_GET_FEATURES
>     +      Initiator: Master
>            Master payload: N/A
>            Slave payload: u64
>
>     @@ -279,6 +287,7 @@ Message types
>
>            Id: 2
>            Ioctl: VHOST_SET_FEATURES
>     +      Initiator: Master
>            Master payload: u64
>
>            Enable features in the underlying vhost implementation using
>     a bitmask.
>     @@ -289,6 +298,7 @@ Message types
>
>            Id: 15
>            Equivalent ioctl: VHOST_GET_FEATURES
>     +      Initiator: Master
>            Master payload: N/A
>            Slave payload: u64
>
>     @@ -302,6 +312,7 @@ Message types
>
>            Id: 16
>            Ioctl: VHOST_SET_FEATURES
>     +      Initiator: Master
>            Master payload: u64
>
>            Enable protocol features in the underlying vhost implementation.
>     @@ -314,6 +325,7 @@ Message types
>
>            Id: 3
>            Equivalent ioctl: VHOST_SET_OWNER
>     +      Initiator: Master
>            Master payload: N/A
>
>            Issued when a new connection is established. It sets the
>     current Master
>     @@ -323,6 +335,7 @@ Message types
>       * VHOST_USER_RESET_OWNER
>
>            Id: 4
>     +      Initiator: Master
>            Master payload: N/A
>
>            This is no longer used. Used to be sent to request disabling
>     @@ -335,6 +348,7 @@ Message types
>
>            Id: 5
>            Equivalent ioctl: VHOST_SET_MEM_TABLE
>     +      Initiator: Master
>            Master payload: memory regions description
>
>            Sets the memory map regions on the slave so it can translate
>     the vring
>     @@ -346,6 +360,7 @@ Message types
>
>            Id: 6
>            Equivalent ioctl: VHOST_SET_LOG_BASE
>     +      Initiator: Master
>            Master payload: u64
>            Slave payload: N/A
>
>     @@ -360,6 +375,7 @@ Message types
>
>            Id: 7
>            Equivalent ioctl: VHOST_SET_LOG_FD
>     +      Initiator: Master
>            Master payload: N/A
>
>            Sets the logging file descriptor, which is passed as
>     ancillary data.
>     @@ -368,6 +384,7 @@ Message types
>
>            Id: 8
>            Equivalent ioctl: VHOST_SET_VRING_NUM
>     +      Initiator: Master
>            Master payload: vring state description
>
>            Set the size of the queue.
>     @@ -376,6 +393,7 @@ Message types
>
>            Id: 9
>            Equivalent ioctl: VHOST_SET_VRING_ADDR
>     +      Initiator: Master
>            Master payload: vring address description
>            Slave payload: N/A
>
>     @@ -385,6 +403,7 @@ Message types
>
>            Id: 10
>            Equivalent ioctl: VHOST_SET_VRING_BASE
>     +      Initiator: Master
>            Master payload: vring state description
>
>            Sets the base offset in the available vring.
>     @@ -393,6 +412,7 @@ Message types
>
>            Id: 11
>            Equivalent ioctl: VHOST_USER_GET_VRING_BASE
>     +      Initiator: Master
>            Master payload: vring state description
>            Slave payload: vring state description
>
>     @@ -402,6 +422,7 @@ Message types
>
>            Id: 12
>            Equivalent ioctl: VHOST_SET_VRING_KICK
>     +      Initiator: Master
>            Master payload: u64
>
>            Set the event file descriptor for adding buffers to the vring. It
>     @@ -415,6 +436,7 @@ Message types
>
>            Id: 13
>            Equivalent ioctl: VHOST_SET_VRING_CALL
>     +      Initiator: Master
>            Master payload: u64
>
>            Set the event file descriptor to signal when buffers are used. It
>     @@ -428,6 +450,7 @@ Message types
>
>            Id: 14
>            Equivalent ioctl: VHOST_SET_VRING_ERR
>     +      Initiator: Master
>            Master payload: u64
>
>            Set the event file descriptor to signal when error occurs. It
>     @@ -440,6 +463,7 @@ Message types
>
>            Id: 17
>            Equivalent ioctl: N/A
>     +      Initiator: Master
>            Master payload: N/A
>            Slave payload: u64
>
>     @@ -451,6 +475,7 @@ Message types
>
>            Id: 18
>            Equivalent ioctl: N/A
>     +      Initiator: Master
>            Master payload: vring state description
>
>            Signal slave to enable or disable corresponding vring.
>     @@ -461,6 +486,7 @@ Message types
>
>            Id: 19
>            Equivalent ioctl: N/A
>     +      Initiator: Master
>            Master payload: u64
>
>            Ask vhost user backend to broadcast a fake RARP to notify the
>     migration
>     @@ -475,6 +501,7 @@ Message types
>
>            Id: 20
>            Equivalent ioctl: N/A
>     +      Initiator: Master
>            Master payload: u64
>
>            Set host MTU value exposed to the guest.
>     @@ -486,6 +513,17 @@ Message types
>            If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must
>     respond
>            with zero in case the specified MTU is valid, or non-zero
>     otherwise.
>
>     + * VHOST_USER_SET_SLAVE_REQ_FD
>     +
>     +      Id: 21
>     +      Equivalent ioctl: N/A
>     +      Initiator: Master
>     +
>     +      Set the socket file descriptor for slave initiated requests.
>     +      This request should be sent only when
>     VHOST_USER_F_PROTOCOL_FEATURES
>     +      has been negotiated, and protocol feature bit
>     VHOST_USER_PROTOCOL_F_SLAVE_REQ
>     +      bit is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>     +
>
>
> looks good to me
>
>
>      VHOST_USER_PROTOCOL_F_REPLY_ACK:
>      -------------------------------
>      The original vhost-user specification only demands replies for certain
>     --
>     2.9.3
>
>
> --
> Marc-André Lureau
Marc-André Lureau April 14, 2017, 9:03 a.m. UTC | #3
Hi

On Tue, Apr 11, 2017 at 5:53 PM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> Hi Marc-André,
>
> On 04/11/2017 03:06 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
> >
> >     This vhost-user specification update aims at enabling the
> >     slave to send requests to the master using a dedicated socket
> >     created by the master.
> >
> >     It can be used for example when the slave implements a device
> >     IOTLB to send cache miss requests to the master.
> >
> >     The message types list is updated with an "Initiator" field to
> >     indicate for each type whether the master and/or slave can
> >     initiate the request.
> >
> >     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com
> >     <mailto:maxime.coquelin@redhat.com>>
> >
> >
> > This is very similar to a patch I proposed for shutdown slave initiated
> > requests:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>
> Indeed, thanks for pointing this out, I wasn't aware of your series.
>
> I find your proposal of having dedicated messages types
> (VHOST_USER_SLAVE_*) cleaner.
>
> ok


> Are you ok if I handover your patch, and replace
> VHOST_USER_SET_SLAVE_FD to VHOST_USER_SET_SLAVE_REQ_FD?
>

They are very similar, I suggest you update your patch with the best of
both.

I suppose you came to the same conclusion with me that trying to make the
communication both ways on the same fd would be quite difficult, although
it's a bit strange that the qemu implementation forces the design of the
protocol in some direction.
Wang, Wei W April 24, 2017, 8:05 a.m. UTC | #4
On 04/14/2017 05:03 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Apr 11, 2017 at 5:53 PM Maxime Coquelin 
> <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
>
>     Hi Marc-André,
>
>     On 04/11/2017 03:06 PM, Marc-André Lureau wrote:
>     > Hi
>     >
>     > On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin
>     > <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>
>     <mailto:maxime.coquelin@redhat.com
>     <mailto:maxime.coquelin@redhat.com>>> wrote:
>     >
>     >     This vhost-user specification update aims at enabling the
>     >     slave to send requests to the master using a dedicated socket
>     >     created by the master.
>     >
>     >     It can be used for example when the slave implements a device
>     >     IOTLB to send cache miss requests to the master.
>     >
>     >     The message types list is updated with an "Initiator" field to
>     >     indicate for each type whether the master and/or slave can
>     >     initiate the request.
>     >
>     >     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com
>     <mailto:maxime.coquelin@redhat.com>
>     >     <mailto:maxime.coquelin@redhat.com
>     <mailto:maxime.coquelin@redhat.com>>>
>     >
>     >
>     > This is very similar to a patch I proposed for shutdown slave
>     initiated
>     > requests:
>     > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>
>     Indeed, thanks for pointing this out, I wasn't aware of your series.
>
>     I find your proposal of having dedicated messages types
>     (VHOST_USER_SLAVE_*) cleaner.
>
> ok
>
>     Are you ok if I handover your patch, and replace
>     VHOST_USER_SET_SLAVE_FD to VHOST_USER_SET_SLAVE_REQ_FD?
>
>
> They are very similar, I suggest you update your patch with the best 
> of both.
>
> I suppose you came to the same conclusion with me that trying to make 
> the communication both ways on the same fd would be quite difficult, 
> although it's a bit strange that the qemu implementation forces the 
> design of the protocol in some direction.
> -- 
>

When would you get the implementation patch ready? Thanks.

Best,
Wei
Maxime Coquelin April 25, 2017, 11:55 a.m. UTC | #5
Hi Wei,

On 04/24/2017 10:05 AM, Wei Wang wrote:
> On 04/14/2017 05:03 PM, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Apr 11, 2017 at 5:53 PM Maxime Coquelin 
>> <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
>>
>>     Hi Marc-André,
>>
>>     On 04/11/2017 03:06 PM, Marc-André Lureau wrote:
>>     > Hi
>>     >
>>     > On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin
>>     > <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>
>>     <mailto:maxime.coquelin@redhat.com
>>     <mailto:maxime.coquelin@redhat.com>>> wrote:
>>     >
>>     >     This vhost-user specification update aims at enabling the
>>     >     slave to send requests to the master using a dedicated socket
>>     >     created by the master.
>>     >
>>     >     It can be used for example when the slave implements a device
>>     >     IOTLB to send cache miss requests to the master.
>>     >
>>     >     The message types list is updated with an "Initiator" field to
>>     >     indicate for each type whether the master and/or slave can
>>     >     initiate the request.
>>     >
>>     >     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com
>>     <mailto:maxime.coquelin@redhat.com>
>>     >     <mailto:maxime.coquelin@redhat.com
>>     <mailto:maxime.coquelin@redhat.com>>>
>>     >
>>     >
>>     > This is very similar to a patch I proposed for shutdown slave
>>     initiated
>>     > requests:
>>     > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>>
>>     Indeed, thanks for pointing this out, I wasn't aware of your series.
>>
>>     I find your proposal of having dedicated messages types
>>     (VHOST_USER_SLAVE_*) cleaner.
>>
>> ok
>>
>>     Are you ok if I handover your patch, and replace
>>     VHOST_USER_SET_SLAVE_FD to VHOST_USER_SET_SLAVE_REQ_FD?
>>
>>
>> They are very similar, I suggest you update your patch with the best 
>> of both.
>>
>> I suppose you came to the same conclusion with me that trying to make 
>> the communication both ways on the same fd would be quite difficult, 
>> although it's a bit strange that the qemu implementation forces the 
>> design of the protocol in some direction.
>> -- 
>>
> 
> When would you get the implementation patch ready? Thanks.

I sent second version of the RFC on April 14th, which comprises the
implementation:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02467.html

Cheers,
Maxime
Wang, Wei W April 26, 2017, 11:29 a.m. UTC | #6
On 04/25/2017 07:55 PM, Maxime Coquelin wrote:
> Hi Wei,
>
> On 04/24/2017 10:05 AM, Wei Wang wrote:
>> On 04/14/2017 05:03 PM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Apr 11, 2017 at 5:53 PM Maxime Coquelin 
>>> <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
>>>
>>>     Hi Marc-André,
>>>
>>>     On 04/11/2017 03:06 PM, Marc-André Lureau wrote:
>>>     > Hi
>>>     >
>>>     > On Tue, Apr 11, 2017 at 12:10 PM Maxime Coquelin
>>>     > <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>
>>>     <mailto:maxime.coquelin@redhat.com
>>>     <mailto:maxime.coquelin@redhat.com>>> wrote:
>>>     >
>>>     >     This vhost-user specification update aims at enabling the
>>>     >     slave to send requests to the master using a dedicated socket
>>>     >     created by the master.
>>>     >
>>>     >     It can be used for example when the slave implements a device
>>>     >     IOTLB to send cache miss requests to the master.
>>>     >
>>>     >     The message types list is updated with an "Initiator" 
>>> field to
>>>     >     indicate for each type whether the master and/or slave can
>>>     >     initiate the request.
>>>     >
>>>     >     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com
>>>     <mailto:maxime.coquelin@redhat.com>
>>>     >     <mailto:maxime.coquelin@redhat.com
>>>     <mailto:maxime.coquelin@redhat.com>>>
>>>     >
>>>     >
>>>     > This is very similar to a patch I proposed for shutdown slave
>>>     initiated
>>>     > requests:
>>>     > 
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>>>
>>>     Indeed, thanks for pointing this out, I wasn't aware of your 
>>> series.
>>>
>>>     I find your proposal of having dedicated messages types
>>>     (VHOST_USER_SLAVE_*) cleaner.
>>>
>>> ok
>>>
>>>     Are you ok if I handover your patch, and replace
>>>     VHOST_USER_SET_SLAVE_FD to VHOST_USER_SET_SLAVE_REQ_FD?
>>>
>>>
>>> They are very similar, I suggest you update your patch with the best 
>>> of both.
>>>
>>> I suppose you came to the same conclusion with me that trying to 
>>> make the communication both ways on the same fd would be quite 
>>> difficult, although it's a bit strange that the qemu implementation 
>>> forces the design of the protocol in some direction.
>>> -- 
>>>
>>
>> When would you get the implementation patch ready? Thanks.
>
> I sent second version of the RFC on April 14th, which comprises the
> implementation:
> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02467.html

Thanks, Maxime. I was trying to make the connection bidirectional
(https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg02617.html),
which was reported as problematic due to the possibility of race (though
I think it can be solved by re-sending the msg in that rare case).

Anyway, hope to see you guys' second channel based implementation to
be merged soon. I would also consider to switch to use it then.

Best,
Wei
diff mbox

Patch

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 036890f..b365047 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -139,6 +139,7 @@  in the ancillary data:
  * VHOST_USER_SET_VRING_KICK
  * VHOST_USER_SET_VRING_CALL
  * VHOST_USER_SET_VRING_ERR
+ * VHOST_USER_SET_SLAVE_REQ_FD
 
 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.
@@ -150,6 +151,11 @@  As older slaves don't support negotiating protocol features,
 a feature bit was dedicated for this purpose:
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
 
+If the slave supports VHOST_USER_PROTOCOL_F_SLAVE_REQ protocol feature, the
+master may create a secondary Unix domain socket and send its file descriptor
+to the slave using VHOST_USER_SET_SLAVE_REQ_FD request. This new channel enables
+the slave to send message requests and master to send message replies.
+
 Starting and stopping rings
 ----------------------
 Client must only process each ring when it is started.
@@ -260,6 +266,7 @@  Protocol features
 #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
 
 Message types
 -------------
@@ -268,6 +275,7 @@  Message types
 
       Id: 1
       Equivalent ioctl: VHOST_GET_FEATURES
+      Initiator: Master
       Master payload: N/A
       Slave payload: u64
 
@@ -279,6 +287,7 @@  Message types
 
       Id: 2
       Ioctl: VHOST_SET_FEATURES
+      Initiator: Master
       Master payload: u64
 
       Enable features in the underlying vhost implementation using a bitmask.
@@ -289,6 +298,7 @@  Message types
 
       Id: 15
       Equivalent ioctl: VHOST_GET_FEATURES
+      Initiator: Master
       Master payload: N/A
       Slave payload: u64
 
@@ -302,6 +312,7 @@  Message types
 
       Id: 16
       Ioctl: VHOST_SET_FEATURES
+      Initiator: Master
       Master payload: u64
 
       Enable protocol features in the underlying vhost implementation.
@@ -314,6 +325,7 @@  Message types
 
       Id: 3
       Equivalent ioctl: VHOST_SET_OWNER
+      Initiator: Master
       Master payload: N/A
 
       Issued when a new connection is established. It sets the current Master
@@ -323,6 +335,7 @@  Message types
  * VHOST_USER_RESET_OWNER
 
       Id: 4
+      Initiator: Master
       Master payload: N/A
 
       This is no longer used. Used to be sent to request disabling
@@ -335,6 +348,7 @@  Message types
 
       Id: 5
       Equivalent ioctl: VHOST_SET_MEM_TABLE
+      Initiator: Master
       Master payload: memory regions description
 
       Sets the memory map regions on the slave so it can translate the vring
@@ -346,6 +360,7 @@  Message types
 
       Id: 6
       Equivalent ioctl: VHOST_SET_LOG_BASE
+      Initiator: Master
       Master payload: u64
       Slave payload: N/A
 
@@ -360,6 +375,7 @@  Message types
 
       Id: 7
       Equivalent ioctl: VHOST_SET_LOG_FD
+      Initiator: Master
       Master payload: N/A
 
       Sets the logging file descriptor, which is passed as ancillary data.
@@ -368,6 +384,7 @@  Message types
 
       Id: 8
       Equivalent ioctl: VHOST_SET_VRING_NUM
+      Initiator: Master
       Master payload: vring state description
 
       Set the size of the queue.
@@ -376,6 +393,7 @@  Message types
 
       Id: 9
       Equivalent ioctl: VHOST_SET_VRING_ADDR
+      Initiator: Master
       Master payload: vring address description
       Slave payload: N/A
 
@@ -385,6 +403,7 @@  Message types
 
       Id: 10
       Equivalent ioctl: VHOST_SET_VRING_BASE
+      Initiator: Master
       Master payload: vring state description
 
       Sets the base offset in the available vring.
@@ -393,6 +412,7 @@  Message types
 
       Id: 11
       Equivalent ioctl: VHOST_USER_GET_VRING_BASE
+      Initiator: Master
       Master payload: vring state description
       Slave payload: vring state description
 
@@ -402,6 +422,7 @@  Message types
 
       Id: 12
       Equivalent ioctl: VHOST_SET_VRING_KICK
+      Initiator: Master
       Master payload: u64
 
       Set the event file descriptor for adding buffers to the vring. It
@@ -415,6 +436,7 @@  Message types
 
       Id: 13
       Equivalent ioctl: VHOST_SET_VRING_CALL
+      Initiator: Master
       Master payload: u64
 
       Set the event file descriptor to signal when buffers are used. It
@@ -428,6 +450,7 @@  Message types
 
       Id: 14
       Equivalent ioctl: VHOST_SET_VRING_ERR
+      Initiator: Master
       Master payload: u64
 
       Set the event file descriptor to signal when error occurs. It
@@ -440,6 +463,7 @@  Message types
 
       Id: 17
       Equivalent ioctl: N/A
+      Initiator: Master
       Master payload: N/A
       Slave payload: u64
 
@@ -451,6 +475,7 @@  Message types
 
       Id: 18
       Equivalent ioctl: N/A
+      Initiator: Master
       Master payload: vring state description
 
       Signal slave to enable or disable corresponding vring.
@@ -461,6 +486,7 @@  Message types
 
       Id: 19
       Equivalent ioctl: N/A
+      Initiator: Master
       Master payload: u64
 
       Ask vhost user backend to broadcast a fake RARP to notify the migration
@@ -475,6 +501,7 @@  Message types
 
       Id: 20
       Equivalent ioctl: N/A
+      Initiator: Master
       Master payload: u64
 
       Set host MTU value exposed to the guest.
@@ -486,6 +513,17 @@  Message types
       If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must respond
       with zero in case the specified MTU is valid, or non-zero otherwise.
 
+ * VHOST_USER_SET_SLAVE_REQ_FD
+
+      Id: 21
+      Equivalent ioctl: N/A
+      Initiator: Master
+
+      Set the socket file descriptor for slave initiated requests.
+      This request should be sent only when VHOST_USER_F_PROTOCOL_FEATURES
+      has been negotiated, and protocol feature bit VHOST_USER_PROTOCOL_F_SLAVE_REQ
+      bit is present in VHOST_USER_GET_PROTOCOL_FEATURES.
+
 VHOST_USER_PROTOCOL_F_REPLY_ACK:
 -------------------------------
 The original vhost-user specification only demands replies for certain