[Qemu-devel] [PATCH v2 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature

[Qemu-devel] [PATCH v2 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature

[Maxime Coquelin]

V2 makes vhost-user init to fail if the device implements config
feature but the backend doesn't.

While reviewing DPDK series adding support to VHOST_USER_SET_CONFIG
and VHOST_USER_GET_CONFIG request, I found that it was not backed
with a dedicated protocol feature.

This series addresses this by adding a new protocol feature bit,
and by only negotiating it if the device supports it, as suggested
by Michael. Indeed, if the feature is supported by other type of
devices in the future, it would confuse the backends as it couldn't
know whether the device really support it or not.

To know whether the vhost device support config feature, the trick
is to check whether it implemented the config_ops. That's the 
reason why the first patch moves setting the config ops in
vhost-user-blk befoire calling vhost_user_init().

The series targets v2.12 release, else we may have to disable these
requests in this release.

*NOTE*: The series has only been tested as I don't have the
environment to try it. Changpeng, can you please test it?

Thanks,
Maxime

Changes since v1:
=================
 - Fail vhost-user init if device implements config
   feature but the backend doesn't. (mst)

Maxime Coquelin (2):
  vhost-user-blk: set config ops before vhost-user init
  vhost-user: back SET/GET_CONFIG requests with a protocol feature

 docs/interop/vhost-user.txt | 21 ++++++++++++---------
 hw/block/vhost-user-blk.c   |  4 ++--
 hw/virtio/vhost-user.c      | 22 ++++++++++++++++++++++
 3 files changed, 36 insertions(+), 11 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init

[Maxime Coquelin]

As soon as vhost-user init is done, the backend may send
VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the
notification callback before it.

Also, it will be used to know whether the device supports
the config feature to advertize it or not.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/block/vhost-user-blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index f840f07dfe..262baca432 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
     s->dev.vq_index = 0;
     s->dev.backend_features = 0;
 
+    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
+
     ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0);
     if (ret < 0) {
         error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
@@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         s->blkcfg.num_queues = s->num_queues;
     }
 
-    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
-
     return;
 
 vhost_err:
-- 
2.14.3

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

[Maxime Coquelin]

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

Re: [Qemu-devel] [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init

[Liu, Changpeng]

> -----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 1/2] vhost-user-blk: set config ops before vhost-user init
> 
> As soon as vhost-user init is done, the backend may send
> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the
> notification callback before it.
> 
> Also, it will be used to know whether the device supports
> the config feature to advertize it or not.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  hw/block/vhost-user-blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f840f07dfe..262baca432 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
>      s->dev.vq_index = 0;
>      s->dev.backend_features = 0;
> 
> +    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
Please also remove the line "assert(hdev->vhost_ops);" in function vhost_dev_set_config_notifier at vhost.c file.
> +
>      ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0);
>      if (ret < 0) {
>          error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
> @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
>          s->blkcfg.num_queues = s->num_queues;
>      }
> 
> -    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
> -
>      return;
> 
>  vhost_err:
> --
> 2.14.3

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

[Liu, Changpeng]

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

Re: [Qemu-devel] [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init

[Maxime Coquelin]

On 03/29/2018 02:53 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 1/2] vhost-user-blk: set config ops before vhost-user init
>>
>> As soon as vhost-user init is done, the backend may send
>> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the
>> notification callback before it.
>>
>> Also, it will be used to know whether the device supports
>> the config feature to advertize it or not.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   hw/block/vhost-user-blk.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index f840f07dfe..262baca432 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState
>> *dev, Error **errp)
>>       s->dev.vq_index = 0;
>>       s->dev.backend_features = 0;
>>
>> +    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
> Please also remove the line "assert(hdev->vhost_ops);" in function vhost_dev_set_config_notifier at vhost.c file.

Oh right, posting v3 removing this.

Thanks,
Maxime
>> +
>>       ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0);
>>       if (ret < 0) {
>>           error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
>> @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState
>> *dev, Error **errp)
>>           s->blkcfg.num_queues = s->num_queues;
>>       }
>>
>> -    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>> -
>>       return;
>>
>>   vhost_err:
>> --
>> 2.14.3
> 

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

[Maxime Coquelin]

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
> 

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

[Liu, Changpeng]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, March 29, 2018 3:56 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; mst@redhat.com;
> marcandre.lureau@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a
> protocol feature
> 
> 
> 
> 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.
I have added the fix to the example based on your patch, I'm wondering
I can send it out for review now or wait for your commit being merged ?
> 
> 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
> >

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

[Maxime Coquelin]

On 03/29/2018 10:05 AM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, March 29, 2018 3:56 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; mst@redhat.com;
>> marcandre.lureau@redhat.com; qemu-devel@nongnu.org
>> Subject: Re: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a
>> protocol feature
>>
>>
>>
>> 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.
> I have added the fix to the example based on your patch, I'm wondering
> I can send it out for review now or wait for your commit being merged ?

I think you can post, and add a note in comments that it depends on my
series.

Thanks,
Maxime

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