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 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 ?

>> 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