Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which can be
used for live migration of vhost user devices, also vhost user devices
can benefit from the messages to get/set virtio config space from/to the
I/O target. For the purpose to support virtio config space change,
VHOST_USER_SET_CONFIG_FD message is added as the event notifier
in case virtio config space change in the I/O target.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 docs/interop/vhost-user.txt       | 39 ++++++++++++++++
 hw/virtio/vhost-user.c            | 98 +++++++++++++++++++++++++++++++++++++++
 hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
 include/hw/virtio/vhost-backend.h |  8 ++++
 include/hw/virtio/vhost.h         | 16 +++++++
 5 files changed, 224 insertions(+)

diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index 954771d..1b98388 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -116,6 +116,16 @@ Depending on the request type, payload can be:
     - 3: IOTLB invalidate
     - 4: IOTLB access fail
 
+ * Virtio device config space
+   ---------------------------
+   | offset | size | payload |
+   ---------------------------
+
+   Offset: a 32-bit offset of virtio device's configuration space
+   Size: a 32-bit size of configuration space that master wanted to change
+   Payload: a 256-bytes array holding the contents of the virtio
+       device's configuration space
+
 In QEMU the vhost-user message is implemented with the following struct:
 
 typedef struct VhostUserMsg {
@@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
         VhostUserMemory memory;
         VhostUserLog log;
         struct vhost_iotlb_msg iotlb;
+        VhostUserConfig config;
     };
 } QEMU_PACKED VhostUserMsg;
 
@@ -596,6 +607,34 @@ Master message types
       and expect this message once (per VQ) during device configuration
       (ie. before the master starts the VQ).
 
+ * VHOST_USER_GET_CONFIG
+      Id: 24
+      Equivalent ioctl: N/A
+      Master payload: virtio device config space
+
+      Submitted by the vhost-user master to fetch the contents of the virtio
+      device configuration space. The vhost-user master may cache the contents
+      to avoid repeated VHOST_USER_GET_CONFIG calls.
+
+* VHOST_USER_SET_CONFIG
+      Id: 25
+      Equivalent ioctl: N/A
+      Master payload: virtio device config space
+
+      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.
+
+* VHOST_USER_SET_CONFIG_FD
+      Id: 26
+      Equivalent ioctl: N/A
+      Master payload: N/A
+
+      Sets the notifier file descriptor, which is passed as ancillary data.
+      The vhost-user slave uses the file descriptor to notify the vhost-user
+      master of changes to the virtio configuration space. The vhost-user
+      master can read the virtio configuration space to get the latest update.
+
 Slave message types
 -------------------
 
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 093675e..ef1687b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -26,6 +26,11 @@
 #define VHOST_MEMORY_MAX_NREGIONS    8
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
 
+/*
+ * Maximum size of virtio device config space
+ */
+#define VHOST_USER_MAX_CONFIG_SIZE 256
+
 enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_MQ = 0,
     VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
@@ -65,6 +70,9 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_SLAVE_REQ_FD = 21,
     VHOST_USER_IOTLB_MSG = 22,
     VHOST_USER_SET_VRING_ENDIAN = 23,
+    VHOST_USER_GET_CONFIG = 24,
+    VHOST_USER_SET_CONFIG = 25,
+    VHOST_USER_SET_CONFIG_FD = 26,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -92,6 +100,12 @@ typedef struct VhostUserLog {
     uint64_t mmap_offset;
 } VhostUserLog;
 
+typedef struct VhostUserConfig {
+    uint32_t offset;
+    uint32_t size;
+    uint8_t region[VHOST_USER_MAX_CONFIG_SIZE];
+} VhostUserConfig;
+
 typedef struct VhostUserMsg {
     VhostUserRequest request;
 
@@ -109,6 +123,7 @@ typedef struct VhostUserMsg {
         VhostUserMemory memory;
         VhostUserLog log;
         struct vhost_iotlb_msg iotlb;
+        VhostUserConfig config;
     } payload;
 } QEMU_PACKED VhostUserMsg;
 
