* [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
@ 2017-12-13 2:29 ` Changpeng Liu
2017-12-13 10:15 ` Stefan Hajnoczi
2017-12-20 15:47 ` Marc-André Lureau
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 2/4] vhost-user-blk: introduce a new vhost-user-blk host device Changpeng Liu
` (4 subsequent siblings)
5 siblings, 2 replies; 16+ messages in thread
From: Changpeng Liu @ 2017-12-13 2:29 UTC (permalink / raw)
To: changpeng.liu, qemu-devel
Cc: stefanha, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
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(+)
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
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space Changpeng Liu
@ 2017-12-13 10:15 ` Stefan Hajnoczi
2017-12-20 15:47 ` Marc-André Lureau
1 sibling, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2017-12-13 10:15 UTC (permalink / raw)
To: Changpeng Liu
Cc: qemu-devel, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
[-- Attachment #1: Type: text/plain, Size: 956 bytes --]
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>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space Changpeng Liu
2017-12-13 10:15 ` Stefan Hajnoczi
@ 2017-12-20 15:47 ` Marc-André Lureau
2017-12-20 18:28 ` Marc-André Lureau
2017-12-21 0:21 ` Michael S. Tsirkin
1 sibling, 2 replies; 16+ messages in thread
From: Marc-André Lureau @ 2017-12-20 15:47 UTC (permalink / raw)
To: Changpeng Liu
Cc: QEMU, james.r.harris, Michael S. Tsirkin, Stefan Hajnoczi,
Paolo Bonzini, Felipe Franciosi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-20 15:47 ` Marc-André Lureau
@ 2017-12-20 18:28 ` Marc-André Lureau
2017-12-21 0:18 ` Michael S. Tsirkin
2017-12-21 0:21 ` Michael S. Tsirkin
1 sibling, 1 reply; 16+ messages in thread
From: Marc-André Lureau @ 2017-12-20 18:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Felipe Franciosi, Paolo Bonzini, QEMU, Stefan Hajnoczi, james.r.harris
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
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-20 18:28 ` Marc-André Lureau
@ 2017-12-21 0:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2017-12-21 0:18 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Felipe Franciosi, Paolo Bonzini, QEMU, Stefan Hajnoczi, james.r.harris
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-20 15:47 ` Marc-André Lureau
2017-12-20 18:28 ` Marc-André Lureau
@ 2017-12-21 0:21 ` Michael S. Tsirkin
2017-12-21 10:47 ` Marc-André Lureau
1 sibling, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2017-12-21 0:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Changpeng Liu, QEMU, james.r.harris, Stefan Hajnoczi,
Paolo Bonzini, Felipe Franciosi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-21 0:21 ` Michael S. Tsirkin
@ 2017-12-21 10:47 ` Marc-André Lureau
2017-12-21 23:47 ` Liu, Changpeng
0 siblings, 1 reply; 16+ messages in thread
From: Marc-André Lureau @ 2017-12-21 10:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Changpeng Liu, QEMU, james.r.harris, Stefan Hajnoczi,
Paolo Bonzini, Felipe Franciosi
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
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
2017-12-21 10:47 ` Marc-André Lureau
@ 2017-12-21 23:47 ` Liu, Changpeng
0 siblings, 0 replies; 16+ messages in thread
From: Liu, Changpeng @ 2017-12-21 23:47 UTC (permalink / raw)
To: Marc-André Lureau, Michael S. Tsirkin
Cc: QEMU, Harris, James R, Stefan Hajnoczi, Paolo Bonzini, Felipe Franciosi
> -----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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v7 2/4] vhost-user-blk: introduce a new vhost-user-blk host device
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space Changpeng Liu
@ 2017-12-13 2:29 ` Changpeng Liu
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 3/4] contrib/libvhost-user: enable virtio config space messages Changpeng Liu
` (3 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Changpeng Liu @ 2017-12-13 2:29 UTC (permalink / raw)
To: changpeng.liu, qemu-devel
Cc: stefanha, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
This commit introduces a new vhost-user device for block, it uses a
chardev to connect with the backend, same with Qemu virito-blk device,
Guest OS still uses the virtio-blk frontend driver.
To use it, start QEMU with command line like this:
qemu-system-x86_64 \
-chardev socket,id=char0,path=/path/vhost.socket \
-device vhost-user-blk-pci,chardev=char0,num-queues=2, \
bootindex=2... \
Users can use different parameters for `num-queues` and `bootindex`.
Different with exist Qemu virtio-blk host device, it makes more easy
for users to implement their own I/O processing logic, such as all
user space I/O stack against hardware block device. It uses the new
vhost messages(VHOST_USER_GET_CONFIG) to get block virtio config
information from backend process.
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
default-configs/pci.mak | 1 +
hw/block/Makefile.objs | 3 +
hw/block/vhost-user-blk.c | 357 +++++++++++++++++++++++++++++++++++++
hw/virtio/virtio-pci.c | 55 ++++++
hw/virtio/virtio-pci.h | 18 ++
include/hw/virtio/vhost-user-blk.h | 41 +++++
6 files changed, 475 insertions(+)
create mode 100644 hw/block/vhost-user-blk.c
create mode 100644 include/hw/virtio/vhost-user-blk.h
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index e514bde..49a0f28 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -43,3 +43,4 @@ CONFIG_VGA_PCI=y
CONFIG_IVSHMEM_DEVICE=$(CONFIG_IVSHMEM)
CONFIG_ROCKER=y
CONFIG_VHOST_USER_SCSI=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
+CONFIG_VHOST_USER_BLK=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index e0ed980..4c19a58 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -13,3 +13,6 @@ obj-$(CONFIG_SH4) += tc58128.o
obj-$(CONFIG_VIRTIO) += virtio-blk.o
obj-$(CONFIG_VIRTIO) += dataplane/
+ifeq ($(CONFIG_VIRTIO),y)
+obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
+endif
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
new file mode 100644
index 0000000..ff326e9
--- /dev/null
+++ b/hw/block/vhost-user-blk.c
@@ -0,0 +1,357 @@
+/*
+ * vhost-user-blk host device
+ *
+ * Copyright(C) 2017 Intel Corporation.
+ *
+ * Authors:
+ * Changpeng Liu <changpeng.liu@intel.com>
+ *
+ * Largely based on the "vhost-user-scsi.c" and "vhost-scsi.c" implemented by:
+ * Felipe Franciosi <felipe@nutanix.com>
+ * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
+ * Nicholas Bellinger <nab@risingtidesystems.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/typedefs.h"
+#include "qemu/cutils.h"
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user-blk.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+
+static const int user_feature_bits[] = {
+ VIRTIO_BLK_F_SIZE_MAX,
+ VIRTIO_BLK_F_SEG_MAX,
+ VIRTIO_BLK_F_GEOMETRY,
+ VIRTIO_BLK_F_BLK_SIZE,
+ VIRTIO_BLK_F_TOPOLOGY,
+ VIRTIO_BLK_F_MQ,
+ VIRTIO_BLK_F_RO,
+ VIRTIO_BLK_F_FLUSH,
+ VIRTIO_BLK_F_CONFIG_WCE,
+ VIRTIO_F_VERSION_1,
+ VIRTIO_RING_F_INDIRECT_DESC,
+ VIRTIO_RING_F_EVENT_IDX,
+ VIRTIO_F_NOTIFY_ON_EMPTY,
+ VHOST_INVALID_FEATURE_BIT
+};
+
+static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+
+ memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
+}
+
+static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ struct virtio_blk_config *blkcfg = (struct virtio_blk_config *)config;
+ int ret;
+
+ if (blkcfg->wce == s->blkcfg.wce) {
+ return;
+ }
+
+ ret = vhost_dev_set_config(&s->dev, &blkcfg->wce,
+ offsetof(struct virtio_blk_config, wce),
+ sizeof(blkcfg->wce),
+ VHOST_SET_CONFIG_TYPE_MASTER);
+ if (ret) {
+ error_report("set device config space failed");
+ return;
+ }
+
+ s->blkcfg.wce = blkcfg->wce;
+}
+
+static void vhost_user_blk_handle_config_change(struct vhost_dev *dev)
+{
+ int ret;
+ struct virtio_blk_config blkcfg;
+ VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
+
+ ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
+ sizeof(struct virtio_blk_config));
+ if (ret < 0) {
+ error_report("get config space failed");
+ return;
+ }
+
+ /* valid for resize only */
+ if (blkcfg.capacity != s->blkcfg.capacity) {
+ s->blkcfg.capacity = blkcfg.capacity;
+ memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct virtio_blk_config));
+ virtio_notify_config(dev->vdev);
+ }
+}
+
+const VhostDevConfigOps blk_ops = {
+ .vhost_dev_config_notifier = vhost_user_blk_handle_config_change,
+};
+
+static void vhost_user_blk_start(VirtIODevice *vdev)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+ VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+ int i, ret;
+
+ if (!k->set_guest_notifiers) {
+ error_report("binding does not support guest notifiers");
+ return;
+ }
+
+ ret = vhost_dev_enable_notifiers(&s->dev, vdev);
+ if (ret < 0) {
+ error_report("Error enabling host notifiers: %d", -ret);
+ return;
+ }
+
+ ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true);
+ if (ret < 0) {
+ error_report("Error binding guest notifier: %d", -ret);
+ goto err_host_notifiers;
+ }
+
+ s->dev.acked_features = vdev->guest_features;
+ ret = vhost_dev_start(&s->dev, vdev);
+ if (ret < 0) {
+ error_report("Error starting vhost: %d", -ret);
+ goto err_guest_notifiers;
+ }
+
+ /* guest_notifier_mask/pending not used yet, so just unmask
+ * everything here. virtio-pci will do the right thing by
+ * enabling/disabling irqfd.
+ */
+ for (i = 0; i < s->dev.nvqs; i++) {
+ vhost_virtqueue_mask(&s->dev, vdev, i, false);
+ }
+
+ return;
+
+err_guest_notifiers:
+ k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
+err_host_notifiers:
+ vhost_dev_disable_notifiers(&s->dev, vdev);
+}
+
+static void vhost_user_blk_stop(VirtIODevice *vdev)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+ VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+ int ret;
+
+ if (!k->set_guest_notifiers) {
+ return;
+ }
+
+ vhost_dev_stop(&s->dev, vdev);
+
+ ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
+ if (ret < 0) {
+ error_report("vhost guest notifier cleanup failed: %d", ret);
+ return;
+ }
+
+ vhost_dev_disable_notifiers(&s->dev, vdev);
+}
+
+static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+
+ if (!vdev->vm_running) {
+ should_start = false;
+ }
+
+ if (s->dev.started == should_start) {
+ return;
+ }
+
+ if (should_start) {
+ vhost_user_blk_start(vdev);
+ } else {
+ vhost_user_blk_stop(vdev);
+ }
+
+}
+
+static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
+ uint64_t features,
+ Error **errp)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ uint64_t get_features;
+
+ /* Turn on pre-defined features */
+ virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
+ virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
+ virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
+ virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
+ virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
+
+ if (s->config_wce) {
+ virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
+ }
+ if (s->config_ro) {
+ virtio_add_feature(&features, VIRTIO_BLK_F_RO);
+ }
+ if (s->num_queues > 1) {
+ virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
+ }
+
+ get_features = vhost_get_features(&s->dev, user_feature_bits, features);
+
+ return get_features;
+}
+
+static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+
+}
+
+static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ int i, ret;
+
+ if (!s->chardev.chr) {
+ error_setg(errp, "vhost-user-blk: chardev is mandatory");
+ return;
+ }
+
+ if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
+ error_setg(errp, "vhost-user-blk: invalid number of IO queues");
+ return;
+ }
+
+ if (!s->queue_size) {
+ error_setg(errp, "vhost-user-blk: queue size must be non-zero");
+ return;
+ }
+
+ virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
+ sizeof(struct virtio_blk_config));
+
+ for (i = 0; i < s->num_queues; i++) {
+ virtio_add_queue(vdev, s->queue_size,
+ vhost_user_blk_handle_output);
+ }
+
+ s->dev.nvqs = s->num_queues;
+ s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs);
+ s->dev.vq_index = 0;
+ s->dev.backend_features = 0;
+
+ ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0);
+ if (ret < 0) {
+ error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
+ strerror(-ret));
+ goto virtio_err;
+ }
+
+ ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
+ sizeof(struct virtio_blk_config));
+ if (ret < 0) {
+ error_setg(errp, "vhost-user-blk: get block config failed");
+ goto vhost_err;
+ }
+
+ if (s->blkcfg.num_queues != s->num_queues) {
+ s->blkcfg.num_queues = s->num_queues;
+ }
+
+ vhost_dev_set_config_notifier(&s->dev, &blk_ops);
+
+ return;
+
+vhost_err:
+ vhost_dev_cleanup(&s->dev);
+virtio_err:
+ g_free(s->dev.vqs);
+ virtio_cleanup(vdev);
+}
+
+static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VHostUserBlk *s = VHOST_USER_BLK(dev);
+
+ vhost_user_blk_set_status(vdev, 0);
+ vhost_dev_cleanup(&s->dev);
+ g_free(s->dev.vqs);
+ virtio_cleanup(vdev);
+}
+
+static void vhost_user_blk_instance_init(Object *obj)
+{
+ VHostUserBlk *s = VHOST_USER_BLK(obj);
+
+ device_add_bootindex_property(obj, &s->bootindex, "bootindex",
+ "/disk@0,0", DEVICE(obj), NULL);
+}
+
+static const VMStateDescription vmstate_vhost_user_blk = {
+ .name = "vhost-user-blk",
+ .minimum_version_id = 1,
+ .version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_VIRTIO_DEVICE,
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static Property vhost_user_blk_properties[] = {
+ DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
+ DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1),
+ DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
+ DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
+ DEFINE_PROP_BIT("config-ro", VHostUserBlk, config_ro, 0, false),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+ dc->props = vhost_user_blk_properties;
+ dc->vmsd = &vmstate_vhost_user_blk;
+ set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ vdc->realize = vhost_user_blk_device_realize;
+ vdc->unrealize = vhost_user_blk_device_unrealize;
+ vdc->get_config = vhost_user_blk_update_config;
+ vdc->set_config = vhost_user_blk_set_config;
+ vdc->get_features = vhost_user_blk_get_features;
+ vdc->set_status = vhost_user_blk_set_status;
+}
+
+static const TypeInfo vhost_user_blk_info = {
+ .name = TYPE_VHOST_USER_BLK,
+ .parent = TYPE_VIRTIO_DEVICE,
+ .instance_size = sizeof(VHostUserBlk),
+ .instance_init = vhost_user_blk_instance_init,
+ .class_init = vhost_user_blk_class_init,
+};
+
+static void virtio_register_types(void)
+{
+ type_register_static(&vhost_user_blk_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e92837c..b5e05d9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1976,6 +1976,58 @@ static const TypeInfo virtio_blk_pci_info = {
.class_init = virtio_blk_pci_class_init,
};
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
+/* vhost-user-blk */
+
+static Property vhost_user_blk_pci_properties[] = {
+ DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
+ DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+ VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev);
+ DeviceState *vdev = DEVICE(&dev->vdev);
+
+ qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+ object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void vhost_user_blk_pci_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+ PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+ set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ dc->props = vhost_user_blk_pci_properties;
+ k->realize = vhost_user_blk_pci_realize;
+ pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+ pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
+ pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+ pcidev_k->class_id = PCI_CLASS_STORAGE_SCSI;
+}
+
+static void vhost_user_blk_pci_instance_init(Object *obj)
+{
+ VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(obj);
+
+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+ TYPE_VHOST_USER_BLK);
+ object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
+ "bootindex", &error_abort);
+}
+
+static const TypeInfo vhost_user_blk_pci_info = {
+ .name = TYPE_VHOST_USER_BLK_PCI,
+ .parent = TYPE_VIRTIO_PCI,
+ .instance_size = sizeof(VHostUserBlkPCI),
+ .instance_init = vhost_user_blk_pci_instance_init,
+ .class_init = vhost_user_blk_pci_class_init,
+};
+#endif
+
/* virtio-scsi-pci */
static Property virtio_scsi_pci_properties[] = {
@@ -2622,6 +2674,9 @@ static void virtio_pci_register_types(void)
type_register_static(&virtio_9p_pci_info);
#endif
type_register_static(&virtio_blk_pci_info);
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
+ type_register_static(&vhost_user_blk_pci_info);
+#endif
type_register_static(&virtio_scsi_pci_info);
type_register_static(&virtio_balloon_pci_info);
type_register_static(&virtio_serial_pci_info);
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 12d3a90..813082b 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -27,6 +27,9 @@
#include "hw/virtio/virtio-gpu.h"
#include "hw/virtio/virtio-crypto.h"
#include "hw/virtio/vhost-user-scsi.h"
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
+#include "hw/virtio/vhost-user-blk.h"
+#endif
#ifdef CONFIG_VIRTFS
#include "hw/9pfs/virtio-9p.h"
@@ -46,6 +49,7 @@ typedef struct VirtIOSerialPCI VirtIOSerialPCI;
typedef struct VirtIONetPCI VirtIONetPCI;
typedef struct VHostSCSIPCI VHostSCSIPCI;
typedef struct VHostUserSCSIPCI VHostUserSCSIPCI;
+typedef struct VHostUserBlkPCI VHostUserBlkPCI;
typedef struct VirtIORngPCI VirtIORngPCI;
typedef struct VirtIOInputPCI VirtIOInputPCI;
typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
@@ -244,6 +248,20 @@ struct VHostUserSCSIPCI {
VHostUserSCSI vdev;
};
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
+/*
+ * vhost-user-blk-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci"
+#define VHOST_USER_BLK_PCI(obj) \
+ OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
+
+struct VHostUserBlkPCI {
+ VirtIOPCIProxy parent_obj;
+ VHostUserBlk vdev;
+};
+#endif
+
/*
* virtio-blk-pci: This extends VirtioPCIProxy.
*/
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
new file mode 100644
index 0000000..5804cc9
--- /dev/null
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -0,0 +1,41 @@
+/*
+ * vhost-user-blk host device
+ * Copyright(C) 2017 Intel Corporation.
+ *
+ * Authors:
+ * Changpeng Liu <changpeng.liu@intel.com>
+ *
+ * Based on vhost-scsi.h, Copyright IBM, Corp. 2011
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef VHOST_USER_BLK_H
+#define VHOST_USER_BLK_H
+
+#include "standard-headers/linux/virtio_blk.h"
+#include "qemu-common.h"
+#include "hw/qdev.h"
+#include "hw/block/block.h"
+#include "chardev/char-fe.h"
+#include "hw/virtio/vhost.h"
+
+#define TYPE_VHOST_USER_BLK "vhost-user-blk"
+#define VHOST_USER_BLK(obj) \
+ OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK)
+
+typedef struct VHostUserBlk {
+ VirtIODevice parent_obj;
+ CharBackend chardev;
+ int32_t bootindex;
+ struct virtio_blk_config blkcfg;
+ uint16_t num_queues;
+ uint32_t queue_size;
+ uint32_t config_wce;
+ uint32_t config_ro;
+ struct vhost_dev dev;
+} VHostUserBlk;
+
+#endif
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v7 3/4] contrib/libvhost-user: enable virtio config space messages
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space Changpeng Liu
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 2/4] vhost-user-blk: introduce a new vhost-user-blk host device Changpeng Liu
@ 2017-12-13 2:29 ` Changpeng Liu
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application Changpeng Liu
` (2 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Changpeng Liu @ 2017-12-13 2:29 UTC (permalink / raw)
To: changpeng.liu, qemu-devel
Cc: stefanha, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
Enable VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG/VHOST_USER_SET_CONFIG_FD
messages in libvhost-user library, users can implement their own I/O target
based on the library. This enable the virtio config space delivered between
Qemu host device and the I/O target, also event notifier is added in case
of virtio config space changed.
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
contrib/libvhost-user/libvhost-user.c | 80 +++++++++++++++++++++++++++++++++++
contrib/libvhost-user/libvhost-user.h | 36 +++++++++++++++-
2 files changed, 115 insertions(+), 1 deletion(-)
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index f409bd3..5dd00c9 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -84,6 +84,9 @@ vu_request_to_string(unsigned int req)
REQ(VHOST_USER_SET_SLAVE_REQ_FD),
REQ(VHOST_USER_IOTLB_MSG),
REQ(VHOST_USER_SET_VRING_ENDIAN),
+ REQ(VHOST_USER_GET_CONFIG),
+ REQ(VHOST_USER_SET_CONFIG),
+ REQ(VHOST_USER_SET_CONFIG_FD),
REQ(VHOST_USER_MAX),
};
#undef REQ
@@ -798,6 +801,70 @@ vu_set_slave_req_fd(VuDev *dev, VhostUserMsg *vmsg)
}
static bool
+vu_get_config(VuDev *dev, VhostUserMsg *vmsg)
+{
+ int ret = -1;
+
+ if (dev->iface->get_config) {
+ ret = dev->iface->get_config(dev, vmsg->payload.config.region,
+ vmsg->payload.config.size);
+ }
+
+ if (ret) {
+ /* resize to zero to indicate an error to master */
+ vmsg->size = 0;
+ }
+
+ return true;
+}
+
+static bool
+vu_set_config(VuDev *dev, VhostUserMsg *vmsg)
+{
+ int ret = -1;
+ bool reply_supported = !!(dev->protocol_features &
+ (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK));
+
+ if (dev->iface->set_config) {
+ ret = dev->iface->set_config(dev, vmsg->payload.config.region,
+ vmsg->payload.config.offset,
+ vmsg->payload.config.size,
+ vmsg->payload.config.flags);
+ }
+
+ vmsg->size = sizeof(vmsg->payload.u64);
+ if (!ret) {
+ vmsg->payload.u64 = 0;
+ } else {
+ /* indicate an error in case reply supported */
+ vmsg->payload.u64 = 1;
+ }
+
+ if (reply_supported) {
+ return true;
+ }
+
+ return false;
+}
+
+static bool
+vu_set_config_fd(VuDev *dev, VhostUserMsg *vmsg)
+{
+ if (vmsg->fd_num != 1) {
+ vu_panic(dev, "Invalid config_fd message");
+ return false;
+ }
+
+ if (dev->config_fd != -1) {
+ close(dev->config_fd);
+ }
+ dev->config_fd = vmsg->fds[0];
+ DPRINT("Got config_fd: %d\n", vmsg->fds[0]);
+
+ return false;
+}
+
+static bool
vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
{
int do_reply = 0;
@@ -862,6 +929,12 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
return vu_set_vring_enable_exec(dev, vmsg);
case VHOST_USER_SET_SLAVE_REQ_FD:
return vu_set_slave_req_fd(dev, vmsg);
+ case VHOST_USER_GET_CONFIG:
+ return vu_get_config(dev, vmsg);
+ case VHOST_USER_SET_CONFIG:
+ return vu_set_config(dev, vmsg);
+ case VHOST_USER_SET_CONFIG_FD:
+ return vu_set_config_fd(dev, vmsg);
case VHOST_USER_NONE:
break;
default:
@@ -940,6 +1013,11 @@ vu_deinit(VuDev *dev)
dev->slave_fd = -1;
}
+ if (dev->config_fd != -1) {
+ close(dev->config_fd);
+ dev->config_fd = -1;
+ }
+
if (dev->sock != -1) {
close(dev->sock);
}
@@ -970,6 +1048,8 @@ vu_init(VuDev *dev,
dev->iface = iface;
dev->log_call_fd = -1;
dev->slave_fd = -1;
+ dev->config_fd = -1;
+
for (i = 0; i < VHOST_MAX_NR_VIRTQUEUE; i++) {
dev->vq[i] = (VuVirtq) {
.call_fd = -1, .kick_fd = -1, .err_fd = -1,
diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
index 2f5864b..677389d 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -30,6 +30,16 @@
#define VHOST_MEMORY_MAX_NREGIONS 8
+typedef enum VhostSetConfigType {
+ VHOST_SET_CONFIG_TYPE_MASTER = 0,
+ VHOST_SET_CONFIG_TYPE_MIGRATION = 1,
+} VhostSetConfigType;
+
+/*
+ * 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,
@@ -38,7 +48,6 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_NET_MTU = 4,
VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5,
VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
-
VHOST_USER_PROTOCOL_F_MAX
};
@@ -69,6 +78,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;
@@ -90,6 +102,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))
+
#if defined(_WIN32)
# define VU_PACKED __attribute__((gcc_struct, packed))
#else
@@ -112,6 +136,7 @@ typedef struct VhostUserMsg {
struct vhost_vring_addr addr;
VhostUserMemory memory;
VhostUserLog log;
+ VhostUserConfig config;
} payload;
int fds[VHOST_MEMORY_MAX_NREGIONS];
@@ -140,6 +165,10 @@ typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
int *do_reply);
typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool started);
typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int qidx);
+typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t len);
+typedef int (*vu_set_config_cb) (VuDev *dev, const uint8_t *data,
+ uint32_t offset, uint32_t size,
+ uint32_t flags);
typedef struct VuDevIface {
/* called by VHOST_USER_GET_FEATURES to get the features bitmask */
@@ -162,6 +191,10 @@ typedef struct VuDevIface {
* on unmanaged exit/crash.
*/
vu_queue_is_processed_in_order_cb queue_is_processed_in_order;
+ /* get the config space of the device */
+ vu_get_config_cb get_config;
+ /* set the config space of the device */
+ vu_set_config_cb set_config;
} VuDevIface;
typedef void (*vu_queue_handler_cb) (VuDev *dev, int qidx);
@@ -227,6 +260,7 @@ struct VuDev {
VuVirtq vq[VHOST_MAX_NR_VIRTQUEUE];
int log_call_fd;
int slave_fd;
+ int config_fd;
uint64_t log_size;
uint8_t *log_table;
uint64_t features;
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
` (2 preceding siblings ...)
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 3/4] contrib/libvhost-user: enable virtio config space messages Changpeng Liu
@ 2017-12-13 2:29 ` Changpeng Liu
2017-12-20 16:02 ` Marc-André Lureau
2017-12-13 10:22 ` [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Stefan Hajnoczi
2017-12-13 10:24 ` Stefan Hajnoczi
5 siblings, 1 reply; 16+ messages in thread
From: Changpeng Liu @ 2017-12-13 2:29 UTC (permalink / raw)
To: changpeng.liu, qemu-devel
Cc: stefanha, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
This commit introcudes a vhost-user-blk backend device, it uses UNIX
domain socket to communicate with QEMU. The vhost-user-blk sample
application should be used with QEMU vhost-user-blk-pci device.
To use it, complie with:
make vhost-user-blk
and start like this:
vhost-user-blk -b /dev/sdb -s /path/vhost.socket
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
.gitignore | 1 +
Makefile | 3 +
Makefile.objs | 1 +
contrib/vhost-user-blk/Makefile.objs | 1 +
contrib/vhost-user-blk/vhost-user-blk.c | 550 ++++++++++++++++++++++++++++++++
5 files changed, 556 insertions(+)
create mode 100644 contrib/vhost-user-blk/Makefile.objs
create mode 100644 contrib/vhost-user-blk/vhost-user-blk.c
diff --git a/.gitignore b/.gitignore
index 588769b..495f854 100644
--- a/.gitignore
+++ b/.gitignore
@@ -55,6 +55,7 @@
/scsi/qemu-pr-helper
/vscclient
/vhost-user-scsi
+/vhost-user-blk
/fsdev/virtfs-proxy-helper
*.tmp
*.[1-9]
diff --git a/Makefile b/Makefile
index ab0354c..3aad271 100644
--- a/Makefile
+++ b/Makefile
@@ -327,6 +327,7 @@ dummy := $(call unnest-vars,, \
ivshmem-server-obj-y \
libvhost-user-obj-y \
vhost-user-scsi-obj-y \
+ vhost-user-blk-obj-y \
qga-vss-dll-obj-y \
block-obj-y \
block-obj-m \
@@ -558,6 +559,8 @@ ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
endif
vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y) libvhost-user.a
$(call LINK, $^)
+vhost-user-blk$(EXESUF): $(vhost-user-blk-obj-y) libvhost-user.a
+ $(call LINK, $^)
module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
$(call quiet-command,$(PYTHON) $< $@ \
diff --git a/Makefile.objs b/Makefile.objs
index 285c6f3..ae9aef7 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -115,6 +115,7 @@ libvhost-user-obj-y = contrib/libvhost-user/
vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
+vhost-user-blk-obj-y = contrib/vhost-user-blk/
######################################################################
trace-events-subdirs =
diff --git a/contrib/vhost-user-blk/Makefile.objs b/contrib/vhost-user-blk/Makefile.objs
new file mode 100644
index 0000000..72e2cdc
--- /dev/null
+++ b/contrib/vhost-user-blk/Makefile.objs
@@ -0,0 +1 @@
+vhost-user-blk-obj-y = vhost-user-blk.o
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
new file mode 100644
index 0000000..841c3c3
--- /dev/null
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -0,0 +1,550 @@
+/*
+ * vhost-user-blk sample application
+ *
+ * Copyright (c) 2017 Intel Corporation. All rights reserved.
+ *
+ * Author:
+ * Changpeng Liu <changpeng.liu@intel.com>
+ *
+ * This work is based on the "vhost-user-scsi" sample and "virtio-blk" driver
+ * implemention by:
+ * Felipe Franciosi <felipe@nutanix.com>
+ * Anthony Liguori <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 only.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "standard-headers/linux/virtio_blk.h"
+#include "contrib/libvhost-user/libvhost-user-glib.h"
+#include "contrib/libvhost-user/libvhost-user.h"
+
+#include <glib.h>
+
+struct virtio_blk_inhdr {
+ unsigned char status;
+};
+
+/* vhost user block device */
+typedef struct VubDev {
+ VugDev parent;
+ int blk_fd;
+ struct virtio_blk_config blkcfg;
+ char *blk_name;
+ GMainLoop *loop;
+} VubDev;
+
+typedef struct VubReq {
+ VuVirtqElement *elem;
+ int64_t sector_num;
+ size_t size;
+ struct virtio_blk_inhdr *in;
+ struct virtio_blk_outhdr *out;
+ VubDev *vdev_blk;
+ struct VuVirtq *vq;
+} VubReq;
+
+/** refer util/iov.c **/
+static size_t vub_iov_size(const struct iovec *iov,
+ const unsigned int iov_cnt)
+{
+ size_t len;
+ unsigned int i;
+
+ len = 0;
+ for (i = 0; i < iov_cnt; i++) {
+ len += iov[i].iov_len;
+ }
+ return len;
+}
+
+static void vub_panic_cb(VuDev *vu_dev, const char *buf)
+{
+ VugDev *gdev;
+ VubDev *vdev_blk;
+
+ assert(vu_dev);
+
+ gdev = container_of(vu_dev, VugDev, parent);
+ vdev_blk = container_of(gdev, VubDev, parent);
+ if (buf) {
+ g_warning("vu_panic: %s", buf);
+ }
+
+ g_main_loop_quit(vdev_blk->loop);
+}
+
+static void vub_req_complete(VubReq *req)
+{
+ VugDev *gdev = &req->vdev_blk->parent;
+ VuDev *vu_dev = &gdev->parent;
+
+ /* IO size with 1 extra status byte */
+ vu_queue_push(vu_dev, req->vq, req->elem,
+ req->size + 1);
+ vu_queue_notify(vu_dev, req->vq);
+
+ if (req->elem) {
+ free(req->elem);
+ }
+ g_free(req);
+}
+
+static int vub_open(const char *file_name, bool wce)
+{
+ int fd;
+ int flags = O_RDWR;
+
+ if (!wce) {
+ flags |= O_DIRECT;
+ }
+
+ fd = open(file_name, flags);
+ if (fd < 0) {
+ fprintf(stderr, "Cannot open file %s, %s\n", file_name,
+ strerror(errno));
+ return -1;
+ }
+
+ return fd;
+}
+
+static void vub_close(int fd)
+{
+ if (fd >= 0) {
+ close(fd);
+ }
+}
+
+static ssize_t
+vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt)
+{
+ VubDev *vdev_blk = req->vdev_blk;
+ ssize_t rc;
+
+ if (!iovcnt) {
+ fprintf(stderr, "Invalid Read IOV count\n");
+ return -1;
+ }
+
+ req->size = vub_iov_size(iov, iovcnt);
+ rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
+ if (rc < 0) {
+ fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
+ vdev_blk->blk_name, req->sector_num, req->size,
+ strerror(errno));
+ return -1;
+ }
+
+ return rc;
+}
+
+static ssize_t
+vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
+{
+ VubDev *vdev_blk = req->vdev_blk;
+ ssize_t rc;
+
+ if (!iovcnt) {
+ fprintf(stderr, "Invalid Write IOV count\n");
+ return -1;
+ }
+
+ req->size = vub_iov_size(iov, iovcnt);
+ rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
+ if (rc < 0) {
+ fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
+ vdev_blk->blk_name, req->sector_num, req->size,
+ strerror(errno));
+ return -1;
+ }
+
+ return rc;
+}
+
+static void
+vub_flush(VubReq *req)
+{
+ VubDev *vdev_blk = req->vdev_blk;
+
+ fdatasync(vdev_blk->blk_fd);
+}
+
+
+static int vub_virtio_process_req(VubDev *vdev_blk,
+ VuVirtq *vq)
+{
+ VugDev *gdev = &vdev_blk->parent;
+ VuDev *vu_dev = &gdev->parent;
+ VuVirtqElement *elem;
+ uint32_t type;
+ unsigned in_num;
+ unsigned out_num;
+ VubReq *req;
+
+ elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement));
+ if (!elem) {
+ return -1;
+ }
+
+ /* refer to hw/block/virtio_blk.c */
+ if (elem->out_num < 1 || elem->in_num < 1) {
+ fprintf(stderr, "virtio-blk request missing headers\n");
+ free(elem);
+ return -1;
+ }
+
+ req = g_new0(VubReq, 1);
+ req->vdev_blk = vdev_blk;
+ req->vq = vq;
+ req->elem = elem;
+
+ in_num = elem->in_num;
+ out_num = elem->out_num;
+
+ /* don't support VIRTIO_F_ANY_LAYOUT and virtio 1.0 only */
+ if (elem->out_sg[0].iov_len < sizeof(struct virtio_blk_outhdr)) {
+ fprintf(stderr, "Invalid outhdr size\n");
+ goto err;
+ }
+ req->out = (struct virtio_blk_outhdr *)elem->out_sg[0].iov_base;
+ out_num--;
+
+ if (elem->in_sg[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
+ fprintf(stderr, "Invalid inhdr size\n");
+ goto err;
+ }
+ req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
+ in_num--;
+
+ type = le32toh(req->out->type);
+ switch (type & ~(VIRTIO_BLK_T_OUT | VIRTIO_BLK_T_BARRIER)) {
+ case VIRTIO_BLK_T_IN: {
+ ssize_t ret = 0;
+ bool is_write = type & VIRTIO_BLK_T_OUT;
+ req->sector_num = le64toh(req->out->sector);
+ if (is_write) {
+ ret = vub_writev(req, &elem->out_sg[1], out_num);
+ } else {
+ ret = vub_readv(req, &elem->in_sg[0], in_num);
+ }
+ if (ret >= 0) {
+ req->in->status = VIRTIO_BLK_S_OK;
+ } else {
+ req->in->status = VIRTIO_BLK_S_IOERR;
+ }
+ vub_req_complete(req);
+ break;
+ }
+ case VIRTIO_BLK_T_FLUSH: {
+ vub_flush(req);
+ req->in->status = VIRTIO_BLK_S_OK;
+ vub_req_complete(req);
+ break;
+ }
+ case VIRTIO_BLK_T_GET_ID: {
+ size_t size = MIN(vub_iov_size(&elem->in_sg[0], in_num),
+ VIRTIO_BLK_ID_BYTES);
+ snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk");
+ req->in->status = VIRTIO_BLK_S_OK;
+ req->size = elem->in_sg[0].iov_len;
+ vub_req_complete(req);
+ break;
+ }
+ default: {
+ req->in->status = VIRTIO_BLK_S_UNSUPP;
+ vub_req_complete(req);
+ break;
+ }
+ }
+
+ return 0;
+
+err:
+ free(elem);
+ g_free(req);
+ return -1;
+}
+
+static void vub_process_vq(VuDev *vu_dev, int idx)
+{
+ VugDev *gdev;
+ VubDev *vdev_blk;
+ VuVirtq *vq;
+ int ret;
+
+ if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
+ fprintf(stderr, "VQ Index out of range: %d\n", idx);
+ vub_panic_cb(vu_dev, NULL);
+ return;
+ }
+
+ gdev = container_of(vu_dev, VugDev, parent);
+ vdev_blk = container_of(gdev, VubDev, parent);
+ assert(vdev_blk);
+
+ vq = vu_get_queue(vu_dev, idx);
+ assert(vq);
+
+ while (1) {
+ ret = vub_virtio_process_req(vdev_blk, vq);
+ if (ret) {
+ break;
+ }
+ }
+}
+
+static void vub_queue_set_started(VuDev *vu_dev, int idx, bool started)
+{
+ VuVirtq *vq;
+
+ assert(vu_dev);
+
+ vq = vu_get_queue(vu_dev, idx);
+ vu_set_queue_handler(vu_dev, vq, started ? vub_process_vq : NULL);
+}
+
+static uint64_t
+vub_get_features(VuDev *dev)
+{
+ return 1ull << VIRTIO_BLK_F_SIZE_MAX |
+ 1ull << VIRTIO_BLK_F_SEG_MAX |
+ 1ull << VIRTIO_BLK_F_TOPOLOGY |
+ 1ull << VIRTIO_BLK_F_BLK_SIZE |
+ 1ull << VIRTIO_BLK_F_FLUSH |
+ 1ull << VIRTIO_BLK_F_CONFIG_WCE |
+ 1ull << VIRTIO_F_VERSION_1 |
+ 1ull << VHOST_USER_F_PROTOCOL_FEATURES;
+}
+
+static int
+vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
+{
+ VugDev *gdev;
+ VubDev *vdev_blk;
+
+ gdev = container_of(vu_dev, VugDev, parent);
+ vdev_blk = container_of(gdev, VubDev, parent);
+ memcpy(config, &vdev_blk->blkcfg, len);
+
+ return 0;
+}
+
+static int
+vub_set_config(VuDev *vu_dev, const uint8_t *data,
+ uint32_t offset, uint32_t size, uint32_t flags)
+{
+ VugDev *gdev;
+ VubDev *vdev_blk;
+ uint8_t wce;
+ int fd;
+
+ /* don't support live migration */
+ if (flags != VHOST_SET_CONFIG_TYPE_MASTER) {
+ return -1;
+ }
+
+ gdev = container_of(vu_dev, VugDev, parent);
+ vdev_blk = container_of(gdev, VubDev, parent);
+
+ if (offset != offsetof(struct virtio_blk_config, wce) ||
+ size != 1) {
+ return -1;
+ }
+
+ wce = *data;
+ if (wce == vdev_blk->blkcfg.wce) {
+ /* Do nothing as same with old configuration */
+ return 0;
+ }
+
+ vdev_blk->blkcfg.wce = wce;
+ fprintf(stdout, "Write Cache Policy Changed\n");
+ vub_close(vdev_blk->blk_fd);
+
+ fd = vub_open(vdev_blk->blk_name, wce);
+ if (fd < 0) {
+ fprintf(stderr, "Error to open block device %s\n", vdev_blk->blk_name);
+ vdev_blk->blk_fd = -1;
+ return -1;
+ }
+ vdev_blk->blk_fd = fd;
+
+ return 0;
+}
+
+static const VuDevIface vub_iface = {
+ .get_features = vub_get_features,
+ .queue_set_started = vub_queue_set_started,
+ .get_config = vub_get_config,
+ .set_config = vub_set_config,
+};
+
+static int unix_sock_new(char *unix_fn)
+{
+ int sock;
+ struct sockaddr_un un;
+ size_t len;
+
+ assert(unix_fn);
+
+ sock = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (sock <= 0) {
+ perror("socket");
+ return -1;
+ }
+
+ un.sun_family = AF_UNIX;
+ (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
+ len = sizeof(un.sun_family) + strlen(un.sun_path);
+
+ (void)unlink(unix_fn);
+ if (bind(sock, (struct sockaddr *)&un, len) < 0) {
+ perror("bind");
+ goto fail;
+ }
+
+ if (listen(sock, 1) < 0) {
+ perror("listen");
+ goto fail;
+ }
+
+ return sock;
+
+fail:
+ (void)close(sock);
+
+ return -1;
+}
+
+static void vub_deinit(struct VubDev *vdev_blk)
+{
+ if (!vdev_blk) {
+ return;
+ }
+
+ if (vdev_blk->blk_fd) {
+ vub_close(vdev_blk->blk_fd);
+ }
+}
+
+static uint32_t
+vub_get_blocksize(int fd)
+{
+ uint32_t blocksize = 512;
+
+ #if defined(__linux__) && defined(BLKSSZGET)
+ if (ioctl(fd, BLKSSZGET, &blocksize) == 0) {
+ return blocklen;
+ }
+ #endif
+
+ return blocksize;
+}
+
+static void
+vub_initialize_config(int fd, struct virtio_blk_config *config)
+{
+ off64_t capacity;
+
+ capacity = lseek64(fd, 0, SEEK_END);
+ config->capacity = capacity >> 9;
+ config->blk_size = vub_get_blocksize(fd);
+ config->size_max = 65536;
+ config->seg_max = 128 - 2;
+ config->min_io_size = 1;
+ config->opt_io_size = 1;
+ config->num_queues = 1;
+}
+
+static int
+vub_new_block(VubDev *vdev_blk, char *blk_file)
+{
+ vdev_blk->blk_fd = vub_open(blk_file, 0);
+ if (vdev_blk->blk_fd < 0) {
+ fprintf(stderr, "Error to open block device %s\n", blk_file);
+ return -1;
+ }
+ vdev_blk->blkcfg.wce = 0;
+ vdev_blk->blk_name = blk_file;
+
+ /* fill virtio_blk_config with block parameters */
+ vub_initialize_config(vdev_blk->blk_fd, &vdev_blk->blkcfg);
+
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ int opt;
+ char *unix_socket = NULL;
+ char *blk_file = NULL;
+ int lsock = -1, csock = -1, rc;
+ VubDev *vdev_blk = NULL;
+
+ while ((opt = getopt(argc, argv, "b:h:s:")) != -1) {
+ switch (opt) {
+ case 'b':
+ blk_file = g_strdup(optarg);
+ break;
+ case 's':
+ unix_socket = g_strdup(optarg);
+ break;
+ case 'h':
+ default:
+ printf("Usage: %s [-b block device or file, -s UNIX domain socket]"
+ " | [ -h ]\n", argv[0]);
+ return -1;
+ }
+ }
+
+ if (!unix_socket || !blk_file) {
+ printf("Usage: %s [-b block device or file, -s UNIX domain socket] |"
+ " [ -h ]\n", argv[0]);
+ return -1;
+ }
+
+ lsock = unix_sock_new(unix_socket);
+ if (lsock < 0) {
+ goto err;
+ }
+
+ csock = accept(lsock, (void *)0, (void *)0);
+ if (csock < 0) {
+ fprintf(stderr, "Accept error %s\n", strerror(errno));
+ goto err;
+ }
+
+ vdev_blk = g_new0(VubDev, 1);
+ vdev_blk->loop = g_main_loop_new(NULL, FALSE);
+
+ rc = vub_new_block(vdev_blk, blk_file);
+ if (rc) {
+ goto err;
+ }
+
+ vug_init(&vdev_blk->parent, csock, vub_panic_cb, &vub_iface);
+
+ g_main_loop_run(vdev_blk->loop);
+
+ vug_deinit(&vdev_blk->parent);
+
+err:
+ if (vdev_blk) {
+ g_main_loop_unref(vdev_blk->loop);
+ vub_deinit(vdev_blk);
+ g_free(vdev_blk);
+ }
+ if (csock >= 0) {
+ close(csock);
+ }
+ if (lsock >= 0) {
+ close(lsock);
+ }
+ g_free(unix_socket);
+ g_free(blk_file);
+
+ return 0;
+}
+
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application Changpeng Liu
@ 2017-12-20 16:02 ` Marc-André Lureau
2017-12-20 17:54 ` Michael S. Tsirkin
0 siblings, 1 reply; 16+ messages in thread
From: Marc-André Lureau @ 2017-12-20 16:02 UTC (permalink / raw)
To: Changpeng Liu
Cc: QEMU, james.r.harris, Michael S. Tsirkin, Stefan Hajnoczi,
Paolo Bonzini, Felipe Franciosi
Hi
On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> wrote:
> This commit introcudes a vhost-user-blk backend device, it uses UNIX
> domain socket to communicate with QEMU. The vhost-user-blk sample
> application should be used with QEMU vhost-user-blk-pci device.
>
> To use it, complie with:
> make vhost-user-blk
>
> and start like this:
> vhost-user-blk -b /dev/sdb -s /path/vhost.socket
>
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Looks ok overall, just a few code cleanups would be welcome,
> ---
> .gitignore | 1 +
> Makefile | 3 +
> Makefile.objs | 1 +
> contrib/vhost-user-blk/Makefile.objs | 1 +
> contrib/vhost-user-blk/vhost-user-blk.c | 550 ++++++++++++++++++++++++++++++++
> 5 files changed, 556 insertions(+)
> create mode 100644 contrib/vhost-user-blk/Makefile.objs
> create mode 100644 contrib/vhost-user-blk/vhost-user-blk.c
>
> diff --git a/.gitignore b/.gitignore
> index 588769b..495f854 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -55,6 +55,7 @@
> /scsi/qemu-pr-helper
> /vscclient
> /vhost-user-scsi
> +/vhost-user-blk
> /fsdev/virtfs-proxy-helper
> *.tmp
> *.[1-9]
> diff --git a/Makefile b/Makefile
> index ab0354c..3aad271 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -327,6 +327,7 @@ dummy := $(call unnest-vars,, \
> ivshmem-server-obj-y \
> libvhost-user-obj-y \
> vhost-user-scsi-obj-y \
> + vhost-user-blk-obj-y \
> qga-vss-dll-obj-y \
> block-obj-y \
> block-obj-m \
> @@ -558,6 +559,8 @@ ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
> endif
> vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y) libvhost-user.a
> $(call LINK, $^)
> +vhost-user-blk$(EXESUF): $(vhost-user-blk-obj-y) libvhost-user.a
> + $(call LINK, $^)
>
> module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
> $(call quiet-command,$(PYTHON) $< $@ \
> diff --git a/Makefile.objs b/Makefile.objs
> index 285c6f3..ae9aef7 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -115,6 +115,7 @@ libvhost-user-obj-y = contrib/libvhost-user/
> vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
> vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
> vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
> +vhost-user-blk-obj-y = contrib/vhost-user-blk/
>
> ######################################################################
> trace-events-subdirs =
> diff --git a/contrib/vhost-user-blk/Makefile.objs b/contrib/vhost-user-blk/Makefile.objs
> new file mode 100644
> index 0000000..72e2cdc
> --- /dev/null
> +++ b/contrib/vhost-user-blk/Makefile.objs
> @@ -0,0 +1 @@
> +vhost-user-blk-obj-y = vhost-user-blk.o
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> new file mode 100644
> index 0000000..841c3c3
> --- /dev/null
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -0,0 +1,550 @@
> +/*
> + * vhost-user-blk sample application
> + *
> + * Copyright (c) 2017 Intel Corporation. All rights reserved.
> + *
> + * Author:
> + * Changpeng Liu <changpeng.liu@intel.com>
> + *
> + * This work is based on the "vhost-user-scsi" sample and "virtio-blk" driver
> + * implemention by:
> + * Felipe Franciosi <felipe@nutanix.com>
> + * Anthony Liguori <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 only.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "standard-headers/linux/virtio_blk.h"
> +#include "contrib/libvhost-user/libvhost-user-glib.h"
> +#include "contrib/libvhost-user/libvhost-user.h"
> +
> +#include <glib.h>
> +
> +struct virtio_blk_inhdr {
> + unsigned char status;
> +};
> +
> +/* vhost user block device */
> +typedef struct VubDev {
> + VugDev parent;
> + int blk_fd;
> + struct virtio_blk_config blkcfg;
> + char *blk_name;
> + GMainLoop *loop;
> +} VubDev;
> +
> +typedef struct VubReq {
> + VuVirtqElement *elem;
> + int64_t sector_num;
> + size_t size;
> + struct virtio_blk_inhdr *in;
> + struct virtio_blk_outhdr *out;
> + VubDev *vdev_blk;
> + struct VuVirtq *vq;
> +} VubReq;
> +
> +/** refer util/iov.c **/
> +static size_t vub_iov_size(const struct iovec *iov,
> + const unsigned int iov_cnt)
> +{
> + size_t len;
> + unsigned int i;
> +
> + len = 0;
> + for (i = 0; i < iov_cnt; i++) {
> + len += iov[i].iov_len;
> + }
> + return len;
> +}
> +
> +static void vub_panic_cb(VuDev *vu_dev, const char *buf)
> +{
> + VugDev *gdev;
> + VubDev *vdev_blk;
> +
> + assert(vu_dev);
> +
> + gdev = container_of(vu_dev, VugDev, parent);
> + vdev_blk = container_of(gdev, VubDev, parent);
> + if (buf) {
> + g_warning("vu_panic: %s", buf);
> + }
> +
> + g_main_loop_quit(vdev_blk->loop);
> +}
> +
> +static void vub_req_complete(VubReq *req)
> +{
> + VugDev *gdev = &req->vdev_blk->parent;
> + VuDev *vu_dev = &gdev->parent;
> +
> + /* IO size with 1 extra status byte */
> + vu_queue_push(vu_dev, req->vq, req->elem,
> + req->size + 1);
> + vu_queue_notify(vu_dev, req->vq);
> +
> + if (req->elem) {
> + free(req->elem);
> + }
> + g_free(req);
> +}
> +
> +static int vub_open(const char *file_name, bool wce)
> +{
> + int fd;
> + int flags = O_RDWR;
> +
> + if (!wce) {
> + flags |= O_DIRECT;
> + }
> +
> + fd = open(file_name, flags);
> + if (fd < 0) {
> + fprintf(stderr, "Cannot open file %s, %s\n", file_name,
> + strerror(errno));
> + return -1;
> + }
> +
> + return fd;
> +}
> +
> +static void vub_close(int fd)
> +{
> + if (fd >= 0) {
> + close(fd);
> + }
> +}
Not sure that function is worth it, see below:
> +
> +static ssize_t
> +vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> +{
> + VubDev *vdev_blk = req->vdev_blk;
> + ssize_t rc;
> +
> + if (!iovcnt) {
> + fprintf(stderr, "Invalid Read IOV count\n");
> + return -1;
> + }
> +
> + req->size = vub_iov_size(iov, iovcnt);
> + rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
> + if (rc < 0) {
> + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> + vdev_blk->blk_name, req->sector_num, req->size,
> + strerror(errno));
> + return -1;
> + }
> +
> + return rc;
> +}
> +
> +static ssize_t
> +vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> +{
> + VubDev *vdev_blk = req->vdev_blk;
> + ssize_t rc;
> +
> + if (!iovcnt) {
> + fprintf(stderr, "Invalid Write IOV count\n");
> + return -1;
> + }
> +
> + req->size = vub_iov_size(iov, iovcnt);
> + rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
> + if (rc < 0) {
> + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> + vdev_blk->blk_name, req->sector_num, req->size,
> + strerror(errno));
> + return -1;
> + }
> +
> + return rc;
> +}
> +
> +static void
> +vub_flush(VubReq *req)
> +{
> + VubDev *vdev_blk = req->vdev_blk;
> +
> + fdatasync(vdev_blk->blk_fd);
> +}
> +
> +
2 lines?
> +static int vub_virtio_process_req(VubDev *vdev_blk,
> + VuVirtq *vq)
> +{
> + VugDev *gdev = &vdev_blk->parent;
> + VuDev *vu_dev = &gdev->parent;
> + VuVirtqElement *elem;
> + uint32_t type;
> + unsigned in_num;
> + unsigned out_num;
> + VubReq *req;
> +
> + elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement));
> + if (!elem) {
> + return -1;
> + }
> +
> + /* refer to hw/block/virtio_blk.c */
> + if (elem->out_num < 1 || elem->in_num < 1) {
> + fprintf(stderr, "virtio-blk request missing headers\n");
> + free(elem);
> + return -1;
> + }
> +
> + req = g_new0(VubReq, 1);
> + req->vdev_blk = vdev_blk;
> + req->vq = vq;
> + req->elem = elem;
> +
> + in_num = elem->in_num;
> + out_num = elem->out_num;
> +
> + /* don't support VIRTIO_F_ANY_LAYOUT and virtio 1.0 only */
> + if (elem->out_sg[0].iov_len < sizeof(struct virtio_blk_outhdr)) {
> + fprintf(stderr, "Invalid outhdr size\n");
> + goto err;
> + }
> + req->out = (struct virtio_blk_outhdr *)elem->out_sg[0].iov_base;
> + out_num--;
> +
> + if (elem->in_sg[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> + fprintf(stderr, "Invalid inhdr size\n");
> + goto err;
> + }
> + req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
> + in_num--;
> +
> + type = le32toh(req->out->type);
> + switch (type & ~(VIRTIO_BLK_T_OUT | VIRTIO_BLK_T_BARRIER)) {
> + case VIRTIO_BLK_T_IN: {
> + ssize_t ret = 0;
> + bool is_write = type & VIRTIO_BLK_T_OUT;
> + req->sector_num = le64toh(req->out->sector);
> + if (is_write) {
> + ret = vub_writev(req, &elem->out_sg[1], out_num);
> + } else {
> + ret = vub_readv(req, &elem->in_sg[0], in_num);
> + }
> + if (ret >= 0) {
> + req->in->status = VIRTIO_BLK_S_OK;
> + } else {
> + req->in->status = VIRTIO_BLK_S_IOERR;
> + }
> + vub_req_complete(req);
> + break;
> + }
> + case VIRTIO_BLK_T_FLUSH: {
> + vub_flush(req);
> + req->in->status = VIRTIO_BLK_S_OK;
> + vub_req_complete(req);
> + break;
> + }
> + case VIRTIO_BLK_T_GET_ID: {
> + size_t size = MIN(vub_iov_size(&elem->in_sg[0], in_num),
> + VIRTIO_BLK_ID_BYTES);
> + snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk");
> + req->in->status = VIRTIO_BLK_S_OK;
> + req->size = elem->in_sg[0].iov_len;
> + vub_req_complete(req);
> + break;
> + }
> + default: {
> + req->in->status = VIRTIO_BLK_S_UNSUPP;
> + vub_req_complete(req);
> + break;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + free(elem);
> + g_free(req);
> + return -1;
> +}
> +
> +static void vub_process_vq(VuDev *vu_dev, int idx)
> +{
> + VugDev *gdev;
> + VubDev *vdev_blk;
> + VuVirtq *vq;
> + int ret;
> +
> + if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
> + fprintf(stderr, "VQ Index out of range: %d\n", idx);
> + vub_panic_cb(vu_dev, NULL);
> + return;
> + }
> +
> + gdev = container_of(vu_dev, VugDev, parent);
> + vdev_blk = container_of(gdev, VubDev, parent);
> + assert(vdev_blk);
> +
> + vq = vu_get_queue(vu_dev, idx);
> + assert(vq);
> +
> + while (1) {
> + ret = vub_virtio_process_req(vdev_blk, vq);
> + if (ret) {
> + break;
> + }
> + }
> +}
> +
> +static void vub_queue_set_started(VuDev *vu_dev, int idx, bool started)
> +{
> + VuVirtq *vq;
> +
> + assert(vu_dev);
> +
> + vq = vu_get_queue(vu_dev, idx);
> + vu_set_queue_handler(vu_dev, vq, started ? vub_process_vq : NULL);
> +}
> +
> +static uint64_t
> +vub_get_features(VuDev *dev)
> +{
> + return 1ull << VIRTIO_BLK_F_SIZE_MAX |
> + 1ull << VIRTIO_BLK_F_SEG_MAX |
> + 1ull << VIRTIO_BLK_F_TOPOLOGY |
> + 1ull << VIRTIO_BLK_F_BLK_SIZE |
> + 1ull << VIRTIO_BLK_F_FLUSH |
> + 1ull << VIRTIO_BLK_F_CONFIG_WCE |
> + 1ull << VIRTIO_F_VERSION_1 |
> + 1ull << VHOST_USER_F_PROTOCOL_FEATURES;
> +}
> +
> +static int
> +vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
> +{
> + VugDev *gdev;
> + VubDev *vdev_blk;
> +
> + gdev = container_of(vu_dev, VugDev, parent);
> + vdev_blk = container_of(gdev, VubDev, parent);
> + memcpy(config, &vdev_blk->blkcfg, len);
> +
> + return 0;
> +}
> +
> +static int
> +vub_set_config(VuDev *vu_dev, const uint8_t *data,
> + uint32_t offset, uint32_t size, uint32_t flags)
> +{
> + VugDev *gdev;
> + VubDev *vdev_blk;
> + uint8_t wce;
> + int fd;
> +
> + /* don't support live migration */
> + if (flags != VHOST_SET_CONFIG_TYPE_MASTER) {
> + return -1;
> + }
> +
> + gdev = container_of(vu_dev, VugDev, parent);
> + vdev_blk = container_of(gdev, VubDev, parent);
> +
> + if (offset != offsetof(struct virtio_blk_config, wce) ||
> + size != 1) {
> + return -1;
> + }
> +
> + wce = *data;
> + if (wce == vdev_blk->blkcfg.wce) {
> + /* Do nothing as same with old configuration */
> + return 0;
> + }
> +
> + vdev_blk->blkcfg.wce = wce;
> + fprintf(stdout, "Write Cache Policy Changed\n");
> + vub_close(vdev_blk->blk_fd);
if (vdev_blk->blk_fd >= 0)
close(vdev_blk->blk_fd);
vdev_blk->blk_fd = -1;
> +
> + fd = vub_open(vdev_blk->blk_name, wce);
> + if (fd < 0) {
> + fprintf(stderr, "Error to open block device %s\n", vdev_blk->blk_name);
> + vdev_blk->blk_fd = -1;
> + return -1;
> + }
> + vdev_blk->blk_fd = fd;
> +
> + return 0;
> +}
> +
> +static const VuDevIface vub_iface = {
> + .get_features = vub_get_features,
> + .queue_set_started = vub_queue_set_started,
> + .get_config = vub_get_config,
> + .set_config = vub_set_config,
> +};
> +
> +static int unix_sock_new(char *unix_fn)
> +{
> + int sock;
> + struct sockaddr_un un;
> + size_t len;
> +
> + assert(unix_fn);
> +
> + sock = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (sock <= 0) {
> + perror("socket");
> + return -1;
> + }
> +
> + un.sun_family = AF_UNIX;
> + (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
> + len = sizeof(un.sun_family) + strlen(un.sun_path);
> +
> + (void)unlink(unix_fn);
> + if (bind(sock, (struct sockaddr *)&un, len) < 0) {
> + perror("bind");
> + goto fail;
> + }
> +
> + if (listen(sock, 1) < 0) {
> + perror("listen");
> + goto fail;
> + }
> +
> + return sock;
> +
> +fail:
> + (void)close(sock);
> +
> + return -1;
> +}
> +
> +static void vub_deinit(struct VubDev *vdev_blk)
> +{
> + if (!vdev_blk) {
> + return;
> + }
Make it vub_free() to remove the g_free() after the call below?
> +
> + if (vdev_blk->blk_fd) {
> + vub_close(vdev_blk->blk_fd);
if (vdev_blk->blk_fd >= 0)
close(vdev_blk->blk_fd)
& remove vub_close()
> + }
> +}
> +
> +static uint32_t
> +vub_get_blocksize(int fd)
> +{
> + uint32_t blocksize = 512;
> +
> + #if defined(__linux__) && defined(BLKSSZGET)
> + if (ioctl(fd, BLKSSZGET, &blocksize) == 0) {
> + return blocklen;
> + }
> + #endif
> +
> + return blocksize;
> +}
> +
> +static void
> +vub_initialize_config(int fd, struct virtio_blk_config *config)
> +{
> + off64_t capacity;
> +
> + capacity = lseek64(fd, 0, SEEK_END);
> + config->capacity = capacity >> 9;
> + config->blk_size = vub_get_blocksize(fd);
> + config->size_max = 65536;
> + config->seg_max = 128 - 2;
> + config->min_io_size = 1;
> + config->opt_io_size = 1;
> + config->num_queues = 1;
> +}
> +
> +static int
> +vub_new_block(VubDev *vdev_blk, char *blk_file)
Make it vub_new() instead?
> +{
> + vdev_blk->blk_fd = vub_open(blk_file, 0);
> + if (vdev_blk->blk_fd < 0) {
> + fprintf(stderr, "Error to open block device %s\n", blk_file);
> + return -1;
> + }
> + vdev_blk->blkcfg.wce = 0;
> + vdev_blk->blk_name = blk_file;
> +
> + /* fill virtio_blk_config with block parameters */
> + vub_initialize_config(vdev_blk->blk_fd, &vdev_blk->blkcfg);
> +
> + return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int opt;
> + char *unix_socket = NULL;
> + char *blk_file = NULL;
> + int lsock = -1, csock = -1, rc;
> + VubDev *vdev_blk = NULL;
> +
> + while ((opt = getopt(argc, argv, "b:h:s:")) != -1) {
> + switch (opt) {
> + case 'b':
> + blk_file = g_strdup(optarg);
> + break;
> + case 's':
> + unix_socket = g_strdup(optarg);
> + break;
> + case 'h':
> + default:
> + printf("Usage: %s [-b block device or file, -s UNIX domain socket]"
> + " | [ -h ]\n", argv[0]);
> + return -1;
> + }
> + }
> +
> + if (!unix_socket || !blk_file) {
> + printf("Usage: %s [-b block device or file, -s UNIX domain socket] |"
> + " [ -h ]\n", argv[0]);
> + return -1;
> + }
> +
> + lsock = unix_sock_new(unix_socket);
> + if (lsock < 0) {
> + goto err;
> + }
> +
> + csock = accept(lsock, (void *)0, (void *)0);
> + if (csock < 0) {
> + fprintf(stderr, "Accept error %s\n", strerror(errno));
> + goto err;
> + }
> +
> + vdev_blk = g_new0(VubDev, 1);
> + vdev_blk->loop = g_main_loop_new(NULL, FALSE);
> +
> + rc = vub_new_block(vdev_blk, blk_file);
The 3 lines above could be simplified to be:
vub = vub_new(blk_file);
> + if (rc) {
> + goto err;
> + }
> +
> + vug_init(&vdev_blk->parent, csock, vub_panic_cb, &vub_iface);
> +
> + g_main_loop_run(vdev_blk->loop);
> +
> + vug_deinit(&vdev_blk->parent);
> +
> +err:
> + if (vdev_blk) {
> + g_main_loop_unref(vdev_blk->loop);
> + vub_deinit(vdev_blk);
> + g_free(vdev_blk);
> + }
And the above block could simply be:
and this could simply be:
vub_free(vub)
> + if (csock >= 0) {
> + close(csock);
> + }
> + if (lsock >= 0) {
> + close(lsock);
> + }
> + g_free(unix_socket);
> + g_free(blk_file);
> +
> + return 0;
> +}
> +
Please remove that extra empty line
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application
2017-12-20 16:02 ` Marc-André Lureau
@ 2017-12-20 17:54 ` Michael S. Tsirkin
0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2017-12-20 17:54 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Changpeng Liu, QEMU, james.r.harris, Stefan Hajnoczi,
Paolo Bonzini, Felipe Franciosi
On Wed, Dec 20, 2017 at 05:02:18PM +0100, Marc-André Lureau wrote:
> Hi
>
> On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng.liu@intel.com> wrote:
> > This commit introcudes a vhost-user-blk backend device, it uses UNIX
> > domain socket to communicate with QEMU. The vhost-user-blk sample
> > application should be used with QEMU vhost-user-blk-pci device.
> >
> > To use it, complie with:
> > make vhost-user-blk
> >
> > and start like this:
> > vhost-user-blk -b /dev/sdb -s /path/vhost.socket
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>
> Looks ok overall, just a few code cleanups would be welcome,
I merged it by now, pls do cleanups by adding patches on top.
> > ---
> > .gitignore | 1 +
> > Makefile | 3 +
> > Makefile.objs | 1 +
> > contrib/vhost-user-blk/Makefile.objs | 1 +
> > contrib/vhost-user-blk/vhost-user-blk.c | 550 ++++++++++++++++++++++++++++++++
> > 5 files changed, 556 insertions(+)
> > create mode 100644 contrib/vhost-user-blk/Makefile.objs
> > create mode 100644 contrib/vhost-user-blk/vhost-user-blk.c
> >
> > diff --git a/.gitignore b/.gitignore
> > index 588769b..495f854 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -55,6 +55,7 @@
> > /scsi/qemu-pr-helper
> > /vscclient
> > /vhost-user-scsi
> > +/vhost-user-blk
> > /fsdev/virtfs-proxy-helper
> > *.tmp
> > *.[1-9]
> > diff --git a/Makefile b/Makefile
> > index ab0354c..3aad271 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -327,6 +327,7 @@ dummy := $(call unnest-vars,, \
> > ivshmem-server-obj-y \
> > libvhost-user-obj-y \
> > vhost-user-scsi-obj-y \
> > + vhost-user-blk-obj-y \
> > qga-vss-dll-obj-y \
> > block-obj-y \
> > block-obj-m \
> > @@ -558,6 +559,8 @@ ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
> > endif
> > vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y) libvhost-user.a
> > $(call LINK, $^)
> > +vhost-user-blk$(EXESUF): $(vhost-user-blk-obj-y) libvhost-user.a
> > + $(call LINK, $^)
> >
> > module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
> > $(call quiet-command,$(PYTHON) $< $@ \
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 285c6f3..ae9aef7 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -115,6 +115,7 @@ libvhost-user-obj-y = contrib/libvhost-user/
> > vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
> > vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
> > vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
> > +vhost-user-blk-obj-y = contrib/vhost-user-blk/
> >
> > ######################################################################
> > trace-events-subdirs =
> > diff --git a/contrib/vhost-user-blk/Makefile.objs b/contrib/vhost-user-blk/Makefile.objs
> > new file mode 100644
> > index 0000000..72e2cdc
> > --- /dev/null
> > +++ b/contrib/vhost-user-blk/Makefile.objs
> > @@ -0,0 +1 @@
> > +vhost-user-blk-obj-y = vhost-user-blk.o
> > diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> > new file mode 100644
> > index 0000000..841c3c3
> > --- /dev/null
> > +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> > @@ -0,0 +1,550 @@
> > +/*
> > + * vhost-user-blk sample application
> > + *
> > + * Copyright (c) 2017 Intel Corporation. All rights reserved.
> > + *
> > + * Author:
> > + * Changpeng Liu <changpeng.liu@intel.com>
> > + *
> > + * This work is based on the "vhost-user-scsi" sample and "virtio-blk" driver
> > + * implemention by:
> > + * Felipe Franciosi <felipe@nutanix.com>
> > + * Anthony Liguori <aliguori@us.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 only.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "standard-headers/linux/virtio_blk.h"
> > +#include "contrib/libvhost-user/libvhost-user-glib.h"
> > +#include "contrib/libvhost-user/libvhost-user.h"
> > +
> > +#include <glib.h>
> > +
> > +struct virtio_blk_inhdr {
> > + unsigned char status;
> > +};
> > +
> > +/* vhost user block device */
> > +typedef struct VubDev {
> > + VugDev parent;
> > + int blk_fd;
> > + struct virtio_blk_config blkcfg;
> > + char *blk_name;
> > + GMainLoop *loop;
> > +} VubDev;
> > +
> > +typedef struct VubReq {
> > + VuVirtqElement *elem;
> > + int64_t sector_num;
> > + size_t size;
> > + struct virtio_blk_inhdr *in;
> > + struct virtio_blk_outhdr *out;
> > + VubDev *vdev_blk;
> > + struct VuVirtq *vq;
> > +} VubReq;
> > +
> > +/** refer util/iov.c **/
> > +static size_t vub_iov_size(const struct iovec *iov,
> > + const unsigned int iov_cnt)
> > +{
> > + size_t len;
> > + unsigned int i;
> > +
> > + len = 0;
> > + for (i = 0; i < iov_cnt; i++) {
> > + len += iov[i].iov_len;
> > + }
> > + return len;
> > +}
> > +
> > +static void vub_panic_cb(VuDev *vu_dev, const char *buf)
> > +{
> > + VugDev *gdev;
> > + VubDev *vdev_blk;
> > +
> > + assert(vu_dev);
> > +
> > + gdev = container_of(vu_dev, VugDev, parent);
> > + vdev_blk = container_of(gdev, VubDev, parent);
> > + if (buf) {
> > + g_warning("vu_panic: %s", buf);
> > + }
> > +
> > + g_main_loop_quit(vdev_blk->loop);
> > +}
> > +
> > +static void vub_req_complete(VubReq *req)
> > +{
> > + VugDev *gdev = &req->vdev_blk->parent;
> > + VuDev *vu_dev = &gdev->parent;
> > +
> > + /* IO size with 1 extra status byte */
> > + vu_queue_push(vu_dev, req->vq, req->elem,
> > + req->size + 1);
> > + vu_queue_notify(vu_dev, req->vq);
> > +
> > + if (req->elem) {
> > + free(req->elem);
> > + }
> > + g_free(req);
> > +}
> > +
> > +static int vub_open(const char *file_name, bool wce)
> > +{
> > + int fd;
> > + int flags = O_RDWR;
> > +
> > + if (!wce) {
> > + flags |= O_DIRECT;
> > + }
> > +
> > + fd = open(file_name, flags);
> > + if (fd < 0) {
> > + fprintf(stderr, "Cannot open file %s, %s\n", file_name,
> > + strerror(errno));
> > + return -1;
> > + }
> > +
> > + return fd;
> > +}
> > +
> > +static void vub_close(int fd)
> > +{
> > + if (fd >= 0) {
> > + close(fd);
> > + }
> > +}
>
> Not sure that function is worth it, see below:
>
> > +
> > +static ssize_t
> > +vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> > +{
> > + VubDev *vdev_blk = req->vdev_blk;
> > + ssize_t rc;
> > +
> > + if (!iovcnt) {
> > + fprintf(stderr, "Invalid Read IOV count\n");
> > + return -1;
> > + }
> > +
> > + req->size = vub_iov_size(iov, iovcnt);
> > + rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
> > + if (rc < 0) {
> > + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> > + vdev_blk->blk_name, req->sector_num, req->size,
> > + strerror(errno));
> > + return -1;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static ssize_t
> > +vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> > +{
> > + VubDev *vdev_blk = req->vdev_blk;
> > + ssize_t rc;
> > +
> > + if (!iovcnt) {
> > + fprintf(stderr, "Invalid Write IOV count\n");
> > + return -1;
> > + }
> > +
> > + req->size = vub_iov_size(iov, iovcnt);
> > + rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
> > + if (rc < 0) {
> > + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> > + vdev_blk->blk_name, req->sector_num, req->size,
> > + strerror(errno));
> > + return -1;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static void
> > +vub_flush(VubReq *req)
> > +{
> > + VubDev *vdev_blk = req->vdev_blk;
> > +
> > + fdatasync(vdev_blk->blk_fd);
> > +}
> > +
> > +
>
> 2 lines?
>
> > +static int vub_virtio_process_req(VubDev *vdev_blk,
> > + VuVirtq *vq)
> > +{
> > + VugDev *gdev = &vdev_blk->parent;
> > + VuDev *vu_dev = &gdev->parent;
> > + VuVirtqElement *elem;
> > + uint32_t type;
> > + unsigned in_num;
> > + unsigned out_num;
> > + VubReq *req;
> > +
> > + elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement));
> > + if (!elem) {
> > + return -1;
> > + }
> > +
> > + /* refer to hw/block/virtio_blk.c */
> > + if (elem->out_num < 1 || elem->in_num < 1) {
> > + fprintf(stderr, "virtio-blk request missing headers\n");
> > + free(elem);
> > + return -1;
> > + }
> > +
> > + req = g_new0(VubReq, 1);
> > + req->vdev_blk = vdev_blk;
> > + req->vq = vq;
> > + req->elem = elem;
> > +
> > + in_num = elem->in_num;
> > + out_num = elem->out_num;
> > +
> > + /* don't support VIRTIO_F_ANY_LAYOUT and virtio 1.0 only */
> > + if (elem->out_sg[0].iov_len < sizeof(struct virtio_blk_outhdr)) {
> > + fprintf(stderr, "Invalid outhdr size\n");
> > + goto err;
> > + }
> > + req->out = (struct virtio_blk_outhdr *)elem->out_sg[0].iov_base;
> > + out_num--;
> > +
> > + if (elem->in_sg[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> > + fprintf(stderr, "Invalid inhdr size\n");
> > + goto err;
> > + }
> > + req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
> > + in_num--;
> > +
> > + type = le32toh(req->out->type);
> > + switch (type & ~(VIRTIO_BLK_T_OUT | VIRTIO_BLK_T_BARRIER)) {
> > + case VIRTIO_BLK_T_IN: {
> > + ssize_t ret = 0;
> > + bool is_write = type & VIRTIO_BLK_T_OUT;
> > + req->sector_num = le64toh(req->out->sector);
> > + if (is_write) {
> > + ret = vub_writev(req, &elem->out_sg[1], out_num);
> > + } else {
> > + ret = vub_readv(req, &elem->in_sg[0], in_num);
> > + }
> > + if (ret >= 0) {
> > + req->in->status = VIRTIO_BLK_S_OK;
> > + } else {
> > + req->in->status = VIRTIO_BLK_S_IOERR;
> > + }
> > + vub_req_complete(req);
> > + break;
> > + }
> > + case VIRTIO_BLK_T_FLUSH: {
> > + vub_flush(req);
> > + req->in->status = VIRTIO_BLK_S_OK;
> > + vub_req_complete(req);
> > + break;
> > + }
> > + case VIRTIO_BLK_T_GET_ID: {
> > + size_t size = MIN(vub_iov_size(&elem->in_sg[0], in_num),
> > + VIRTIO_BLK_ID_BYTES);
> > + snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk");
> > + req->in->status = VIRTIO_BLK_S_OK;
> > + req->size = elem->in_sg[0].iov_len;
> > + vub_req_complete(req);
> > + break;
> > + }
> > + default: {
> > + req->in->status = VIRTIO_BLK_S_UNSUPP;
> > + vub_req_complete(req);
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + free(elem);
> > + g_free(req);
> > + return -1;
> > +}
> > +
> > +static void vub_process_vq(VuDev *vu_dev, int idx)
> > +{
> > + VugDev *gdev;
> > + VubDev *vdev_blk;
> > + VuVirtq *vq;
> > + int ret;
> > +
> > + if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
> > + fprintf(stderr, "VQ Index out of range: %d\n", idx);
> > + vub_panic_cb(vu_dev, NULL);
> > + return;
> > + }
> > +
> > + gdev = container_of(vu_dev, VugDev, parent);
> > + vdev_blk = container_of(gdev, VubDev, parent);
> > + assert(vdev_blk);
> > +
> > + vq = vu_get_queue(vu_dev, idx);
> > + assert(vq);
> > +
> > + while (1) {
> > + ret = vub_virtio_process_req(vdev_blk, vq);
> > + if (ret) {
> > + break;
> > + }
> > + }
> > +}
> > +
> > +static void vub_queue_set_started(VuDev *vu_dev, int idx, bool started)
> > +{
> > + VuVirtq *vq;
> > +
> > + assert(vu_dev);
> > +
> > + vq = vu_get_queue(vu_dev, idx);
> > + vu_set_queue_handler(vu_dev, vq, started ? vub_process_vq : NULL);
> > +}
> > +
> > +static uint64_t
> > +vub_get_features(VuDev *dev)
> > +{
> > + return 1ull << VIRTIO_BLK_F_SIZE_MAX |
> > + 1ull << VIRTIO_BLK_F_SEG_MAX |
> > + 1ull << VIRTIO_BLK_F_TOPOLOGY |
> > + 1ull << VIRTIO_BLK_F_BLK_SIZE |
> > + 1ull << VIRTIO_BLK_F_FLUSH |
> > + 1ull << VIRTIO_BLK_F_CONFIG_WCE |
> > + 1ull << VIRTIO_F_VERSION_1 |
> > + 1ull << VHOST_USER_F_PROTOCOL_FEATURES;
> > +}
> > +
> > +static int
> > +vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
> > +{
> > + VugDev *gdev;
> > + VubDev *vdev_blk;
> > +
> > + gdev = container_of(vu_dev, VugDev, parent);
> > + vdev_blk = container_of(gdev, VubDev, parent);
> > + memcpy(config, &vdev_blk->blkcfg, len);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +vub_set_config(VuDev *vu_dev, const uint8_t *data,
> > + uint32_t offset, uint32_t size, uint32_t flags)
> > +{
> > + VugDev *gdev;
> > + VubDev *vdev_blk;
> > + uint8_t wce;
> > + int fd;
> > +
> > + /* don't support live migration */
> > + if (flags != VHOST_SET_CONFIG_TYPE_MASTER) {
> > + return -1;
> > + }
> > +
> > + gdev = container_of(vu_dev, VugDev, parent);
> > + vdev_blk = container_of(gdev, VubDev, parent);
> > +
> > + if (offset != offsetof(struct virtio_blk_config, wce) ||
> > + size != 1) {
> > + return -1;
> > + }
> > +
> > + wce = *data;
> > + if (wce == vdev_blk->blkcfg.wce) {
> > + /* Do nothing as same with old configuration */
> > + return 0;
> > + }
> > +
> > + vdev_blk->blkcfg.wce = wce;
> > + fprintf(stdout, "Write Cache Policy Changed\n");
> > + vub_close(vdev_blk->blk_fd);
>
> if (vdev_blk->blk_fd >= 0)
> close(vdev_blk->blk_fd);
> vdev_blk->blk_fd = -1;
>
> > +
> > + fd = vub_open(vdev_blk->blk_name, wce);
> > + if (fd < 0) {
> > + fprintf(stderr, "Error to open block device %s\n", vdev_blk->blk_name);
> > + vdev_blk->blk_fd = -1;
> > + return -1;
> > + }
> > + vdev_blk->blk_fd = fd;
> > +
> > + return 0;
> > +}
> > +
> > +static const VuDevIface vub_iface = {
> > + .get_features = vub_get_features,
> > + .queue_set_started = vub_queue_set_started,
> > + .get_config = vub_get_config,
> > + .set_config = vub_set_config,
> > +};
> > +
> > +static int unix_sock_new(char *unix_fn)
> > +{
> > + int sock;
> > + struct sockaddr_un un;
> > + size_t len;
> > +
> > + assert(unix_fn);
> > +
> > + sock = socket(AF_UNIX, SOCK_STREAM, 0);
> > + if (sock <= 0) {
> > + perror("socket");
> > + return -1;
> > + }
> > +
> > + un.sun_family = AF_UNIX;
> > + (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
> > + len = sizeof(un.sun_family) + strlen(un.sun_path);
> > +
> > + (void)unlink(unix_fn);
> > + if (bind(sock, (struct sockaddr *)&un, len) < 0) {
> > + perror("bind");
> > + goto fail;
> > + }
> > +
> > + if (listen(sock, 1) < 0) {
> > + perror("listen");
> > + goto fail;
> > + }
> > +
> > + return sock;
> > +
> > +fail:
> > + (void)close(sock);
> > +
> > + return -1;
> > +}
> > +
> > +static void vub_deinit(struct VubDev *vdev_blk)
> > +{
> > + if (!vdev_blk) {
> > + return;
> > + }
>
> Make it vub_free() to remove the g_free() after the call below?
>
> > +
> > + if (vdev_blk->blk_fd) {
> > + vub_close(vdev_blk->blk_fd);
>
> if (vdev_blk->blk_fd >= 0)
> close(vdev_blk->blk_fd)
>
> & remove vub_close()
>
> > + }
> > +}
> > +
> > +static uint32_t
> > +vub_get_blocksize(int fd)
> > +{
> > + uint32_t blocksize = 512;
> > +
> > + #if defined(__linux__) && defined(BLKSSZGET)
> > + if (ioctl(fd, BLKSSZGET, &blocksize) == 0) {
> > + return blocklen;
> > + }
> > + #endif
> > +
> > + return blocksize;
> > +}
> > +
> > +static void
> > +vub_initialize_config(int fd, struct virtio_blk_config *config)
> > +{
> > + off64_t capacity;
> > +
> > + capacity = lseek64(fd, 0, SEEK_END);
> > + config->capacity = capacity >> 9;
> > + config->blk_size = vub_get_blocksize(fd);
> > + config->size_max = 65536;
> > + config->seg_max = 128 - 2;
> > + config->min_io_size = 1;
> > + config->opt_io_size = 1;
> > + config->num_queues = 1;
> > +}
> > +
> > +static int
> > +vub_new_block(VubDev *vdev_blk, char *blk_file)
>
> Make it vub_new() instead?
>
> > +{
> > + vdev_blk->blk_fd = vub_open(blk_file, 0);
> > + if (vdev_blk->blk_fd < 0) {
> > + fprintf(stderr, "Error to open block device %s\n", blk_file);
> > + return -1;
> > + }
> > + vdev_blk->blkcfg.wce = 0;
> > + vdev_blk->blk_name = blk_file;
> > +
> > + /* fill virtio_blk_config with block parameters */
> > + vub_initialize_config(vdev_blk->blk_fd, &vdev_blk->blkcfg);
> > +
> > + return 0;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + int opt;
> > + char *unix_socket = NULL;
> > + char *blk_file = NULL;
> > + int lsock = -1, csock = -1, rc;
> > + VubDev *vdev_blk = NULL;
> > +
> > + while ((opt = getopt(argc, argv, "b:h:s:")) != -1) {
> > + switch (opt) {
> > + case 'b':
> > + blk_file = g_strdup(optarg);
> > + break;
> > + case 's':
> > + unix_socket = g_strdup(optarg);
> > + break;
> > + case 'h':
> > + default:
> > + printf("Usage: %s [-b block device or file, -s UNIX domain socket]"
> > + " | [ -h ]\n", argv[0]);
> > + return -1;
> > + }
> > + }
> > +
> > + if (!unix_socket || !blk_file) {
> > + printf("Usage: %s [-b block device or file, -s UNIX domain socket] |"
> > + " [ -h ]\n", argv[0]);
> > + return -1;
> > + }
> > +
> > + lsock = unix_sock_new(unix_socket);
> > + if (lsock < 0) {
> > + goto err;
> > + }
> > +
> > + csock = accept(lsock, (void *)0, (void *)0);
> > + if (csock < 0) {
> > + fprintf(stderr, "Accept error %s\n", strerror(errno));
> > + goto err;
> > + }
> > +
> > + vdev_blk = g_new0(VubDev, 1);
> > + vdev_blk->loop = g_main_loop_new(NULL, FALSE);
> > +
> > + rc = vub_new_block(vdev_blk, blk_file);
>
> The 3 lines above could be simplified to be:
>
> vub = vub_new(blk_file);
>
> > + if (rc) {
> > + goto err;
> > + }
> > +
> > + vug_init(&vdev_blk->parent, csock, vub_panic_cb, &vub_iface);
> > +
> > + g_main_loop_run(vdev_blk->loop);
> > +
> > + vug_deinit(&vdev_blk->parent);
> > +
> > +err:
> > + if (vdev_blk) {
> > + g_main_loop_unref(vdev_blk->loop);
> > + vub_deinit(vdev_blk);
> > + g_free(vdev_blk);
> > + }
>
> And the above block could simply be:
>
> and this could simply be:
>
> vub_free(vub)
>
> > + if (csock >= 0) {
> > + close(csock);
> > + }
> > + if (lsock >= 0) {
> > + close(lsock);
> > + }
> > + g_free(unix_socket);
> > + g_free(blk_file);
> > +
> > + return 0;
> > +}
> > +
>
> Please remove that extra empty line
>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
` (3 preceding siblings ...)
2017-12-13 2:29 ` [Qemu-devel] [PATCH v7 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application Changpeng Liu
@ 2017-12-13 10:22 ` Stefan Hajnoczi
2017-12-13 10:24 ` Stefan Hajnoczi
5 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2017-12-13 10:22 UTC (permalink / raw)
To: Changpeng Liu
Cc: qemu-devel, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]
On Wed, Dec 13, 2017 at 10:28:59AM +0800, Changpeng Liu wrote:
> Although virtio scsi specification was designed as a replacement for virtio_blk,
> there are still many users using virtio_blk. QEMU 2.9 introduced a new device
> vhost user scsi which can process I/O in user space for virtio_scsi, this commit
> introduces a new vhost user block host device, which can support virtio_blk in
> Guest OS, and I/O processing in another I/O target.
>
> Due to the limitation for virtio_blk specification, virtio_blk device cannot get
> block information such as capacity, block size etc via the specification, several
> new vhost user messages were added to deliver virtio config space
> information between Qemu and I/O target, VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG
> messages used for get/set config space from/to I/O target, VHOST_USER_SET_CONFIG_FD
> was added for event notifier in case the change of virtio config space. Also, those
> messages can be used for vhost device live migration as well.
>
> Changes:
> v6-v7: change the parameter of set configuration function let it only contain
> valid data buffer.
> v5-v6: add header flags for vhost-user master so that the slave can know the
> purpose for set config, also vhost-user get/set messages' payload doesn't
> contain invalid data buffers.
> v4-v5: add header offset and size for virtio config space.
> v3-v4: refactoring the vhost user block example patch based on new libvhost-user library.
> v2-v3: add new vhost user message to get/set virtio config space.
>
> Changpeng Liu (4):
> vhost-user: add new vhost user messages to support virtio config space
> vhost-user-blk: introduce a new vhost-user-blk host device
> contrib/libvhost-user: enable virtio config space messages
> contrib/vhost-user-blk: introduce a vhost-user-blk sample application
>
> .gitignore | 1 +
> Makefile | 3 +
> Makefile.objs | 1 +
> contrib/libvhost-user/libvhost-user.c | 80 +++++
> contrib/libvhost-user/libvhost-user.h | 36 ++-
> contrib/vhost-user-blk/Makefile.objs | 1 +
> contrib/vhost-user-blk/vhost-user-blk.c | 550 ++++++++++++++++++++++++++++++++
> default-configs/pci.mak | 1 +
> docs/interop/vhost-user.txt | 45 +++
> hw/block/Makefile.objs | 3 +
> hw/block/vhost-user-blk.c | 357 +++++++++++++++++++++
> hw/virtio/vhost-user.c | 107 +++++++
> hw/virtio/vhost.c | 64 ++++
> hw/virtio/virtio-pci.c | 55 ++++
> hw/virtio/virtio-pci.h | 18 ++
> include/hw/virtio/vhost-backend.h | 14 +
> include/hw/virtio/vhost-user-blk.h | 41 +++
> include/hw/virtio/vhost.h | 16 +
> 18 files changed, 1392 insertions(+), 1 deletion(-)
> create mode 100644 contrib/vhost-user-blk/Makefile.objs
> create mode 100644 contrib/vhost-user-blk/vhost-user-blk.c
> create mode 100644 hw/block/vhost-user-blk.c
> create mode 100644 include/hw/virtio/vhost-user-blk.h
>
> --
> 1.9.3
>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU
2017-12-13 2:28 [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Changpeng Liu
` (4 preceding siblings ...)
2017-12-13 10:22 ` [Qemu-devel] [PATCH v7 0/4] Introduce a new vhost-user-blk host device to QEMU Stefan Hajnoczi
@ 2017-12-13 10:24 ` Stefan Hajnoczi
5 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2017-12-13 10:24 UTC (permalink / raw)
To: Changpeng Liu
Cc: qemu-devel, pbonzini, mst, marcandre.lureau, felipe, james.r.harris
[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]
On Wed, Dec 13, 2017 at 10:28:59AM +0800, Changpeng Liu wrote:
> Although virtio scsi specification was designed as a replacement for virtio_blk,
> there are still many users using virtio_blk. QEMU 2.9 introduced a new device
> vhost user scsi which can process I/O in user space for virtio_scsi, this commit
> introduces a new vhost user block host device, which can support virtio_blk in
> Guest OS, and I/O processing in another I/O target.
>
> Due to the limitation for virtio_blk specification, virtio_blk device cannot get
> block information such as capacity, block size etc via the specification, several
> new vhost user messages were added to deliver virtio config space
> information between Qemu and I/O target, VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG
> messages used for get/set config space from/to I/O target, VHOST_USER_SET_CONFIG_FD
> was added for event notifier in case the change of virtio config space. Also, those
> messages can be used for vhost device live migration as well.
Michael or Marc-André, please take this through your tree once you've
reviewed it.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread