Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 03/29/2018 02:57 AM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, March 29, 2018 3:28 AM
>> To: mst@redhat.com; Liu, Changpeng <changpeng.liu@intel.com>;
>> marcandre.lureau@redhat.com; qemu-devel@nongnu.org
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a
>> protocol feature
>>
>> Without a dedicated protocol feature, QEMU cannot know whether
>> the backend can handle VHOST_USER_SET_CONFIG and
>> VHOST_USER_GET_CONFIG messages.
>>
>> This patch adds a protocol feature that is only advertised by
>> QEMU if the device implements the config ops. Vhost user init
>> fails if the device support the feature but the backend doesn't.
>>
>> The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG
>> requests if the protocol feature has been negotiated.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Passed our own vhost-user-blk target with the patch, I can submit a fix to QEMU vhost-user-blk example
> after this commit.
> 
> Tested-by: Changpeng Liu <changpeng.liu@intel.com>

Thanks for having tested, and for proposing to implement the example
part. Just notice I forgot to apply your Tested-by on the series.

Maxime
>> ---
>>   docs/interop/vhost-user.txt | 21 ++++++++++++---------
>>   hw/virtio/vhost-user.c      | 22 ++++++++++++++++++++++
>>   2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
>> index c058c407df..534caab18a 100644
>> --- a/docs/interop/vhost-user.txt
>> +++ b/docs/interop/vhost-user.txt
>> @@ -379,6 +379,7 @@ Protocol features
>>   #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
>>
>>   Master message types
>>   --------------------
>> @@ -664,7 +665,8 @@ Master message types
>>         Master payload: virtio device config space
>>         Slave payload: virtio device config space
>>
>> -      Submitted by the vhost-user master to fetch the contents of the virtio
>> +      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
>> +      submitted by the vhost-user master to fetch the contents of the virtio
>>         device configuration space, vhost-user slave's payload size MUST match
>>         master's request, vhost-user slave uses zero length of payload to
>>         indicate an error to vhost-user master. The vhost-user master may
>> @@ -677,7 +679,8 @@ Master message types
>>         Master payload: virtio device config space
>>         Slave payload: N/A
>>
>> -      Submitted by the vhost-user master when the Guest changes the virtio
>> +      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
>> +      submitted by the vhost-user master when the Guest changes the virtio
>>         device configuration space and also can be used for live migration
>>         on the destination host. The vhost-user slave must check the flags
>>         field, and slaves MUST NOT accept SET_CONFIG for read-only
>> @@ -766,13 +769,13 @@ Slave message types
>>        Slave payload: N/A
>>        Master payload: N/A
>>
>> -     Vhost-user slave sends such messages to notify that the virtio device's
>> -     configuration space has changed, for those host devices which can support
>> -     such feature, host driver can send VHOST_USER_GET_CONFIG message to
>> slave
>> -     to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is
>> -     negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must
>> -     respond with zero when operation is successfully completed, or non-zero
>> -     otherwise.
>> +     When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave
>> sends
>> +     such messages to notify that the virtio device's configuration space has
>> +     changed, for those host devices which can support such feature, host
>> +     driver can send VHOST_USER_GET_CONFIG message to slave to get the latest
>> +     content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave
>> set
>> +     the VHOST_USER_NEED_REPLY flag, master must respond with zero when
>> +     operation is successfully completed, or non-zero otherwise.
>>
>>   VHOST_USER_PROTOCOL_F_REPLY_ACK:
>>   -------------------------------
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 44aea5c0a8..cc8a24aa31 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature {
>>       VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
>>       VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
>>       VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
>> +    VHOST_USER_PROTOCOL_F_CONFIG = 9,
>>       VHOST_USER_PROTOCOL_F_MAX
>>   };
>>
>> @@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev, void
>> *opaque)
>>
>>           dev->protocol_features =
>>               protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK;
>> +
>> +        if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) {
>> +            /* Dont acknowledge CONFIG feature if device doesn't support it */
>> +            dev->protocol_features &= ~(1ULL <<
>> VHOST_USER_PROTOCOL_F_CONFIG);
>> +        } else if (!(protocol_features &
>> +                    (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
>> +            error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG "
>> +                    "but backend does not support it.");
>> +            return -1;
>> +        }
>> +
>>           err = vhost_user_set_protocol_features(dev, dev->protocol_features);
>>           if (err < 0) {
>>               return err;
>> @@ -1405,6 +1417,11 @@ static int vhost_user_get_config(struct vhost_dev
>> *dev, uint8_t *config,
>>           .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len,
>>       };
>>
>> +    if (!virtio_has_feature(dev->protocol_features,
>> +                VHOST_USER_PROTOCOL_F_CONFIG)) {
>> +        return -1;
>> +    }
>> +
>>       if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
>>           return -1;
>>       }
>> @@ -1448,6 +1465,11 @@ static int vhost_user_set_config(struct vhost_dev
>> *dev, const uint8_t *data,
>>           .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + size,
>>       };
>>
>> +    if (!virtio_has_feature(dev->protocol_features,
>> +                VHOST_USER_PROTOCOL_F_CONFIG)) {
>> +        return -1;
>> +    }
>> +
>>       if (reply_supported) {
>>           msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>       }
>> --
>> 2.14.3
>