@@ -922,6 +937,86 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
     /* No-op as the receive channel is not dedicated to IOTLB messages. */
 }
 
+static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
+                                 size_t config_len)
+{
+    VhostUserMsg msg = {
+        .request = VHOST_USER_GET_CONFIG,
+        .flags = VHOST_USER_VERSION,
+        .size = sizeof(msg.payload.config),
+    };
+
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
+
+    if (vhost_user_read(dev, &msg) < 0) {
+        return -1;
+    }
+
+    if (msg.request != VHOST_USER_GET_CONFIG) {
+        error_report("Received unexpected msg type. Expected %d received %d",
+                     VHOST_USER_GET_CONFIG, msg.request);
+        return -1;
+    }
+
+    if (msg.size != sizeof(msg.payload.config)) {
+        error_report("Received bad msg size.");
+        return -1;
+    }
+
+    memcpy(config, msg.payload.config.region, config_len);
+
+    return 0;
+}
+
+static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *config,
+                                 uint32_t offset, uint32_t size)
+{
+    uint8_t *p;
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+    VhostUserMsg msg = {
+        .request = VHOST_USER_SET_CONFIG,
+        .flags = VHOST_USER_VERSION,
+        .size = sizeof(msg.payload.config),
+    };
+
+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
+    msg.payload.config.offset = offset;
+    msg.payload.config.size = size;
+    p = msg.payload.config.region;
+    memcpy(p + offset, config + offset, size);
+
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
+
+    if (reply_supported) {
+        return process_message_reply(dev, &msg);
+    }
+
+    return 0;
+}
+
+static int vhost_user_set_config_fd(struct vhost_dev *dev, int fd)
+{
+    VhostUserMsg msg = {
+       .request = VHOST_USER_SET_CONFIG_FD,
+       .flags = VHOST_USER_VERSION,
+    };
+
+    if (vhost_user_write(dev, &msg, &fd, 1) < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
         .vhost_backend_init = vhost_user_init,
@@ -948,4 +1043,7 @@ const VhostOps user_ops = {
         .vhost_net_set_mtu = vhost_user_net_set_mtu,
         .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
         .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+        .vhost_get_config = vhost_user_get_config,
+        .vhost_set_config = vhost_user_set_config,
+        .vhost_set_config_fd = vhost_user_set_config_fd,
 };
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddc42f0..744ca85 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1354,6 +1354,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
+    if (hdev->config_ops) {
+        event_notifier_cleanup(&hdev->config_notifier);
+    }
     if (hdev->mem) {
         /* those are only safe after successful init */
         memory_listener_unregister(&hdev->memory_listener);
@@ -1501,6 +1504,66 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
     }
 }
 
+int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
+                         size_t config_len)
+{
+    assert(hdev->vhost_ops);
+
+    if (hdev->vhost_ops->vhost_get_config) {
+        return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
+    }
+
+    return 0;
+}
+
+int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *config,
+                         uint32_t offset, uint32_t size)
+{
+    assert(hdev->vhost_ops);
+
+    if (hdev->vhost_ops->vhost_set_config) {
+        return hdev->vhost_ops->vhost_set_config(hdev, config, offset, size);
+    }
+
+    return 0;
+}
+
+static void vhost_dev_config_notifier_read(EventNotifier *n)
+{
+    struct vhost_dev *hdev = container_of(n, struct vhost_dev,
+                                         config_notifier);
+
+    if (event_notifier_test_and_clear(n)) {
+        if (hdev->config_ops) {
+            hdev->config_ops->vhost_dev_config_notifier(hdev);
+        }
+    }
+}
+
+int vhost_dev_set_config_notifier(struct vhost_dev *hdev,
+                                  const VhostDevConfigOps *ops)
+{
+    int r, fd;
+
+    assert(hdev->vhost_ops);
+
+    r = event_notifier_init(&hdev->config_notifier, 0);
+    if (r < 0) {
+        return r;
+    }
+
+    hdev->config_ops = ops;
+    event_notifier_set_handler(&hdev->config_notifier,
+                               vhost_dev_config_notifier_read);
+
+    if (hdev->vhost_ops->vhost_set_config_fd) {
+        fd  = event_notifier_get_fd(&hdev->config_notifier);
+        return hdev->vhost_ops->vhost_set_config_fd(hdev, fd);
+    }
+
+    return 0;
+}
+
 /* Host notifiers must be enabled at this point. */
 int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index a7a5f22..3ab0b79 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -84,6 +84,11 @@ typedef void (*vhost_set_iotlb_callback_op)(struct vhost_dev *dev,
                                            int enabled);
 typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
                                               struct vhost_iotlb_msg *imsg);
+typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *config,
+                                   uint32_t offset, uint32_t size);
+typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config,
+                                   size_t config_len);
+typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd);
 
 typedef struct VhostOps {
     VhostBackendType backend_type;
@@ -118,6 +123,9 @@ typedef struct VhostOps {
     vhost_vsock_set_running_op vhost_vsock_set_running;
     vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
     vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg;
+    vhost_get_config_op vhost_get_config;
+    vhost_set_config_op vhost_set_config;
+    vhost_set_config_fd_op vhost_set_config_fd;
 } VhostOps;
 
 extern const VhostOps user_ops;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 467dc77..593056d 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -46,6 +46,12 @@ struct vhost_iommu {
     QLIST_ENTRY(vhost_iommu) iommu_next;
 };
 
+typedef struct VhostDevConfigOps {
+    /* Vhost device config space changed callback
+     */
+    void (*vhost_dev_config_notifier)(struct vhost_dev *dev);
+} VhostDevConfigOps;
+
 struct vhost_memory;
 struct vhost_dev {
     VirtIODevice *vdev;
@@ -76,6 +82,8 @@ struct vhost_dev {
     QLIST_ENTRY(vhost_dev) entry;
     QLIST_HEAD(, vhost_iommu) iommu_list;
     IOMMUNotifier n;
+    EventNotifier config_notifier;
+    const VhostDevConfigOps *config_ops;
 };
 
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
@@ -106,4 +114,12 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
                           struct vhost_vring_file *file);
 
 int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
+int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
+                         size_t config_len);
+int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *config,
+                         uint32_t offset, uint32_t size);
+/* notifier callback in case vhost device config space changed
+ */
+int vhost_dev_set_config_notifier(struct vhost_dev *dev,
+                                  const VhostDevConfigOps *ops);
 #endif
-- 
1.9.3

On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which can be
> used for live migration of vhost user devices, also vhost user devices
> can benefit from the messages to get/set virtio config space from/to the
> I/O target. For the purpose to support virtio config space change,
> VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> in case virtio config space change in the I/O target.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
>  hw/virtio/vhost-user.c            | 98 +++++++++++++++++++++++++++++++++++++++
>  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  8 ++++
>  include/hw/virtio/vhost.h         | 16 +++++++
>  5 files changed, 224 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 954771d..1b98388 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
>      - 3: IOTLB invalidate
>      - 4: IOTLB access fail
>  
> + * Virtio device config space
> +   ---------------------------
> +   | offset | size | payload |
> +   ---------------------------
> +
> +   Offset: a 32-bit offset of virtio device's configuration space

s/of/in the/

> +   Size: a 32-bit size of configuration space that master wanted to change

Is this also used for GET_CONFIG?  If yes, I suggest "a 32-bit
configuration space access size in bytes".

Please mention that Size must be <= 256 bytes.

> +   Payload: a 256-bytes array holding the contents of the virtio
> +       device's configuration space

What about bytes outside the [offset, offset+size) range?  I guess they
must be 0 and are ignored by the master/slave.

