diff mbox

[RFC,v2,3/4] vhost-user: add slave-req-fd support

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

Commit Message

Maxime Coquelin April 14, 2017, 5:40 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to give a socket to the slave to let him make requests to the
master.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 docs/specs/vhost-user.txt |  30 +++++++++++-
 hw/virtio/vhost-user.c    | 117 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 2 deletions(-)

Comments

Peter Xu April 17, 2017, 3:29 a.m. UTC | #1
On Fri, Apr 14, 2017 at 07:40:55PM +0200, Maxime Coquelin wrote:

[...]

> @@ -486,6 +500,18 @@ 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
> +      Master payload: N/A
> +
> +      Set the socket file descriptor for slave initiated requests. It is passed
> +      in the ancillary data.
> +      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.

Here, do we need to mention REPLY_ACK as well? Like:

      If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must
      respond with zero in case the slave request channel is setup
      correctly, or non-zero otherwise.

Since I see the other two users are mentioning it.

[...]

> +static int vhost_setup_slave_channel(struct vhost_dev *dev)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_SLAVE_REQ_FD,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +    struct vhost_user *u = dev->opaque;
> +    int sv[2];
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (!virtio_has_feature(dev->protocol_features,
> +                            VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
> +        return 0;
> +    }
> +
> +    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
> +        error_report("socketpair() failed");
> +        return -1;
> +    }
> +
> +    u->slave_fd = sv[0];
> +    qemu_set_fd_handler(u->slave_fd, slave_read, NULL, dev);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
> +    vhost_user_write(dev, &msg, &sv[1], 1);

Do we need to close(sv[1]) afterward?

> +
> +    if (reply_supported) {
> +        return process_message_reply(dev, msg.request);

Here do we need to cleanup u->slave_fd if backend replied something
wrong? Or I guess it might be leaked.

Thanks,

> +    }
> +
> +    return 0;
> +}
> +
Maxime Coquelin April 19, 2017, 7:23 a.m. UTC | #2
On 04/17/2017 05:29 AM, Peter Xu wrote:
> On Fri, Apr 14, 2017 at 07:40:55PM +0200, Maxime Coquelin wrote:
> 
> [...]
> 
>> @@ -486,6 +500,18 @@ 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
>> +      Master payload: N/A
>> +
>> +      Set the socket file descriptor for slave initiated requests. It is passed
>> +      in the ancillary data.
>> +      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.
> 
> Here, do we need to mention REPLY_ACK as well? Like:
> 
>        If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must
>        respond with zero in case the slave request channel is setup
>        correctly, or non-zero otherwise.
> 
> Since I see the other two users are mentioning it.

Sure, it won't hurt. I'll add this in next revision.

