| Message ID | 1513132143-10758-2-git-send-email-changpeng.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 13, 2017 at 10:29:00AM +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 | 45 ++++++++++++++++ > hw/virtio/vhost-user.c | 107 ++++++++++++++++++++++++++++++++++++++ > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > include/hw/virtio/vhost-backend.h | 14 +++++ > include/hw/virtio/vhost.h | 16 ++++++ > 5 files changed, 246 insertions(+) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Hi On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> 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. Why not use a new slave message instead of adding another fd to watch for? VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration space change > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > --- > docs/interop/vhost-user.txt | 45 ++++++++++++++++ > hw/virtio/vhost-user.c | 107 ++++++++++++++++++++++++++++++++++++++ > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > include/hw/virtio/vhost-backend.h | 14 +++++ > include/hw/virtio/vhost.h | 16 ++++++ > 5 files changed, 246 insertions(+) > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > index 954771d..826ba18 100644 > --- a/docs/interop/vhost-user.txt > +++ b/docs/interop/vhost-user.txt > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: > - 3: IOTLB invalidate > - 4: IOTLB access fail > > + * Virtio device config space > + ----------------------------------- > + | offset | size | flags | payload | > + ----------------------------------- > + > + Offset: a 32-bit offset of virtio device's configuration space > + Size: a 32-bit configuration space access size in bytes > + Flags: a 32-bit value: > + - 0: Vhost master messages used for writeable fields > + - 1: Vhost master messages used for live migration > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { > VhostUserMemory memory; > VhostUserLog log; > struct vhost_iotlb_msg iotlb; > + VhostUserConfig config; > }; > } QEMU_PACKED VhostUserMsg; > > @@ -596,6 +610,37 @@ Master message types > and expect this message once (per VQ) during device configuration > (ie. before the master starts the VQ). > > + * VHOST_USER_GET_CONFIG Please add an empty line (same style as other messages). Same for other messages. > + Id: 24 > + Equivalent ioctl: N/A > + Master payload: virtio device config space Why is the master sending the current config space? I suppose the content should be meaningful then. Make that explicit in the spec please + missing Slave payload (make it explicit that size must match) > + > + Submitted by the vhost-user master to fetch the contents of the virtio > + device configuration space, vhost-user slave uses zero length of payload > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags > + field, and slaves MUST NOT accept SET_CONFIG for read-only > + configuration space fields unless the live migration bit is set. I would make reply mandatory for any newly introduced message. If not, please add the message to the list that don't in "Communication" > + > +* 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. Imho there should be a slave message instead. > + > Slave message types > ------------------- > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { > uint64_t mmap_offset; > } VhostUserLog; > > +typedef struct VhostUserConfig { > + uint32_t offset; > + uint32_t size; > + uint32_t flags; > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; > +} VhostUserConfig; > + > +static VhostUserConfig c __attribute__ ((unused)); > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ > + + sizeof(c.size) \ > + + sizeof(c.flags)) > + > typedef struct VhostUserMsg { > VhostUserRequest request; > > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { > VhostUserMemory memory; > VhostUserLog log; > struct vhost_iotlb_msg iotlb; > + VhostUserConfig config; > } payload; > } QEMU_PACKED VhostUserMsg; > > @@ -922,6 +943,89 @@ 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, > + uint32_t config_len) > +{ > + VhostUserMsg msg = { > + msg.request = VHOST_USER_GET_CONFIG, > + msg.flags = VHOST_USER_VERSION, > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, > + }; > + > + msg.payload.config.offset = 0; > + msg.payload.config.size = config_len; > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a stack overflow. Add an assert() ? > + > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { > + 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 *data, > + uint32_t offset, uint32_t size, uint32_t flags) > +{ > + uint8_t *p; > + bool reply_supported = virtio_has_feature(dev->protocol_features, > + VHOST_USER_PROTOCOL_F_REPLY_ACK); > + > + VhostUserMsg msg = { > + msg.request = VHOST_USER_SET_CONFIG, > + msg.flags = VHOST_USER_VERSION, > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, > + }; > + > + if (reply_supported) { > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > + } > + > + msg.payload.config.offset = offset, > + msg.payload.config.size = size, > + msg.payload.config.flags = flags, > + p = msg.payload.config.region; > + memcpy(p, data, size); here again, max config size should be checked. > + > + 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 +1052,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 e4290ce..aa93398 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1358,6 +1358,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); > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, > } > } > > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, > + uint32_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 -1; > +} > + > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, > + uint32_t offset, uint32_t size, uint32_t flags) > +{ > + assert(hdev->vhost_ops); > + > + if (hdev->vhost_ops->vhost_set_config) { > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, > + size, flags); > + } > + > + return -1; > +} > + > +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 -1; > +} > + > /* 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..8996d5f 100644 > --- a/include/hw/virtio/vhost-backend.h > +++ b/include/hw/virtio/vhost-backend.h > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { > VHOST_BACKEND_TYPE_MAX = 3, > } VhostBackendType; > > +typedef enum VhostSetConfigType { > + VHOST_SET_CONFIG_TYPE_MASTER = 0, > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, > +} VhostSetConfigType; > + > struct vhost_dev; > struct vhost_log; > struct vhost_memory; > @@ -84,6 +89,12 @@ 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 *data, > + uint32_t offset, uint32_t size, > + uint32_t flags); > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, > + uint32_t config_len); > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); > > typedef struct VhostOps { > VhostBackendType backend_type; > @@ -118,6 +129,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..7f43a82 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, > + uint32_t config_len); > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, > + uint32_t offset, uint32_t size, uint32_t flags); > +/* 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 > >
Michael, did you merge that one too? I think the series shouldn't be applied yet until my concerns are cleared. Thanks Le mer. 20 déc. 2017 à 16:47, Marc-André Lureau <marcandre.lureau@gmail.com> a écrit : > Hi > > On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> > 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. > > Why not use a new slave message instead of adding another fd to watch for? > > VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration > space change > > > > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > > --- > > docs/interop/vhost-user.txt | 45 ++++++++++++++++ > > hw/virtio/vhost-user.c | 107 > ++++++++++++++++++++++++++++++++++++++ > > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > > include/hw/virtio/vhost-backend.h | 14 +++++ > > include/hw/virtio/vhost.h | 16 ++++++ > > 5 files changed, 246 insertions(+) > > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > index 954771d..826ba18 100644 > > --- a/docs/interop/vhost-user.txt > > +++ b/docs/interop/vhost-user.txt > > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: > > - 3: IOTLB invalidate > > - 4: IOTLB access fail > > > > + * Virtio device config space > > + ----------------------------------- > > + | offset | size | flags | payload | > > + ----------------------------------- > > + > > + Offset: a 32-bit offset of virtio device's configuration space > > + Size: a 32-bit configuration space access size in bytes > > + Flags: a 32-bit value: > > + - 0: Vhost master messages used for writeable fields > > + - 1: Vhost master messages used for live migration > > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > }; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -596,6 +610,37 @@ Master message types > > and expect this message once (per VQ) during device configuration > > (ie. before the master starts the VQ). > > > > + * VHOST_USER_GET_CONFIG > > Please add an empty line (same style as other messages). Same for > other messages. > > > + Id: 24 > > + Equivalent ioctl: N/A > > + Master payload: virtio device config space > > Why is the master sending the current config space? I suppose the > content should be meaningful then. Make that explicit in the spec > please > > + missing Slave payload (make it explicit that size must match) > > > + > > + Submitted by the vhost-user master to fetch the contents of the > virtio > > + device configuration space, vhost-user slave uses zero length of > payload > > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags > > + field, and slaves MUST NOT accept SET_CONFIG for read-only > > + configuration space fields unless the live migration bit is set. > > I would make reply mandatory for any newly introduced message. If not, > please add the message to the list that don't in "Communication" > > > + > > +* 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. > > Imho there should be a slave message instead. > > > + > > Slave message types > > ------------------- > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { > > uint64_t mmap_offset; > > } VhostUserLog; > > > > +typedef struct VhostUserConfig { > > + uint32_t offset; > > + uint32_t size; > > + uint32_t flags; > > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; > > +} VhostUserConfig; > > + > > +static VhostUserConfig c __attribute__ ((unused)); > > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ > > + + sizeof(c.size) \ > > + + sizeof(c.flags)) > > + > > typedef struct VhostUserMsg { > > VhostUserRequest request; > > > > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > } payload; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -922,6 +943,89 @@ 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, > > + uint32_t config_len) > > +{ > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_GET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, > > + }; > > + > > + msg.payload.config.offset = 0; > > + msg.payload.config.size = config_len; > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a > stack overflow. Add an assert() ? > > > + > > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { > > + 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 > *data, > > + uint32_t offset, uint32_t size, > uint32_t flags) > > +{ > > + uint8_t *p; > > + bool reply_supported = virtio_has_feature(dev->protocol_features, > > + > VHOST_USER_PROTOCOL_F_REPLY_ACK); > > + > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_SET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, > > + }; > > + > > + if (reply_supported) { > > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > > + } > > + > > + msg.payload.config.offset = offset, > > + msg.payload.config.size = size, > > + msg.payload.config.flags = flags, > > + p = msg.payload.config.region; > > + memcpy(p, data, size); > > here again, max config size should be checked. > > > + > > + 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 +1052,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 e4290ce..aa93398 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1358,6 +1358,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); > > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, > const int *feature_bits, > > } > > } > > > > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, > > + uint32_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 -1; > > +} > > + > > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t flags) > > +{ > > + assert(hdev->vhost_ops); > > + > > + if (hdev->vhost_ops->vhost_set_config) { > > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, > > + size, flags); > > + } > > + > > + return -1; > > +} > > + > > +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 -1; > > +} > > + > > /* 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..8996d5f 100644 > > --- a/include/hw/virtio/vhost-backend.h > > +++ b/include/hw/virtio/vhost-backend.h > > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { > > VHOST_BACKEND_TYPE_MAX = 3, > > } VhostBackendType; > > > > +typedef enum VhostSetConfigType { > > + VHOST_SET_CONFIG_TYPE_MASTER = 0, > > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, > > +} VhostSetConfigType; > > + > > struct vhost_dev; > > struct vhost_log; > > struct vhost_memory; > > @@ -84,6 +89,12 @@ 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 > *data, > > + uint32_t offset, uint32_t size, > > + uint32_t flags); > > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t > *config, > > + uint32_t config_len); > > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); > > > > typedef struct VhostOps { > > VhostBackendType backend_type; > > @@ -118,6 +129,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..7f43a82 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, > > + uint32_t config_len); > > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t > flags); > > +/* 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 > > > > > > > > -- > Marc-André Lureau >
On Wed, Dec 20, 2017 at 06:28:04PM +0000, Marc-André Lureau wrote: > Michael, did you merge that one too? I think the series shouldn't be applied > yet until my concerns are cleared. Thanks OK I'll drop these for now. > Le mer. 20 déc. 2017 à 16:47, Marc-André Lureau <marcandre.lureau@gmail.com> a > écrit : > > Hi > > On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> > 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. > > Why not use a new slave message instead of adding another fd to watch for? > > VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration > space change > > > > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > > --- > > docs/interop/vhost-user.txt | 45 ++++++++++++++++ > > hw/virtio/vhost-user.c | 107 > ++++++++++++++++++++++++++++++++++++++ > > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > > include/hw/virtio/vhost-backend.h | 14 +++++ > > include/hw/virtio/vhost.h | 16 ++++++ > > 5 files changed, 246 insertions(+) > > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > index 954771d..826ba18 100644 > > --- a/docs/interop/vhost-user.txt > > +++ b/docs/interop/vhost-user.txt > > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: > > - 3: IOTLB invalidate > > - 4: IOTLB access fail > > > > + * Virtio device config space > > + ----------------------------------- > > + | offset | size | flags | payload | > > + ----------------------------------- > > + > > + Offset: a 32-bit offset of virtio device's configuration space > > + Size: a 32-bit configuration space access size in bytes > > + Flags: a 32-bit value: > > + - 0: Vhost master messages used for writeable fields > > + - 1: Vhost master messages used for live migration > > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > }; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -596,6 +610,37 @@ Master message types > > and expect this message once (per VQ) during device configuration > > (ie. before the master starts the VQ). > > > > + * VHOST_USER_GET_CONFIG > > Please add an empty line (same style as other messages). Same for > other messages. > > > + Id: 24 > > + Equivalent ioctl: N/A > > + Master payload: virtio device config space > > Why is the master sending the current config space? I suppose the > content should be meaningful then. Make that explicit in the spec > please > > + missing Slave payload (make it explicit that size must match) > > > + > > + Submitted by the vhost-user master to fetch the contents of the > virtio > > + device configuration space, vhost-user slave uses zero length of > payload > > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags > > + field, and slaves MUST NOT accept SET_CONFIG for read-only > > + configuration space fields unless the live migration bit is set. > > I would make reply mandatory for any newly introduced message. If not, > please add the message to the list that don't in "Communication" > > > + > > +* 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. > > Imho there should be a slave message instead. > > > + > > Slave message types > > ------------------- > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { > > uint64_t mmap_offset; > > } VhostUserLog; > > > > +typedef struct VhostUserConfig { > > + uint32_t offset; > > + uint32_t size; > > + uint32_t flags; > > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; > > +} VhostUserConfig; > > + > > +static VhostUserConfig c __attribute__ ((unused)); > > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ > > + + sizeof(c.size) \ > > + + sizeof(c.flags)) > > + > > typedef struct VhostUserMsg { > > VhostUserRequest request; > > > > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > } payload; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -922,6 +943,89 @@ 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, > > + uint32_t config_len) > > +{ > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_GET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, > > + }; > > + > > + msg.payload.config.offset = 0; > > + msg.payload.config.size = config_len; > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a > stack overflow. Add an assert() ? > > > + > > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { > > + 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 > *data, > > + uint32_t offset, uint32_t size, > uint32_t flags) > > +{ > > + uint8_t *p; > > + bool reply_supported = virtio_has_feature(dev->protocol_features, > > + > VHOST_USER_PROTOCOL_F_REPLY_ACK); > > + > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_SET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, > > + }; > > + > > + if (reply_supported) { > > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > > + } > > + > > + msg.payload.config.offset = offset, > > + msg.payload.config.size = size, > > + msg.payload.config.flags = flags, > > + p = msg.payload.config.region; > > + memcpy(p, data, size); > > here again, max config size should be checked. > > > + > > + 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 +1052,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 e4290ce..aa93398 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1358,6 +1358,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); > > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, > const int *feature_bits, > > } > > } > > > > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, > > + uint32_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 -1; > > +} > > + > > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t flags) > > +{ > > + assert(hdev->vhost_ops); > > + > > + if (hdev->vhost_ops->vhost_set_config) { > > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, > > + size, flags); > > + } > > + > > + return -1; > > +} > > + > > +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 -1; > > +} > > + > > /* 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..8996d5f 100644 > > --- a/include/hw/virtio/vhost-backend.h > > +++ b/include/hw/virtio/vhost-backend.h > > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { > > VHOST_BACKEND_TYPE_MAX = 3, > > } VhostBackendType; > > > > +typedef enum VhostSetConfigType { > > + VHOST_SET_CONFIG_TYPE_MASTER = 0, > > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, > > +} VhostSetConfigType; > > + > > struct vhost_dev; > > struct vhost_log; > > struct vhost_memory; > > @@ -84,6 +89,12 @@ 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 > *data, > > + uint32_t offset, uint32_t size, > > + uint32_t flags); > > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t > *config, > > + uint32_t config_len); > > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); > > > > typedef struct VhostOps { > > VhostBackendType backend_type; > > @@ -118,6 +129,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..7f43a82 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, > > + uint32_t config_len); > > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t > flags); > > +/* 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 > > > > > > > > -- > Marc-André Lureau > > -- > Marc-André Lureau
On Wed, Dec 20, 2017 at 04:47:13PM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> 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. > > Why not use a new slave message instead of adding another fd to watch for? > > VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration > space change Well that's a nice idea, but at v7 I'd expect it we are past such fundamental suggestions. In particular Stefan wanted to require that slave is non-blocking, and it's quite hard to support for the existing channel. It's up to the contributor whether to make this change, I'm fine either way. Concerns such as overflow possibility below need to be addressed. > > > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > > --- > > docs/interop/vhost-user.txt | 45 ++++++++++++++++ > > hw/virtio/vhost-user.c | 107 ++++++++++++++++++++++++++++++++++++++ > > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > > include/hw/virtio/vhost-backend.h | 14 +++++ > > include/hw/virtio/vhost.h | 16 ++++++ > > 5 files changed, 246 insertions(+) > > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > index 954771d..826ba18 100644 > > --- a/docs/interop/vhost-user.txt > > +++ b/docs/interop/vhost-user.txt > > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: > > - 3: IOTLB invalidate > > - 4: IOTLB access fail > > > > + * Virtio device config space > > + ----------------------------------- > > + | offset | size | flags | payload | > > + ----------------------------------- > > + > > + Offset: a 32-bit offset of virtio device's configuration space > > + Size: a 32-bit configuration space access size in bytes > > + Flags: a 32-bit value: > > + - 0: Vhost master messages used for writeable fields > > + - 1: Vhost master messages used for live migration > > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > }; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -596,6 +610,37 @@ Master message types > > and expect this message once (per VQ) during device configuration > > (ie. before the master starts the VQ). > > > > + * VHOST_USER_GET_CONFIG > > Please add an empty line (same style as other messages). Same for > other messages. > > > + Id: 24 > > + Equivalent ioctl: N/A > > + Master payload: virtio device config space > > Why is the master sending the current config space? I suppose the > content should be meaningful then. Make that explicit in the spec > please > > + missing Slave payload (make it explicit that size must match) > > > + > > + Submitted by the vhost-user master to fetch the contents of the virtio > > + device configuration space, vhost-user slave uses zero length of payload > > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags > > + field, and slaves MUST NOT accept SET_CONFIG for read-only > > + configuration space fields unless the live migration bit is set. > > I would make reply mandatory for any newly introduced message. If not, > please add the message to the list that don't in "Communication" > > > + > > +* 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. > > Imho there should be a slave message instead. > > > + > > Slave message types > > ------------------- > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { > > uint64_t mmap_offset; > > } VhostUserLog; > > > > +typedef struct VhostUserConfig { > > + uint32_t offset; > > + uint32_t size; > > + uint32_t flags; > > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; > > +} VhostUserConfig; > > + > > +static VhostUserConfig c __attribute__ ((unused)); > > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ > > + + sizeof(c.size) \ > > + + sizeof(c.flags)) > > + > > typedef struct VhostUserMsg { > > VhostUserRequest request; > > > > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { > > VhostUserMemory memory; > > VhostUserLog log; > > struct vhost_iotlb_msg iotlb; > > + VhostUserConfig config; > > } payload; > > } QEMU_PACKED VhostUserMsg; > > > > @@ -922,6 +943,89 @@ 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, > > + uint32_t config_len) > > +{ > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_GET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, > > + }; > > + > > + msg.payload.config.offset = 0; > > + msg.payload.config.size = config_len; > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a > stack overflow. Add an assert() ? assert isn't a good way to handle failures. E.g. device could be added by hotplug. How about disconnecting and failing device creation? > > + > > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { > > + 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 *data, > > + uint32_t offset, uint32_t size, uint32_t flags) > > +{ > > + uint8_t *p; > > + bool reply_supported = virtio_has_feature(dev->protocol_features, > > + VHOST_USER_PROTOCOL_F_REPLY_ACK); > > + > > + VhostUserMsg msg = { > > + msg.request = VHOST_USER_SET_CONFIG, > > + msg.flags = VHOST_USER_VERSION, > > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, > > + }; > > + > > + if (reply_supported) { > > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > > + } > > + > > + msg.payload.config.offset = offset, > > + msg.payload.config.size = size, > > + msg.payload.config.flags = flags, > > + p = msg.payload.config.region; > > + memcpy(p, data, size); > > here again, max config size should be checked. > > > + > > + 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 +1052,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 e4290ce..aa93398 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1358,6 +1358,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); > > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, > > } > > } > > > > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, > > + uint32_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 -1; > > +} > > + > > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t flags) > > +{ > > + assert(hdev->vhost_ops); > > + > > + if (hdev->vhost_ops->vhost_set_config) { > > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, > > + size, flags); > > + } > > + > > + return -1; > > +} > > + > > +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 -1; > > +} > > + > > /* 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..8996d5f 100644 > > --- a/include/hw/virtio/vhost-backend.h > > +++ b/include/hw/virtio/vhost-backend.h > > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { > > VHOST_BACKEND_TYPE_MAX = 3, > > } VhostBackendType; > > > > +typedef enum VhostSetConfigType { > > + VHOST_SET_CONFIG_TYPE_MASTER = 0, > > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, > > +} VhostSetConfigType; > > + > > struct vhost_dev; > > struct vhost_log; > > struct vhost_memory; > > @@ -84,6 +89,12 @@ 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 *data, > > + uint32_t offset, uint32_t size, > > + uint32_t flags); > > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, > > + uint32_t config_len); > > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); > > > > typedef struct VhostOps { > > VhostBackendType backend_type; > > @@ -118,6 +129,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..7f43a82 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, > > + uint32_t config_len); > > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t flags); > > +/* 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 > > > > > > > > -- > Marc-André Lureau
Hi On Thu, Dec 21, 2017 at 1:21 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Dec 20, 2017 at 04:47:13PM +0100, Marc-André Lureau wrote: >> Hi >> >> On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> 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. >> >> Why not use a new slave message instead of adding another fd to watch for? >> >> VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration >> space change > > > Well that's a nice idea, but at v7 I'd expect it we are past such > fundamental suggestions. I am sorry, I was quite busy with other work. (that happen to all of us). Hopefully, we still have some time for development for this release. Eventually, we could make this improvement on top during this cycle, but I would rather have protocol changes agreed before they hit master. > > In particular Stefan wanted to require that slave is non-blocking, > and it's quite hard to support for the existing channel. The notification could be non blocking, but the messages will go on the same channel. The slave channel is actually not so busy and will require a single message (instead of notify + master get with current proposal) Adding a slave channel message should also be easier than adding another event fd (both in master and slave). > > It's up to the contributor whether to make this change, I'm fine > either way. > > Concerns such as overflow possibility below need to be addressed. thanks > >> >> > >> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> >> > --- >> > docs/interop/vhost-user.txt | 45 ++++++++++++++++ >> > hw/virtio/vhost-user.c | 107 ++++++++++++++++++++++++++++++++++++++ >> > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ >> > include/hw/virtio/vhost-backend.h | 14 +++++ >> > include/hw/virtio/vhost.h | 16 ++++++ >> > 5 files changed, 246 insertions(+) >> > >> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt >> > index 954771d..826ba18 100644 >> > --- a/docs/interop/vhost-user.txt >> > +++ b/docs/interop/vhost-user.txt >> > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: >> > - 3: IOTLB invalidate >> > - 4: IOTLB access fail >> > >> > + * Virtio device config space >> > + ----------------------------------- >> > + | offset | size | flags | payload | >> > + ----------------------------------- >> > + >> > + Offset: a 32-bit offset of virtio device's configuration space >> > + Size: a 32-bit configuration space access size in bytes >> > + Flags: a 32-bit value: >> > + - 0: Vhost master messages used for writeable fields >> > + - 1: Vhost master messages used for live migration >> > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { >> > VhostUserMemory memory; >> > VhostUserLog log; >> > struct vhost_iotlb_msg iotlb; >> > + VhostUserConfig config; >> > }; >> > } QEMU_PACKED VhostUserMsg; >> > >> > @@ -596,6 +610,37 @@ Master message types >> > and expect this message once (per VQ) during device configuration >> > (ie. before the master starts the VQ). >> > >> > + * VHOST_USER_GET_CONFIG >> >> Please add an empty line (same style as other messages). Same for >> other messages. >> >> > + Id: 24 >> > + Equivalent ioctl: N/A >> > + Master payload: virtio device config space >> >> Why is the master sending the current config space? I suppose the >> content should be meaningful then. Make that explicit in the spec >> please >> >> + missing Slave payload (make it explicit that size must match) >> >> > + >> > + Submitted by the vhost-user master to fetch the contents of the virtio >> > + device configuration space, vhost-user slave uses zero length of payload >> > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags >> > + field, and slaves MUST NOT accept SET_CONFIG for read-only >> > + configuration space fields unless the live migration bit is set. >> >> I would make reply mandatory for any newly introduced message. If not, >> please add the message to the list that don't in "Communication" >> >> > + >> > +* 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. >> >> Imho there should be a slave message instead. >> >> > + >> > Slave message types >> > ------------------- >> > >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >> > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { >> > uint64_t mmap_offset; >> > } VhostUserLog; >> > >> > +typedef struct VhostUserConfig { >> > + uint32_t offset; >> > + uint32_t size; >> > + uint32_t flags; >> > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; >> > +} VhostUserConfig; >> > + >> > +static VhostUserConfig c __attribute__ ((unused)); >> > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ >> > + + sizeof(c.size) \ >> > + + sizeof(c.flags)) >> > + >> > typedef struct VhostUserMsg { >> > VhostUserRequest request; >> > >> > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { >> > VhostUserMemory memory; >> > VhostUserLog log; >> > struct vhost_iotlb_msg iotlb; >> > + VhostUserConfig config; >> > } payload; >> > } QEMU_PACKED VhostUserMsg; >> > >> > @@ -922,6 +943,89 @@ 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, >> > + uint32_t config_len) >> > +{ >> > + VhostUserMsg msg = { >> > + msg.request = VHOST_USER_GET_CONFIG, >> > + msg.flags = VHOST_USER_VERSION, >> > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, >> > + }; >> > + >> > + msg.payload.config.offset = 0; >> > + msg.payload.config.size = config_len; >> > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { >> > + return -1; >> > + } >> >> if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a >> stack overflow. Add an assert() ? > > assert isn't a good way to handle failures. E.g. device could be added > by hotplug. How about disconnecting and failing device creation? > >> > + >> > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { >> > + 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 *data, >> > + uint32_t offset, uint32_t size, uint32_t flags) >> > +{ >> > + uint8_t *p; >> > + bool reply_supported = virtio_has_feature(dev->protocol_features, >> > + VHOST_USER_PROTOCOL_F_REPLY_ACK); >> > + >> > + VhostUserMsg msg = { >> > + msg.request = VHOST_USER_SET_CONFIG, >> > + msg.flags = VHOST_USER_VERSION, >> > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, >> > + }; >> > + >> > + if (reply_supported) { >> > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; >> > + } >> > + >> > + msg.payload.config.offset = offset, >> > + msg.payload.config.size = size, >> > + msg.payload.config.flags = flags, >> > + p = msg.payload.config.region; >> > + memcpy(p, data, size); >> >> here again, max config size should be checked. >> >> > + >> > + 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 +1052,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 e4290ce..aa93398 100644 >> > --- a/hw/virtio/vhost.c >> > +++ b/hw/virtio/vhost.c >> > @@ -1358,6 +1358,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); >> > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, >> > } >> > } >> > >> > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, >> > + uint32_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 -1; >> > +} >> > + >> > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, >> > + uint32_t offset, uint32_t size, uint32_t flags) >> > +{ >> > + assert(hdev->vhost_ops); >> > + >> > + if (hdev->vhost_ops->vhost_set_config) { >> > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, >> > + size, flags); >> > + } >> > + >> > + return -1; >> > +} >> > + >> > +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 -1; >> > +} >> > + >> > /* 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..8996d5f 100644 >> > --- a/include/hw/virtio/vhost-backend.h >> > +++ b/include/hw/virtio/vhost-backend.h >> > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { >> > VHOST_BACKEND_TYPE_MAX = 3, >> > } VhostBackendType; >> > >> > +typedef enum VhostSetConfigType { >> > + VHOST_SET_CONFIG_TYPE_MASTER = 0, >> > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, >> > +} VhostSetConfigType; >> > + >> > struct vhost_dev; >> > struct vhost_log; >> > struct vhost_memory; >> > @@ -84,6 +89,12 @@ 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 *data, >> > + uint32_t offset, uint32_t size, >> > + uint32_t flags); >> > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, >> > + uint32_t config_len); >> > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); >> > >> > typedef struct VhostOps { >> > VhostBackendType backend_type; >> > @@ -118,6 +129,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..7f43a82 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, >> > + uint32_t config_len); >> > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, >> > + uint32_t offset, uint32_t size, uint32_t flags); >> > +/* 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 >> > >> > >> >> >> >> -- >> Marc-André Lureau
> -----Original Message----- > From: Marc-André Lureau [mailto:marcandre.lureau@gmail.com] > Sent: Thursday, December 21, 2017 6:48 PM > To: Michael S. Tsirkin <mst@redhat.com> > Cc: Liu, Changpeng <changpeng.liu@intel.com>; QEMU <qemu- > devel@nongnu.org>; Harris, James R <james.r.harris@intel.com>; Stefan Hajnoczi > <stefanha@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Felipe Franciosi > <felipe@nutanix.com> > Subject: Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages > to support virtio config space > > Hi > > On Thu, Dec 21, 2017 at 1:21 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Dec 20, 2017 at 04:47:13PM +0100, Marc-André Lureau wrote: > >> Hi > >> > >> On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> > 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. > >> > >> Why not use a new slave message instead of adding another fd to watch for? > >> > >> VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration > >> space change > > > > > > Well that's a nice idea, but at v7 I'd expect it we are past such > > fundamental suggestions. > > I am sorry, I was quite busy with other work. (that happen to all of > us). Hopefully, we still have some time for development for this > release. Eventually, we could make this improvement on top during this > cycle, but I would rather have protocol changes agreed before they hit > master. I can replace the event file descriptor for configuration space with a new slave message, since the patch set was first developed based on QEMU 2.8, and the vhost slave channel didn't support at that time, currently add a new slave message also make sense. I will send v8 to cover this change and the comments you mentioned. Is everyone agreed that use a slave channel message about virtio configuration changes? > > > > > In particular Stefan wanted to require that slave is non-blocking, > > and it's quite hard to support for the existing channel. > > The notification could be non blocking, but the messages will go on > the same channel. > > The slave channel is actually not so busy and will require a single > message (instead of notify + master get with current proposal) > > Adding a slave channel message should also be easier than adding > another event fd (both in master and slave). > > > > > It's up to the contributor whether to make this change, I'm fine > > either way. > > > > Concerns such as overflow possibility below need to be addressed. > > thanks > > > > >> > >> > > >> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > >> > --- > >> > docs/interop/vhost-user.txt | 45 ++++++++++++++++ > >> > hw/virtio/vhost-user.c | 107 > ++++++++++++++++++++++++++++++++++++++ > >> > hw/virtio/vhost.c | 64 +++++++++++++++++++++++ > >> > include/hw/virtio/vhost-backend.h | 14 +++++ > >> > include/hw/virtio/vhost.h | 16 ++++++ > >> > 5 files changed, 246 insertions(+) > >> > > >> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > >> > index 954771d..826ba18 100644 > >> > --- a/docs/interop/vhost-user.txt > >> > +++ b/docs/interop/vhost-user.txt > >> > @@ -116,6 +116,19 @@ Depending on the request type, payload can be: > >> > - 3: IOTLB invalidate > >> > - 4: IOTLB access fail > >> > > >> > + * Virtio device config space > >> > + ----------------------------------- > >> > + | offset | size | flags | payload | > >> > + ----------------------------------- > >> > + > >> > + Offset: a 32-bit offset of virtio device's configuration space > >> > + Size: a 32-bit configuration space access size in bytes > >> > + Flags: a 32-bit value: > >> > + - 0: Vhost master messages used for writeable fields > >> > + - 1: Vhost master messages used for live migration > >> > + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { > >> > VhostUserMemory memory; > >> > VhostUserLog log; > >> > struct vhost_iotlb_msg iotlb; > >> > + VhostUserConfig config; > >> > }; > >> > } QEMU_PACKED VhostUserMsg; > >> > > >> > @@ -596,6 +610,37 @@ Master message types > >> > and expect this message once (per VQ) during device configuration > >> > (ie. before the master starts the VQ). > >> > > >> > + * VHOST_USER_GET_CONFIG > >> > >> Please add an empty line (same style as other messages). Same for > >> other messages. > >> > >> > + Id: 24 > >> > + Equivalent ioctl: N/A > >> > + Master payload: virtio device config space > >> > >> Why is the master sending the current config space? I suppose the > >> content should be meaningful then. Make that explicit in the spec > >> please > >> > >> + missing Slave payload (make it explicit that size must match) > >> > >> > + > >> > + Submitted by the vhost-user master to fetch the contents of the virtio > >> > + device configuration space, vhost-user slave uses zero length of payload > >> > + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags > >> > + field, and slaves MUST NOT accept SET_CONFIG for read-only > >> > + configuration space fields unless the live migration bit is set. > >> > >> I would make reply mandatory for any newly introduced message. If not, > >> please add the message to the list that don't in "Communication" > >> > >> > + > >> > +* 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. > >> > >> Imho there should be a slave message instead. > >> > >> > + > >> > Slave message types > >> > ------------------- > >> > > >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >> > index 093675e..037c165 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,18 @@ typedef struct VhostUserLog { > >> > uint64_t mmap_offset; > >> > } VhostUserLog; > >> > > >> > +typedef struct VhostUserConfig { > >> > + uint32_t offset; > >> > + uint32_t size; > >> > + uint32_t flags; > >> > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; > >> > +} VhostUserConfig; > >> > + > >> > +static VhostUserConfig c __attribute__ ((unused)); > >> > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ > >> > + + sizeof(c.size) \ > >> > + + sizeof(c.flags)) > >> > + > >> > typedef struct VhostUserMsg { > >> > VhostUserRequest request; > >> > > >> > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { > >> > VhostUserMemory memory; > >> > VhostUserLog log; > >> > struct vhost_iotlb_msg iotlb; > >> > + VhostUserConfig config; > >> > } payload; > >> > } QEMU_PACKED VhostUserMsg; > >> > > >> > @@ -922,6 +943,89 @@ 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, > >> > + uint32_t config_len) > >> > +{ > >> > + VhostUserMsg msg = { > >> > + msg.request = VHOST_USER_GET_CONFIG, > >> > + msg.flags = VHOST_USER_VERSION, > >> > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, > >> > + }; > >> > + > >> > + msg.payload.config.offset = 0; > >> > + msg.payload.config.size = config_len; > >> > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > >> > + return -1; > >> > + } > >> > >> if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a > >> stack overflow. Add an assert() ? > > > > assert isn't a good way to handle failures. E.g. device could be added > > by hotplug. How about disconnecting and failing device creation? > > > >> > + > >> > + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { > >> > + 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 *data, > >> > + uint32_t offset, uint32_t size, uint32_t flags) > >> > +{ > >> > + uint8_t *p; > >> > + bool reply_supported = virtio_has_feature(dev->protocol_features, > >> > + VHOST_USER_PROTOCOL_F_REPLY_ACK); > >> > + > >> > + VhostUserMsg msg = { > >> > + msg.request = VHOST_USER_SET_CONFIG, > >> > + msg.flags = VHOST_USER_VERSION, > >> > + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, > >> > + }; > >> > + > >> > + if (reply_supported) { > >> > + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > >> > + } > >> > + > >> > + msg.payload.config.offset = offset, > >> > + msg.payload.config.size = size, > >> > + msg.payload.config.flags = flags, > >> > + p = msg.payload.config.region; > >> > + memcpy(p, data, size); > >> > >> here again, max config size should be checked. > >> > >> > + > >> > + 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 +1052,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 e4290ce..aa93398 100644 > >> > --- a/hw/virtio/vhost.c > >> > +++ b/hw/virtio/vhost.c > >> > @@ -1358,6 +1358,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); > >> > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, > const int *feature_bits, > >> > } > >> > } > >> > > >> > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, > >> > + uint32_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 -1; > >> > +} > >> > + > >> > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, > >> > + uint32_t offset, uint32_t size, uint32_t flags) > >> > +{ > >> > + assert(hdev->vhost_ops); > >> > + > >> > + if (hdev->vhost_ops->vhost_set_config) { > >> > + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, > >> > + size, flags); > >> > + } > >> > + > >> > + return -1; > >> > +} > >> > + > >> > +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 -1; > >> > +} > >> > + > >> > /* 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..8996d5f 100644 > >> > --- a/include/hw/virtio/vhost-backend.h > >> > +++ b/include/hw/virtio/vhost-backend.h > >> > @@ -20,6 +20,11 @@ typedef enum VhostBackendType { > >> > VHOST_BACKEND_TYPE_MAX = 3, > >> > } VhostBackendType; > >> > > >> > +typedef enum VhostSetConfigType { > >> > + VHOST_SET_CONFIG_TYPE_MASTER = 0, > >> > + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, > >> > +} VhostSetConfigType; > >> > + > >> > struct vhost_dev; > >> > struct vhost_log; > >> > struct vhost_memory; > >> > @@ -84,6 +89,12 @@ 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 > *data, > >> > + uint32_t offset, uint32_t size, > >> > + uint32_t flags); > >> > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, > >> > + uint32_t config_len); > >> > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); > >> > > >> > typedef struct VhostOps { > >> > VhostBackendType backend_type; > >> > @@ -118,6 +129,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..7f43a82 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, > >> > + uint32_t config_len); > >> > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, > >> > + uint32_t offset, uint32_t size, uint32_t flags); > >> > +/* 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 > >> > > >> > > >> > >> > >> > >> -- > >> Marc-André Lureau > > > > -- > Marc-André Lureau
diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 954771d..826ba18 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -116,6 +116,19 @@ Depending on the request type, payload can be: - 3: IOTLB invalidate - 4: IOTLB access fail + * Virtio device config space + ----------------------------------- + | offset | size | flags | payload | + ----------------------------------- + + Offset: a 32-bit offset of virtio device's configuration space + Size: a 32-bit configuration space access size in bytes + Flags: a 32-bit value: + - 0: Vhost master messages used for writeable fields + - 1: Vhost master messages used for live migration + Payload: Size 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 +142,7 @@ typedef struct VhostUserMsg { VhostUserMemory memory; VhostUserLog log; struct vhost_iotlb_msg iotlb; + VhostUserConfig config; }; } QEMU_PACKED VhostUserMsg; @@ -596,6 +610,37 @@ 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, vhost-user slave uses zero length of payload + to indicate an error to vhost-user master. 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. The vhost-user slave must check the flags + field, and slaves MUST NOT accept SET_CONFIG for read-only + configuration space fields unless the live migration bit is set. + +* 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..037c165 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,18 @@ typedef struct VhostUserLog { uint64_t mmap_offset; } VhostUserLog; +typedef struct VhostUserConfig { + uint32_t offset; + uint32_t size; + uint32_t flags; + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; +} VhostUserConfig; + +static VhostUserConfig c __attribute__ ((unused)); +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \ + + sizeof(c.size) \ + + sizeof(c.flags)) + typedef struct VhostUserMsg { VhostUserRequest request; @@ -109,6 +129,7 @@ typedef struct VhostUserMsg { VhostUserMemory memory; VhostUserLog log; struct vhost_iotlb_msg iotlb; + VhostUserConfig config; } payload; } QEMU_PACKED VhostUserMsg; @@ -922,6 +943,89 @@ 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, + uint32_t config_len) +{ + VhostUserMsg msg = { + msg.request = VHOST_USER_GET_CONFIG, + msg.flags = VHOST_USER_VERSION, + msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, + }; + + msg.payload.config.offset = 0; + msg.payload.config.size = config_len; + 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 != VHOST_USER_CONFIG_HDR_SIZE + config_len) { + 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 *data, + uint32_t offset, uint32_t size, uint32_t flags) +{ + uint8_t *p; + bool reply_supported = virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_REPLY_ACK); + + VhostUserMsg msg = { + msg.request = VHOST_USER_SET_CONFIG, + msg.flags = VHOST_USER_VERSION, + msg.size = VHOST_USER_CONFIG_HDR_SIZE + size, + }; + + if (reply_supported) { + msg.flags |= VHOST_USER_NEED_REPLY_MASK; + } + + msg.payload.config.offset = offset, + msg.payload.config.size = size, + msg.payload.config.flags = flags, + p = msg.payload.config.region; + memcpy(p, data, 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 +1052,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 e4290ce..aa93398 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1358,6 +1358,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); @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, } } +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, + uint32_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 -1; +} + +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, + uint32_t offset, uint32_t size, uint32_t flags) +{ + assert(hdev->vhost_ops); + + if (hdev->vhost_ops->vhost_set_config) { + return hdev->vhost_ops->vhost_set_config(hdev, data, offset, + size, flags); + } + + return -1; +} + +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 -1; +} + /* 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..8996d5f 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -20,6 +20,11 @@ typedef enum VhostBackendType { VHOST_BACKEND_TYPE_MAX = 3, } VhostBackendType; +typedef enum VhostSetConfigType { + VHOST_SET_CONFIG_TYPE_MASTER = 0, + VHOST_SET_CONFIG_TYPE_MIGRATION = 1, +} VhostSetConfigType; + struct vhost_dev; struct vhost_log; struct vhost_memory; @@ -84,6 +89,12 @@ 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 *data, + uint32_t offset, uint32_t size, + uint32_t flags); +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, + uint32_t config_len); +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd); typedef struct VhostOps { VhostBackendType backend_type; @@ -118,6 +129,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..7f43a82 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, + uint32_t config_len); +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, + uint32_t offset, uint32_t size, uint32_t flags); +/* notifier callback in case vhost device config space changed + */ +int vhost_dev_set_config_notifier(struct vhost_dev *dev, + const VhostDevConfigOps *ops); #endif
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 | 45 ++++++++++++++++ hw/virtio/vhost-user.c | 107 ++++++++++++++++++++++++++++++++++++++ hw/virtio/vhost.c | 64 +++++++++++++++++++++++ include/hw/virtio/vhost-backend.h | 14 +++++ include/hw/virtio/vhost.h | 16 ++++++ 5 files changed, 246 insertions(+)