Would it be cleaner to make Payload a variable-sized field with Size
bytes?  That way it's not necessary to transfer 0s and memcpy() a subset
of the payload array.

> +
>  In QEMU the vhost-user message is implemented with the following struct:
>  
>  typedef struct VhostUserMsg {
> @@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
>          VhostUserMemory memory;
>          VhostUserLog log;
>          struct vhost_iotlb_msg iotlb;
> +        VhostUserConfig config;
>      };
>  } QEMU_PACKED VhostUserMsg;
>  
> @@ -596,6 +607,34 @@ Master message types
>        and expect this message once (per VQ) during device configuration
>        (ie. before the master starts the VQ).
>  
> + * VHOST_USER_GET_CONFIG
> +      Id: 24
> +      Equivalent ioctl: N/A
> +      Master payload: virtio device config space
> +
> +      Submitted by the vhost-user master to fetch the contents of the virtio
> +      device configuration space. The vhost-user master may cache the contents
> +      to avoid repeated VHOST_USER_GET_CONFIG calls.
> +
> +* VHOST_USER_SET_CONFIG
> +      Id: 25
> +      Equivalent ioctl: N/A
> +      Master payload: virtio device config space
> +
> +      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.

There might be security issues if the vhost slave cannot tell whether
SET_CONFIG is coming from the guest driver or from the master process
(live migration).  Typically certain fields are read-only for the guest
driver.  Maybe those fields need to be set by the master after live
migration.

One way to solve this is adding a flags field to the message.  A special
flag can be used for live migration so the slave knows that this
SET_CONFIG message is allowed to write to read-only fields.

It's also worth documenting that slaves MUST NOT accept SET_CONFIG for
read-only configuration space fields unless the live migration bit is
set.  Hopefully this will remind implementors to think through the
security issues.

On Mon, Nov 20, 2017 at 04:26:31PM +0000, Stefan Hajnoczi wrote:
> On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> > Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which can be
> > used for live migration of vhost user devices, also vhost user devices
> > can benefit from the messages to get/set virtio config space from/to the
> > I/O target. For the purpose to support virtio config space change,
> > VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> > in case virtio config space change in the I/O target.
> > 
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
> >  hw/virtio/vhost-user.c            | 98 +++++++++++++++++++++++++++++++++++++++
> >  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
> >  include/hw/virtio/vhost-backend.h |  8 ++++
> >  include/hw/virtio/vhost.h         | 16 +++++++
> >  5 files changed, 224 insertions(+)
> > 
> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > index 954771d..1b98388 100644
> > --- a/docs/interop/vhost-user.txt
> > +++ b/docs/interop/vhost-user.txt
> > @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
> >      - 3: IOTLB invalidate
> >      - 4: IOTLB access fail
> >  
> > + * Virtio device config space
> > +   ---------------------------
> > +   | offset | size | payload |
> > +   ---------------------------
> > +
> > +   Offset: a 32-bit offset of virtio device's configuration space
> 
> s/of/in the/
> 
> > +   Size: a 32-bit size of configuration space that master wanted to change
> 
> Is this also used for GET_CONFIG?  If yes, I suggest "a 32-bit
> configuration space access size in bytes".
> 
> Please mention that Size must be <= 256 bytes.
> 
> > +   Payload: a 256-bytes array holding the contents of the virtio
> > +       device's configuration space
> 
> What about bytes outside the [offset, offset+size) range?  I guess they
> must be 0 and are ignored by the master/slave.
> 
> Would it be cleaner to make Payload a variable-sized field with Size
> bytes?  That way it's not necessary to transfer 0s and memcpy() a subset
> of the payload array.
> 
> > +
> >  In QEMU the vhost-user message is implemented with the following struct:
> >  
> >  typedef struct VhostUserMsg {
> > @@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
> >          VhostUserMemory memory;
> >          VhostUserLog log;
> >          struct vhost_iotlb_msg iotlb;
> > +        VhostUserConfig config;
> >      };
> >  } QEMU_PACKED VhostUserMsg;
> >  
> > @@ -596,6 +607,34 @@ Master message types
> >        and expect this message once (per VQ) during device configuration
> >        (ie. before the master starts the VQ).
> >  
> > + * VHOST_USER_GET_CONFIG
> > +      Id: 24
> > +      Equivalent ioctl: N/A
> > +      Master payload: virtio device config space
> > +
> > +      Submitted by the vhost-user master to fetch the contents of the virtio
> > +      device configuration space. The vhost-user master may cache the contents
> > +      to avoid repeated VHOST_USER_GET_CONFIG calls.
> > +
> > +* VHOST_USER_SET_CONFIG
> > +      Id: 25
> > +      Equivalent ioctl: N/A
> > +      Master payload: virtio device config space
> > +
> > +      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.
> 
> There might be security issues if the vhost slave cannot tell whether
> SET_CONFIG is coming from the guest driver or from the master process
> (live migration).  Typically certain fields are read-only for the guest
> driver.  Maybe those fields need to be set by the master after live
> migration.
> 
> One way to solve this is adding a flags field to the message.  A special
> flag can be used for live migration so the slave knows that this
> SET_CONFIG message is allowed to write to read-only fields.
> 
> It's also worth documenting that slaves MUST NOT accept SET_CONFIG for
> read-only configuration space fields unless the live migration bit is
> set.  Hopefully this will remind implementors to think through the
> security issues.

