Re: [Qemu-devel] [PATCH 1/1] vhost-user: modify SET_LOG_BASE only if VHOST_USER_PROTOCOL_F_LOG_SHMFD is set

[Thibaut Collet]

On Mon, Nov 16, 2015 at 6:09 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Mon, Nov 16, 2015 at 05:53:10PM +0100, Thibaut Collet wrote:
>> On Mon, Nov 16, 2015 at 5:21 PM, Michael S. Tsirkin <address@hidden> wrote:
>> > On Mon, Nov 16, 2015 at 05:14:37PM +0100, Thibaut Collet wrote:
>> >> Fixes: 2b8819c6eee5 ("vhost-user: modify SET_LOG_BASE to pass mmap size 
>> >> and
>> >> offset")
>> >>
>> >> For compatibility with old vhost backend content of the SET_LOG_BASE 
>> >> message
>> >> can not be modified.
>> >
>> > Hmm that's true. Interesting. But this only happens on migration,
>> > right? And if VHOST_USER_PROTOCOL_F_LOG_SHMFD is not set
>> > then we block migration. So how come the old message
>> > is ever sent?
>> >
>>
>> Agree. With the migration blocker on VHOST_USER_PROTOCOL_F_LOG_SHMFD
>> message with the new payload can not be sent to old vhost backend.
>> The documentation is a little bit confusing. The message payload for
>> SET_LOG_BASE still indicates a u64 whereas it is no more the case.
>>
>> Modification on the vhost-user.c file is not really needed but maybe
>> we can keep the patch on the doc ?
>
> Absolutely but let's say something like
>         Historically migration would still happen when
>         VHOST_USER_PROTOCOL_F_LOG_SHMFD was not negotiated.
>         In that case, the value sent was u64 with no file
>         descriptors. This message should be ignored.
>

OK.
Regarding the code of the function vhost_user_set_log_base any test on
the shmfd boolean can be removed, regarding the design it is always
true, and it will avoid any doubt if this function can be called with
other conditions than migration. A safety check can be added to log
this unexpected case i.e something like that:

@@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
       .size = sizeof(msg.payload.log),
     };

+    if (!shmfd) {
+        error_report("SET_LOG_BASE can not occur if "
+                          "VHOST_USER_PROTOCOL_F_LOG_SHMFD is not set ");
+        return -1;
+    }

-    if (shmfd && log->fd != -1) {
+    if (log->fd != -1) {
        fds[fd_num++] = log->fd;
    }

    vhost_user_write(dev, &msg, fds, fd_num);

-    if (shmfd) {
[snip]
-    }

     return 0;
}

>> >> The SET_LOG_BASE message payload is modified only if the
>> >> VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol feature has been negociated.
>> >>
>> >> The documentation has been updated accordingly with remarks from Marc 
>> >> André
>> >> Lureau.
>> >>
>> >> Signed-off-by: Thibaut Collet <address@hidden>
>> >> ---
>> >>  docs/specs/vhost-user.txt | 16 ++++++++++++++--
>> >>  hw/virtio/vhost-user.c    | 12 +++++++++---
>> >>  2 files changed, 23 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> >> index 26dde2e..da4bf9c 100644
>> >> --- a/docs/specs/vhost-user.txt
>> >> +++ b/docs/specs/vhost-user.txt
>> >> @@ -87,6 +87,14 @@ Depending on the request type, payload can be:
>> >>     User address: a 64-bit user address
>> >>     mmap offset: 64-bit offset where region starts in the mapped memory
>> >>
>> >> + * vhost user log description
>> >> +   -----------------
>> >> +   | size | offset |
>> >> +   -----------------
>> >> +
>> >> +   size: a 64-bit size
>> >> +   Offset: a 64-bit offset where log starts in the mapped memory
>> >> +
>> >>  In QEMU the vhost-user message is implemented with the following struct:
>> >>
>> >>  typedef struct VhostUserMsg {
>> >> @@ -280,14 +288,18 @@ Message types
>> >>
>> >>        Id: 6
>> >>        Equivalent ioctl: VHOST_SET_LOG_BASE
>> >> -      Master payload: u64
>> >> +      Master payload: - u64 if slave has not the 
>> >> VHOST_USER_PROTOCOL_F_LOG_SHMFD
>> >> +                        protocol feature
>> >> +                      - vhost user log if slave has the
>> >> +                        VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol feature
>> >>        Slave payload: N/A
>> >>
>> >>        Sets logging shared memory space.
>> >>        When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
>> >>        feature, the log memory fd is provided in the ancillary data of
>> >>        VHOST_USER_SET_LOG_BASE message, the size and offset of shared
>> >> -      memory area provided in the message.
>> >> +      memory area provided in the message and the message reply is an
>> >> +      empty message (size of 0).
>> >>
>> >>
>> >>   * VHOST_USER_SET_LOG_FD
>> >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> >> index c443602..dcdfd40 100644
>> >> --- a/hw/virtio/vhost-user.c
>> >> +++ b/hw/virtio/vhost-user.c
>> >> @@ -206,11 +206,17 @@ static int vhost_user_set_log_base(struct vhost_dev 
>> >> *dev, uint64_t base,
>> >>      VhostUserMsg msg = {
>> >>          .request = VHOST_USER_SET_LOG_BASE,
>> >>          .flags = VHOST_USER_VERSION,
>> >> -        .payload.log.mmap_size = log->size,
>> >> -        .payload.log.mmap_offset = 0,
>> >> -        .size = sizeof(msg.payload.log),
>> >>      };
>> >>
>> >> +    if (shmfd) {
>> >> +        msg.payload.log.mmap_size = log->size;
>> >> +        msg.payload.log.mmap_offset = 0;
>> >> +        msg.size = sizeof(msg.payload.log);
>> >> +    } else {
>> >> +        msg.payload.u64 = base;
>> >> +        msg.size = sizeof(msg.payload.u64);
>> >> +    }
>> >> +
>> >>      if (shmfd && log->fd != -1) {
>> >>          fds[fd_num++] = log->fd;
>> >>      }
>> >> --
>> >> 2.1.4