> [...]
> 
>> +static int vhost_setup_slave_channel(struct vhost_dev *dev)
>> +{
>> +    VhostUserMsg msg = {
>> +        .request = VHOST_USER_SET_SLAVE_REQ_FD,
>> +        .flags = VHOST_USER_VERSION,
>> +    };
>> +    struct vhost_user *u = dev->opaque;
>> +    int sv[2];
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +
>> +    if (!virtio_has_feature(dev->protocol_features,
>> +                            VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
>> +        return 0;
>> +    }
>> +
>> +    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
>> +        error_report("socketpair() failed");
>> +        return -1;
>> +    }
>> +
>> +    u->slave_fd = sv[0];
>> +    qemu_set_fd_handler(u->slave_fd, slave_read, NULL, dev);
>> +
>> +    if (reply_supported) {
>> +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +
>> +    vhost_user_write(dev, &msg, &sv[1], 1);
> 
> Do we need to close(sv[1]) afterward?

Right, thanks. It will be leaked otherwise.

>> +
>> +    if (reply_supported) {
>> +        return process_message_reply(dev, msg.request);
> 
> Here do we need to cleanup u->slave_fd if backend replied something
> wrong? Or I guess it might be leaked.

That make sense, I'll fix this.


Thanks!
Maxime
> Thanks,
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>
diff mbox

Patch

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 036890f..49c6293 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.
@@ -252,6 +253,18 @@  Once the source has finished migration, rings will be stopped by
 the source. No further update must be done before rings are
 restarted.
 
+Slave communication
+-------------------
+
+An optional communication channel is provided if the slave declares
+VHOST_USER_PROTOCOL_F_SLAVE_REQ protocol feature, to allow the slave to make
+requests to the master.
+
+The fd is provided via VHOST_USER_SET_SLAVE_REQ_FD ancillary data.
+
+A slave may then send VHOST_USER_SLAVE_* messages to the master
+using this fd communication channel.
+
 Protocol features
 -----------------
 
@@ -260,9 +273,10 @@  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
--------------
+Master message types
+--------------------
 
  * VHOST_USER_GET_FEATURES
 
@@ -486,6 +500,18 @@  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
+      Master payload: N/A
+
+      Set the socket file descriptor for slave initiated requests. It is passed
+      in the ancillary data.
+      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
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f0e10d0..f26f11b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -33,6 +33,7 @@  enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_RARP = 2,
     VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
     VHOST_USER_PROTOCOL_F_NET_MTU = 4,
+    VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5,
 
     VHOST_USER_PROTOCOL_F_MAX
 };
@@ -61,9 +62,15 @@  typedef enum VhostUserRequest {
     VHOST_USER_SET_VRING_ENABLE = 18,
     VHOST_USER_SEND_RARP = 19,
     VHOST_USER_NET_SET_MTU = 20,
+    VHOST_USER_SET_SLAVE_REQ_FD = 21,
     VHOST_USER_MAX
 } VhostUserRequest;
 
+typedef enum VhostUserSlaveRequest {
+    VHOST_USER_SLAVE_NONE = 0,
+    VHOST_USER_SLAVE_MAX
+}  VhostUserSlaveRequest;
+
 typedef struct VhostUserMemoryRegion {
     uint64_t guest_phys_addr;
     uint64_t memory_size;
@@ -113,6 +120,7 @@  static VhostUserMsg m __attribute__ ((unused));
 
 struct vhost_user {
     CharBackend *chr;
+    int slave_fd;
 };
 
 static bool ioeventfd_enabled(void)
@@ -574,6 +582,104 @@  static int vhost_user_reset_device(struct vhost_dev *dev)
     return 0;
 }
 
+static void slave_read(void *opaque)
+{
+    struct vhost_dev *dev = opaque;
+    struct vhost_user *u = dev->opaque;
+    VhostUserMsg msg = { 0, };
+    int size, ret = 0;
+
+    /* Read header */
+    size = read(u->slave_fd, &msg, VHOST_USER_HDR_SIZE);
+    if (size != VHOST_USER_HDR_SIZE) {
+        error_report("Failed to read from slave.");
+        goto err;
+    }
+
+    if (msg.size > VHOST_USER_PAYLOAD_SIZE) {
+        error_report("Failed to read msg header."
+                " Size %d exceeds the maximum %zu.", msg.size,
+                VHOST_USER_PAYLOAD_SIZE);
+        goto err;
+    }
+
+    /* Read payload */
+    size = read(u->slave_fd, &msg.payload, msg.size);
+    if (size != msg.size) {
+        error_report("Failed to read payload from slave.");
+        goto err;
+    }
+
+    switch (msg.request) {
+    default:
+        error_report("Received unexpected msg type.");
+        ret = -EINVAL;
+    }
+
+    /*
+     * REPLY_ACK feature handling. Other reply types has to be managed
+     * directly in their request handlers.
+     */
+    if (msg.flags & VHOST_USER_NEED_REPLY_MASK) {
+        msg.flags &= ~VHOST_USER_NEED_REPLY_MASK;
+        msg.flags |= VHOST_USER_REPLY_MASK;
+
+        msg.payload.u64 = !!ret;
+        msg.size = sizeof(msg.payload.u64);
+
+        size = write(u->slave_fd, &msg, VHOST_USER_HDR_SIZE + msg.size);
+        if (size != VHOST_USER_HDR_SIZE + msg.size) {
+            error_report("Failed to send msg reply to slave.");
+            goto err;
+        }
+    }
+
+    return;
+
+err:
+    qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
+    close(u->slave_fd);
+    u->slave_fd = -1;
+    return;
+}
+
+static int vhost_setup_slave_channel(struct vhost_dev *dev)
+{
+    VhostUserMsg msg = {
+        .request = VHOST_USER_SET_SLAVE_REQ_FD,
+        .flags = VHOST_USER_VERSION,
+    };
+    struct vhost_user *u = dev->opaque;
+    int sv[2];
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+    if (!virtio_has_feature(dev->protocol_features,
+                            VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
+        return 0;
+    }
+
+    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+        error_report("socketpair() failed");
+        return -1;
+    }
+
+    u->slave_fd = sv[0];
+    qemu_set_fd_handler(u->slave_fd, slave_read, NULL, dev);
+
+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
+    vhost_user_write(dev, &msg, &sv[1], 1);
+
+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    }
+
+    return 0;
+}
+
 static int vhost_user_init(struct vhost_dev *dev, void *opaque)
 {
     uint64_t features;
@@ -584,6 +690,7 @@  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
 
     u = g_new0(struct vhost_user, 1);
     u->chr = opaque;
+    u->slave_fd = -1;
     dev->opaque = u;
 
     err = vhost_user_get_features(dev, &features);
@@ -624,6 +731,12 @@  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
                    "VHOST_USER_PROTOCOL_F_LOG_SHMFD feature.");
     }
 
+
+    err = vhost_setup_slave_channel(dev);
+    if (err < 0) {
+        return err;
+    }
+
     return 0;
 }
 
@@ -634,6 +747,10 @@  static int vhost_user_cleanup(struct vhost_dev *dev)
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
 
     u = dev->opaque;
+    if (u->slave_fd >= 0) {
+        close(u->slave_fd);
+        u->slave_fd = -1;
+    }
     g_free(u);
     dev->opaque = 0;