Live migrations is supposed to be migrating guest writeable state too.
If you mean migrating RO fields like size, then
I don't think it's a good idea to reuse SET_CONFIG for that.
SET_CONFIG should obey exactly the virtio semantics.

And I agree, it should say that slave must treat it as a write,
and get config as a read according to virtio semantics.

If someone needs to pass configuration from qemu to
slave, let's add specific messages with precisely defined semantics.

Which reminds me.

virtio 1 changed endian-ness for config.

I think we should specify it's all virtio 1 format, and
just disallow vhost blk for virtio 0 guests.

-- 
MST

On 20/11/2017 21:44, Michael S. Tsirkin wrote:
> Live migrations is supposed to be migrating guest writeable state too.
> If you mean migrating RO fields like size, then
> I don't think it's a good idea to reuse SET_CONFIG for that.
> SET_CONFIG should obey exactly the virtio semantics.
> 
> And I agree, it should say that slave must treat it as a write,
> and get config as a read according to virtio semantics.
> 
> If someone needs to pass configuration from qemu to
> slave, let's add specific messages with precisely defined semantics.

Fair enough, but I'd add nevertheless a 32-bit flags field to both
GET_CONFIG and SET_CONFIG, and document that the slave MUST check that
it is zero and otherwise fail.

Paolo

On Tue, Nov 21, 2017 at 01:16:12AM +0100, Paolo Bonzini wrote:
> On 20/11/2017 21:44, Michael S. Tsirkin wrote:
> > Live migrations is supposed to be migrating guest writeable state too.
> > If you mean migrating RO fields like size, then
> > I don't think it's a good idea to reuse SET_CONFIG for that.
> > SET_CONFIG should obey exactly the virtio semantics.
> > 
> > And I agree, it should say that slave must treat it as a write,
> > and get config as a read according to virtio semantics.
> > 
> > If someone needs to pass configuration from qemu to
> > slave, let's add specific messages with precisely defined semantics.
> 
> Fair enough, but I'd add nevertheless a 32-bit flags field to both
> GET_CONFIG and SET_CONFIG, and document that the slave MUST check that
> it is zero and otherwise fail.
> 
> Paolo

We generally just use protocol feature bits for extensions.
But I have no problem with reserved padding if desired
which seems to be what's described here.

-- 
MST

On 21/11/2017 04:12, Michael S. Tsirkin wrote:
>> Fair enough, but I'd add nevertheless a 32-bit flags field to both
>> GET_CONFIG and SET_CONFIG, and document that the slave MUST check that
>> it is zero and otherwise fail.
>
> We generally just use protocol feature bits for extensions.
> But I have no problem with reserved padding if desired
> which seems to be what's described here.

Correct, you would still have a protocol feature bit, but the format
would be less susceptible to struct size changes which can simplify the
implementation a bit.

Paolo

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Tuesday, November 21, 2017 8:16 AM
> To: Michael S. Tsirkin <mst@redhat.com>; Stefan Hajnoczi <stefanha@gmail.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; qemu-devel@nongnu.org;
> marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R
> <james.r.harris@intel.com>
> Subject: Re: [PATCH v5 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On 20/11/2017 21:44, Michael S. Tsirkin wrote:
> > Live migrations is supposed to be migrating guest writeable state too.
> > If you mean migrating RO fields like size, then
> > I don't think it's a good idea to reuse SET_CONFIG for that.
> > SET_CONFIG should obey exactly the virtio semantics.
> >
> > And I agree, it should say that slave must treat it as a write,
> > and get config as a read according to virtio semantics.
> >
> > If someone needs to pass configuration from qemu to
> > slave, let's add specific messages with precisely defined semantics.
> 
> Fair enough, but I'd add nevertheless a 32-bit flags field to both
> GET_CONFIG and SET_CONFIG, and document that the slave MUST check that
> it is zero and otherwise fail.
Ok.
> 
> Paolo

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, November 21, 2017 4:45 AM
> To: Stefan Hajnoczi <stefanha@gmail.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; qemu-devel@nongnu.org;
> pbonzini@redhat.com; marcandre.lureau@redhat.com; felipe@nutanix.com;
> Harris, James R <james.r.harris@intel.com>
> Subject: Re: [PATCH v5 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On Mon, Nov 20, 2017 at 04:26:31PM +0000, Stefan Hajnoczi wrote:
> > On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> > > Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which
> can be
> > > used for live migration of vhost user devices, also vhost user devices
> > > can benefit from the messages to get/set virtio config space from/to the
> > > I/O target. For the purpose to support virtio config space change,
> > > VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> > > in case virtio config space change in the I/O target.
> > >
> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > ---
> > >  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
> > >  hw/virtio/vhost-user.c            | 98
> +++++++++++++++++++++++++++++++++++++++
> > >  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
> > >  include/hw/virtio/vhost-backend.h |  8 ++++
> > >  include/hw/virtio/vhost.h         | 16 +++++++
> > >  5 files changed, 224 insertions(+)
> > >
> > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > index 954771d..1b98388 100644
> > > --- a/docs/interop/vhost-user.txt
> > > +++ b/docs/interop/vhost-user.txt
> > > @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
> > >      - 3: IOTLB invalidate
> > >      - 4: IOTLB access fail
> > >
> > > + * Virtio device config space
> > > +   ---------------------------
> > > +   | offset | size | payload |
> > > +   ---------------------------
> > > +
> > > +   Offset: a 32-bit offset of virtio device's configuration space
> >
> > s/of/in the/
> >
> > > +   Size: a 32-bit size of configuration space that master wanted to change
> >
> > Is this also used for GET_CONFIG?  If yes, I suggest "a 32-bit
> > configuration space access size in bytes".
> >
> > Please mention that Size must be <= 256 bytes.
> >
> > > +   Payload: a 256-bytes array holding the contents of the virtio
> > > +       device's configuration space
> >
> > What about bytes outside the [offset, offset+size) range?  I guess they
> > must be 0 and are ignored by the master/slave.
> >
> > Would it be cleaner to make Payload a variable-sized field with Size
> > bytes?  That way it's not necessary to transfer 0s and memcpy() a subset
> > of the payload array.
> >
> > > +
> > >  In QEMU the vhost-user message is implemented with the following struct:
> > >
> > >  typedef struct VhostUserMsg {
> > > @@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
> > >          VhostUserMemory memory;
> > >          VhostUserLog log;
> > >          struct vhost_iotlb_msg iotlb;
> > > +        VhostUserConfig config;
> > >      };
> > >  } QEMU_PACKED VhostUserMsg;
> > >
> > > @@ -596,6 +607,34 @@ Master message types
> > >        and expect this message once (per VQ) during device configuration
> > >        (ie. before the master starts the VQ).
> > >
> > > + * VHOST_USER_GET_CONFIG
> > > +      Id: 24
> > > +      Equivalent ioctl: N/A
> > > +      Master payload: virtio device config space
> > > +
> > > +      Submitted by the vhost-user master to fetch the contents of the virtio
> > > +      device configuration space. The vhost-user master may cache the contents
> > > +      to avoid repeated VHOST_USER_GET_CONFIG calls.
> > > +
> > > +* VHOST_USER_SET_CONFIG
> > > +      Id: 25
> > > +      Equivalent ioctl: N/A
> > > +      Master payload: virtio device config space
> > > +
> > > +      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.
> >
> > There might be security issues if the vhost slave cannot tell whether
> > SET_CONFIG is coming from the guest driver or from the master process
> > (live migration).  Typically certain fields are read-only for the guest
> > driver.  Maybe those fields need to be set by the master after live
> > migration.
> >
> > One way to solve this is adding a flags field to the message.  A special
> > flag can be used for live migration so the slave knows that this
> > SET_CONFIG message is allowed to write to read-only fields.
> >
> > It's also worth documenting that slaves MUST NOT accept SET_CONFIG for
> > read-only configuration space fields unless the live migration bit is
> > set.  Hopefully this will remind implementors to think through the
> > security issues.
> 
> Live migrations is supposed to be migrating guest writeable state too.
> If you mean migrating RO fields like size, then
> I don't think it's a good idea to reuse SET_CONFIG for that.
> SET_CONFIG should obey exactly the virtio semantics.
> 
> And I agree, it should say that slave must treat it as a write,
> and get config as a read according to virtio semantics.
> 
> If someone needs to pass configuration from qemu to
> slave, let's add specific messages with precisely defined semantics.
> 
> Which reminds me.
> 
> virtio 1 changed endian-ness for config.
> 
> I think we should specify it's all virtio 1 format, and
> just disallow vhost blk for virtio 0 guests.
Will add the limitation to this txt file.
> 
> --
> MST

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@gmail.com]
> Sent: Tuesday, November 21, 2017 12:27 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com;
> marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R
> <james.r.harris@intel.com>
> Subject: Re: [PATCH v5 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> > Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which can
> be
> > used for live migration of vhost user devices, also vhost user devices
> > can benefit from the messages to get/set virtio config space from/to the
> > I/O target. For the purpose to support virtio config space change,
> > VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> > in case virtio config space change in the I/O target.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
> >  hw/virtio/vhost-user.c            | 98 +++++++++++++++++++++++++++++++++++++++
> >  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
> >  include/hw/virtio/vhost-backend.h |  8 ++++
> >  include/hw/virtio/vhost.h         | 16 +++++++
> >  5 files changed, 224 insertions(+)
> >
> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > index 954771d..1b98388 100644
> > --- a/docs/interop/vhost-user.txt
> > +++ b/docs/interop/vhost-user.txt
> > @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
> >      - 3: IOTLB invalidate
> >      - 4: IOTLB access fail
> >
> > + * Virtio device config space
> > +   ---------------------------
> > +   | offset | size | payload |
> > +   ---------------------------
> > +
> > +   Offset: a 32-bit offset of virtio device's configuration space
> 
> s/of/in the/
> 
> > +   Size: a 32-bit size of configuration space that master wanted to change
> 
> Is this also used for GET_CONFIG?  If yes, I suggest "a 32-bit
> configuration space access size in bytes".
ok.
> 
> Please mention that Size must be <= 256 bytes.
> 
> > +   Payload: a 256-bytes array holding the contents of the virtio
> > +       device's configuration space
> 
> What about bytes outside the [offset, offset+size) range?  I guess they
> must be 0 and are ignored by the master/slave.
> 
> Would it be cleaner to make Payload a variable-sized field with Size
> bytes?  That way it's not necessary to transfer 0s and memcpy() a subset
> of the payload array.
sounds good, but for vhost-blk driver it can call get_config for the whole virtio_blk_config space.
> 
> > +
> >  In QEMU the vhost-user message is implemented with the following struct:
> >
> >  typedef struct VhostUserMsg {
> > @@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
> >          VhostUserMemory memory;
> >          VhostUserLog log;
> >          struct vhost_iotlb_msg iotlb;
> > +        VhostUserConfig config;
> >      };
> >  } QEMU_PACKED VhostUserMsg;
> >
> > @@ -596,6 +607,34 @@ Master message types
> >        and expect this message once (per VQ) during device configuration
> >        (ie. before the master starts the VQ).
> >
> > + * VHOST_USER_GET_CONFIG
> > +      Id: 24
> > +      Equivalent ioctl: N/A
> > +      Master payload: virtio device config space
> > +
> > +      Submitted by the vhost-user master to fetch the contents of the virtio
> > +      device configuration space. The vhost-user master may cache the contents
> > +      to avoid repeated VHOST_USER_GET_CONFIG calls.
> > +
> > +* VHOST_USER_SET_CONFIG
> > +      Id: 25
> > +      Equivalent ioctl: N/A
> > +      Master payload: virtio device config space
> > +
> > +      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.
> 
> There might be security issues if the vhost slave cannot tell whether
> SET_CONFIG is coming from the guest driver or from the master process
> (live migration).  Typically certain fields are read-only for the guest
> driver.  Maybe those fields need to be set by the master after live
> migration.
> 
> One way to solve this is adding a flags field to the message.  A special
> flag can be used for live migration so the slave knows that this
> SET_CONFIG message is allowed to write to read-only fields.
> 
Ok.
> It's also worth documenting that slaves MUST NOT accept SET_CONFIG for
> read-only configuration space fields unless the live migration bit is
> set.  Hopefully this will remind implementors to think through the
> security issues.

On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which can be
> used for live migration of vhost user devices, also vhost user devices
> can benefit from the messages to get/set virtio config space from/to the
> I/O target. For the purpose to support virtio config space change,
> VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> in case virtio config space change in the I/O target.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
>  hw/virtio/vhost-user.c            | 98 +++++++++++++++++++++++++++++++++++++++
>  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  8 ++++
>  include/hw/virtio/vhost.h         | 16 +++++++
>  5 files changed, 224 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 954771d..1b98388 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
>      - 3: IOTLB invalidate
>      - 4: IOTLB access fail
>  
> + * Virtio device config space
> +   ---------------------------
> +   | offset | size | payload |
> +   ---------------------------
> +
> +   Offset: a 32-bit offset of virtio device's configuration space
> +   Size: a 32-bit size of configuration space that master wanted to change

I guess only legal values here are 1-256? But also see below. Also, we
already know the structure size.

> +   Payload: a 256-bytes array holding the contents of the virtio
> +       device's configuration space
> +

Why not *size* bytes? These are not performance critical but still,
why waste cycles.

>  In QEMU the vhost-user message is implemented with the following struct:
>  
>  typedef struct VhostUserMsg {

Virtio spec says:
	For device configuration access, the driver MUST use 8-bit wide accesses for 8-bit wide fields, 16-bit wide
	and aligned accesses for 16-bit wide fields and 32-bit wide and aligned accesses for 32-bit and 64-bit wide
	fields. For 64-bit fields, the driver MAY access each of the high and low 32-bit parts of the field independently.

So if these commands mirror guest accesses, they are always 1,2,4 or 8 bytes,
and aligned.


-- 
MST