Re: [PATCH] vhost-user: fix the issue of vhost deadlock in nested virtualization
On 2/20/24 12:43, Michael S. Tsirkin wrote: On Tue, Feb 20, 2024 at 12:26:49PM +0100, Maxime Coquelin wrote: On 2/13/24 11:05, Michael S. Tsirkin wrote: On Fri, Jan 26, 2024 at 06:07:37PM +0800, Hao Chen wrote: I run "dpdk-vdpa" and "qemur-L2" in "qemu-L1". In a nested virtualization environment, "qemu-L2" vhost-user socket sends a "VHOST_USER_IOTLB_MSG" message to "dpdk-vdpa" and blocks waiting for "dpdk-vdpa" to process the message. If "dpdk-vdpa" doesn't complete the processing of the "VHOST_USER_IOTLB_MSG" message and sends a message that needs to be replied in another thread, such as "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG", "dpdk-vdpa" will also block and wait for "qemu-L2" to process this message. However, "qemu-L2" vhost-user's socket is blocking while waiting for a reply from "dpdk-vdpa" after processing the message "VHOSTr_USER_IOTLB_MSG", and "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG" will not be processed. In this case, both "dpdk-vdpa" and "qemu-L2" are blocked on the vhost read, resulting in a deadlock. You can modify "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG" or "VHOST_USER_IOTLB_MSG" to "no need reply" to fix this issue. There are too many messages in dpdk that are similar to "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG", and I would prefer the latter. Fixes: 24e34754eb78 ("vhost-user: factor out msg head and payload") Signed-off-by: Hao Chen I would be very worried that IOTLB becomes stale and guest memory is corrupted if we just proceed without waiting. Maxime what do you think? How would you address the issue? I agree with you, this is not possible. For example, in case of IOTLB invalidate, the frontend relies on the backend reply to ensure it is no more accessing the memory before proceeding. The reply-ack for VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG request is less important, if it fails the host notifications won't work but would not risk corruption. Maybe on Qemu side we could fail init if processing the request fails, as I think that if negotiated, we can expect it to succeed. What do you think about this proposal? Regards, Maxime Fundamentally, I think that if qemu blocks guest waiting for a rely that is ok but it really has to process incoming messages meanwhile. Same should apply to backend I think ... I understand your point. For DPDK Vhost library, it will likely imply ABI breakage as it would require to asynchronous handling of Vhost-user requests. We would only be able to do it at next LTS release. Hao, as your driver is not available upstream it will be difficult to assist you more. But if you look at other DPDK vDPA driver like SFC for instance, the way they implemented host notification control should be safe against this kind of deadlocks. --- hw/virtio/vhost-user.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index f214df804b..02caa94b6c 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2371,20 +2371,14 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, struct vhost_iotlb_msg *imsg) { -int ret; VhostUserMsg msg = { .hdr.request = VHOST_USER_IOTLB_MSG, .hdr.size = sizeof(msg.payload.iotlb), -.hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK, +.hdr.flags = VHOST_USER_VERSION, .payload.iotlb = *imsg, }; -ret = vhost_user_write(dev, &msg, NULL, 0); -if (ret < 0) { -return ret; -} - -return process_message_reply(dev, &msg); +return vhost_user_write(dev, &msg, NULL, 0); } -- 2.27.0
Re: [PATCH] vhost-user: fix the issue of vhost deadlock in nested virtualization
On 2/13/24 11:05, Michael S. Tsirkin wrote: On Fri, Jan 26, 2024 at 06:07:37PM +0800, Hao Chen wrote: I run "dpdk-vdpa" and "qemur-L2" in "qemu-L1". In a nested virtualization environment, "qemu-L2" vhost-user socket sends a "VHOST_USER_IOTLB_MSG" message to "dpdk-vdpa" and blocks waiting for "dpdk-vdpa" to process the message. If "dpdk-vdpa" doesn't complete the processing of the "VHOST_USER_IOTLB_MSG" message and sends a message that needs to be replied in another thread, such as "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG", "dpdk-vdpa" will also block and wait for "qemu-L2" to process this message. However, "qemu-L2" vhost-user's socket is blocking while waiting for a reply from "dpdk-vdpa" after processing the message "VHOSTr_USER_IOTLB_MSG", and "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG" will not be processed. In this case, both "dpdk-vdpa" and "qemu-L2" are blocked on the vhost read, resulting in a deadlock. You can modify "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG" or "VHOST_USER_IOTLB_MSG" to "no need reply" to fix this issue. There are too many messages in dpdk that are similar to "VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG", and I would prefer the latter. Fixes: 24e34754eb78 ("vhost-user: factor out msg head and payload") Signed-off-by: Hao Chen I would be very worried that IOTLB becomes stale and guest memory is corrupted if we just proceed without waiting. Maxime what do you think? How would you address the issue? I agree with you, this is not possible. For example, in case of IOTLB invalidate, the frontend relies on the backend reply to ensure it is no more accessing the memory before proceeding. The reply-ack for VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG request is less important, if it fails the host notifications won't work but would not risk corruption. Maybe on Qemu side we could fail init if processing the request fails, as I think that if negotiated, we can expect it to succeed. What do you think about this proposal? Regards, Maxime --- hw/virtio/vhost-user.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index f214df804b..02caa94b6c 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2371,20 +2371,14 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, struct vhost_iotlb_msg *imsg) { -int ret; VhostUserMsg msg = { .hdr.request = VHOST_USER_IOTLB_MSG, .hdr.size = sizeof(msg.payload.iotlb), -.hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK, +.hdr.flags = VHOST_USER_VERSION, .payload.iotlb = *imsg, }; -ret = vhost_user_write(dev, &msg, NULL, 0); -if (ret < 0) { -return ret; -} - -return process_message_reply(dev, &msg); +return vhost_user_write(dev, &msg, NULL, 0); } -- 2.27.0
Re: [PATCH v2] vhost-user: send SET_STATUS 0 after GET_VRING_BASE
On 5/2/23 01:04, Stefan Hajnoczi wrote: Setting the VIRTIO Device Status Field to 0 resets the device. The device's state is lost, including the vring configuration. vhost-user.c currently sends SET_STATUS 0 before GET_VRING_BASE. This risks confusion about the lifetime of the vhost-user state (e.g. vring last_avail_idx) across VIRTIO device reset. Eugenio Pérez adjusted the order for vhost-vdpa.c in commit c3716f260bff ("vdpa: move vhost reset after get vring base") and in that commit description suggested doing the same for vhost-user in the future. Go ahead and adjust vhost-user.c now. I ran various online code searches to identify vhost-user backends implementing SET_STATUS. It seems only DPDK implements SET_STATUS and Yajun Wu has confirmed that it is safe to make this change. Fixes: commit 923b8921d210763359e96246a58658ac0db6c645 ("vhost-user: Support vhost_dev_start") Cc: Michael S. Tsirkin Cc: Cindy Lu Cc: Yajun Wu Signed-off-by: Stefan Hajnoczi --- v2: - Added VHOST_USER_PROTOCOL_F_STATUS check [Yajun Wu] - Added "Fixes:" tag [Michael] --- hw/virtio/vhost-user.c | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index e5285df4ba..40974afd06 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2677,7 +2677,20 @@ static int vhost_user_dev_start(struct vhost_dev *dev, bool started) VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); } else { -return vhost_user_set_status(dev, 0); +return 0; +} +} + +static void vhost_user_reset_status(struct vhost_dev *dev) +{ +/* Set device status only for last queue pair */ +if (dev->vq_index + dev->nvqs != dev->vq_index_end) { +return; +} + +if (virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_STATUS)) { +vhost_user_set_status(dev, 0); } } @@ -2716,4 +2729,5 @@ const VhostOps user_ops = { .vhost_get_inflight_fd = vhost_user_get_inflight_fd, .vhost_set_inflight_fd = vhost_user_set_inflight_fd, .vhost_dev_start = vhost_user_dev_start, +.vhost_reset_status = vhost_user_reset_status, }; I confirm it won't introduce regression on DPDK side: Reviewed-by: Maxime Coquelin Thanks, Maxime
Re: [PATCH 1/4] vhost: Re-enable vrings after setting features
On 4/12/23 22:51, Stefan Hajnoczi wrote: On Tue, Apr 11, 2023 at 05:05:12PM +0200, Hanna Czenczek wrote: If the back-end supports the VHOST_USER_F_PROTOCOL_FEATURES feature, setting the vhost features will set this feature, too. Doing so disables all vrings, which may not be intended. For example, enabling or disabling logging during migration requires setting those features (to set or unset VHOST_F_LOG_ALL), which will automatically disable all vrings. In either case, the VM is running (disabling logging is done after a failed or cancelled migration, and only once the VM is running again, see comment in memory_global_dirty_log_stop()), so the vrings should really be enabled. As a result, the back-end seems to hang. To fix this, we must remember whether the vrings are supposed to be enabled, and, if so, re-enable them after a SET_FEATURES call that set VHOST_USER_F_PROTOCOL_FEATURES. It seems less than ideal that there is a short period in which the VM is running but the vrings will be stopped (between SET_FEATURES and SET_VRING_ENABLE). To fix this, we would need to change the protocol, e.g. by introducing a new flag or vhost-user protocol feature to disable disabling vrings whenever VHOST_USER_F_PROTOCOL_FEATURES is set, or add new functions for setting/clearing singular feature bits (so that F_LOG_ALL can be set/cleared without touching F_PROTOCOL_FEATURES). Even with such a potential addition to the protocol, we still need this fix here, because we cannot expect that back-ends will implement this addition. Signed-off-by: Hanna Czenczek --- include/hw/virtio/vhost.h | 10 ++ hw/virtio/vhost.c | 13 + 2 files changed, 23 insertions(+) diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index a52f273347..2fe02ed5d4 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -90,6 +90,16 @@ struct vhost_dev { int vq_index_end; /* if non-zero, minimum required value for max_queues */ int num_queues; + +/* + * Whether the virtqueues are supposed to be enabled (via + * SET_VRING_ENABLE). Setting the features (e.g. for + * enabling/disabling logging) will disable all virtqueues if + * VHOST_USER_F_PROTOCOL_FEATURES is set, so then we need to + * re-enable them if this field is set. + */ +bool enable_vqs; + /** * vhost feature handling requires matching the feature set * offered by a backend which may be a subset of the total diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index a266396576..cbff589efa 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -50,6 +50,8 @@ static unsigned int used_memslots; static QLIST_HEAD(, vhost_dev) vhost_devices = QLIST_HEAD_INITIALIZER(vhost_devices); +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable); + bool vhost_has_free_slot(void) { unsigned int slots_limit = ~0U; @@ -899,6 +901,15 @@ static int vhost_dev_set_features(struct vhost_dev *dev, } } +if (dev->enable_vqs) { +/* + * Setting VHOST_USER_F_PROTOCOL_FEATURES would have disabled all + * virtqueues, even if that was not intended; re-enable them if + * necessary. + */ +vhost_dev_set_vring_enable(dev, true); +} + out: return r; } @@ -1896,6 +1907,8 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size, static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable) { +hdev->enable_vqs = enable; + if (!hdev->vhost_ops->vhost_set_vring_enable) { return 0; } The vhost-user spec doesn't say that VHOST_F_LOG_ALL needs to be toggled at runtime and I don't think VHOST_USER_SET_PROTOCOL_FEATURES is intended to be used like that. This issue shows why doing so is a bad idea. VHOST_F_LOG_ALL does not need to be toggled to control logging. Logging is controlled at runtime by the presence of the dirty log (VHOST_USER_SET_LOG_BASE) and the per-vring logging flag (VHOST_VRING_F_LOG). I suggest permanently enabling VHOST_F_LOG_ALL upon connection when the the backend supports it. No spec changes are required. libvhost-user looks like it will work. I didn't look at DPDK/SPDK, but checking that it works there is important too. I have CCed people who may be interested in this issue. This is the first time I've looked at vhost-user logging, so this idea may not work. In the case of DPDK, we rely on VHOST_F_LOG_ALL to be set to know whether we should do dirty pages logging or not [0], so setting this feature at init time will cause performance degradation. The check on whether the log base address has been set is done afterwards. Regards, Maxime Stefan [0]: https://git.dpdk.org/dpdk/tree/lib/vhost/vhost.h#n594
Re: Emulating device configuration / max_virtqueue_pairs in vhost-vdpa and vhost-user
On 3/8/23 13:15, Michael S. Tsirkin wrote: On Wed, Mar 08, 2023 at 11:33:45AM +0100, Maxime Coquelin wrote: Hello Michael, On 2/1/23 12:20, Michael S. Tsirkin wrote: On Wed, Feb 01, 2023 at 12:14:18PM +0100, Maxime Coquelin wrote: Thanks Eugenio for working on this. On 1/31/23 20:10, Eugenio Perez Martin wrote: Hi, The current approach of offering an emulated CVQ to the guest and map the commands to vhost-user is not scaling well: * Some devices already offer it, so the transformation is redundant. * There is no support for commands with variable length (RSS?) We can solve both of them by offering it through vhost-user the same way as vhost-vdpa do. With this approach qemu needs to track the commands, for similar reasons as vhost-vdpa: qemu needs to track the device status for live migration. vhost-user should use the same SVQ code for this, so we avoid duplications. One of the challenges here is to know what virtqueue to shadow / isolate. The vhost-user device may not have the same queues as the device frontend: * The first depends on the actual vhost-user device, and qemu fetches it with VHOST_USER_GET_QUEUE_NUM at the moment. * The qemu device frontend's is set by netdev queues= cmdline parameter in qemu For the device, the CVQ is the last one it offers, but for the guest it is the last one offered in config space. To create a new vhost-user command to decrease that maximum number of queues may be an option. But we can do it without adding more commands, remapping the CVQ index at virtqueue setup. I think it should be doable using (struct vhost_dev).vq_index and maybe a few adjustments here and there. Thoughts? I am fine with both proposals. I think index remapping will require a bit more rework in the DPDK Vhost-user library, but nothing insurmountable. I am currently working on a PoC adding support for VDUSE in the DPDK Vhost library, and recently added control queue support. We can reuse it if we want to prototype your proposal. Maxime Thanks! technically backend knows how many vqs are there, last one is cvq... not sure we need full blown remapping ... Before VHOST_USER_PROTOCOL_F_STATUS was supported by qemu (very recently, v7.2.0), we had no way for the backend to be sure the frontend won't configure new queue pairs, this not not defined in the spec AFAICT [0]. In DPDK Vhost library, we notify the application it can start to use the device once the first queue pair is setup and enabled, then we notify the application when new queues are ready to be processed. In this case, I think we cannot deduce whether the queue is a data or a control queue when it is setup. When VHOST_USER_PROTOCOL_F_STATUS is supported, we know no more queues will be configured once the DRIVER_OK status is set. In this case, we can deduce the last queue setup will be the control queue at DRIVER_OK time if the number of queues is odd. Using index remapping, we would know directly at queue setup time whether this is a data or control queue based on its index value, i.e. if the index equals to max queue index supported by the backend. But thinking at it again, we may at least back this with a protocol feature to avoid issues with legacy backends. I hope it clarifies, let me know if anything unclear. Thanks, Maxime [0]: https://elixir.bootlin.com/qemu/latest/source/docs/interop/vhost-user.rst OK maybe document this. Sure, working on it... But I just found a discrepancy related to VHOST_USER_GET_QUEUE_NUM between the spec and the frontend/backend implementations. In the spec [0], VHOST_USER_GET_QUEUE_NUM reply is the number of queues. In Qemu Vhost-user Net frontend [1], VHOST_USER_GET_QUEUE_NUM is handled as the number of queue *pairs*, and so does the DPDK Vhost library. Vhost-user-bridge Qemu test application handles it as the number of queues. Other device types seem to handle it as the number of queues, which makes sense since they don't have the notion of queue pair. Fixing the QEMU and DPDK implementations would require a new protocol feature bit not to break compatibility with older versions. So maybe we should add in the spec that for network devices, VHOST_USER_GET_QUEUE_NUM reply represents the number of queue pairs, and also fix vhost-user-bridge to reply with the number of queue pairs? Maxime [0]: https://elixir.bootlin.com/qemu/latest/source/docs/interop/vhost-user.rst#L1091 [1]: https://elixir.bootlin.com/qemu/latest/source/net/vhost-user.c#L69
Re: Emulating device configuration / max_virtqueue_pairs in vhost-vdpa and vhost-user
Hello Michael, On 2/1/23 12:20, Michael S. Tsirkin wrote: On Wed, Feb 01, 2023 at 12:14:18PM +0100, Maxime Coquelin wrote: Thanks Eugenio for working on this. On 1/31/23 20:10, Eugenio Perez Martin wrote: Hi, The current approach of offering an emulated CVQ to the guest and map the commands to vhost-user is not scaling well: * Some devices already offer it, so the transformation is redundant. * There is no support for commands with variable length (RSS?) We can solve both of them by offering it through vhost-user the same way as vhost-vdpa do. With this approach qemu needs to track the commands, for similar reasons as vhost-vdpa: qemu needs to track the device status for live migration. vhost-user should use the same SVQ code for this, so we avoid duplications. One of the challenges here is to know what virtqueue to shadow / isolate. The vhost-user device may not have the same queues as the device frontend: * The first depends on the actual vhost-user device, and qemu fetches it with VHOST_USER_GET_QUEUE_NUM at the moment. * The qemu device frontend's is set by netdev queues= cmdline parameter in qemu For the device, the CVQ is the last one it offers, but for the guest it is the last one offered in config space. To create a new vhost-user command to decrease that maximum number of queues may be an option. But we can do it without adding more commands, remapping the CVQ index at virtqueue setup. I think it should be doable using (struct vhost_dev).vq_index and maybe a few adjustments here and there. Thoughts? I am fine with both proposals. I think index remapping will require a bit more rework in the DPDK Vhost-user library, but nothing insurmountable. I am currently working on a PoC adding support for VDUSE in the DPDK Vhost library, and recently added control queue support. We can reuse it if we want to prototype your proposal. Maxime Thanks! technically backend knows how many vqs are there, last one is cvq... not sure we need full blown remapping ... Before VHOST_USER_PROTOCOL_F_STATUS was supported by qemu (very recently, v7.2.0), we had no way for the backend to be sure the frontend won't configure new queue pairs, this not not defined in the spec AFAICT [0]. In DPDK Vhost library, we notify the application it can start to use the device once the first queue pair is setup and enabled, then we notify the application when new queues are ready to be processed. In this case, I think we cannot deduce whether the queue is a data or a control queue when it is setup. When VHOST_USER_PROTOCOL_F_STATUS is supported, we know no more queues will be configured once the DRIVER_OK status is set. In this case, we can deduce the last queue setup will be the control queue at DRIVER_OK time if the number of queues is odd. Using index remapping, we would know directly at queue setup time whether this is a data or control queue based on its index value, i.e. if the index equals to max queue index supported by the backend. But thinking at it again, we may at least back this with a protocol feature to avoid issues with legacy backends. I hope it clarifies, let me know if anything unclear. Thanks, Maxime [0]: https://elixir.bootlin.com/qemu/latest/source/docs/interop/vhost-user.rst
Re: Emulating device configuration / max_virtqueue_pairs in vhost-vdpa and vhost-user
On 2/1/23 12:19, Eugenio Perez Martin wrote: On Wed, Feb 1, 2023 at 12:14 PM Maxime Coquelin wrote: Thanks Eugenio for working on this. On 1/31/23 20:10, Eugenio Perez Martin wrote: Hi, The current approach of offering an emulated CVQ to the guest and map the commands to vhost-user is not scaling well: * Some devices already offer it, so the transformation is redundant. * There is no support for commands with variable length (RSS?) We can solve both of them by offering it through vhost-user the same way as vhost-vdpa do. With this approach qemu needs to track the commands, for similar reasons as vhost-vdpa: qemu needs to track the device status for live migration. vhost-user should use the same SVQ code for this, so we avoid duplications. One of the challenges here is to know what virtqueue to shadow / isolate. The vhost-user device may not have the same queues as the device frontend: * The first depends on the actual vhost-user device, and qemu fetches it with VHOST_USER_GET_QUEUE_NUM at the moment. * The qemu device frontend's is set by netdev queues= cmdline parameter in qemu For the device, the CVQ is the last one it offers, but for the guest it is the last one offered in config space. To create a new vhost-user command to decrease that maximum number of queues may be an option. But we can do it without adding more commands, remapping the CVQ index at virtqueue setup. I think it should be doable using (struct vhost_dev).vq_index and maybe a few adjustments here and there. Thoughts? I am fine with both proposals. I think index remapping will require a bit more rework in the DPDK Vhost-user library, but nothing insurmountable. Can you expand on this? I think my proposal does not require modifying anything in the device. Or you mean hw/virtio/vhost-user.c and similar? I had deeper look, and both proposals should not be very different in term of rework on DPDK side. Maxime Thanks! I am currently working on a PoC adding support for VDUSE in the DPDK Vhost library, and recently added control queue support. We can reuse it if we want to prototype your proposal. Sure, that would be great. Thanks!
[PATCH v2 3/3] vhost-user: Adopt new backend naming
The Vhost-user specification changed feature and request naming from _SLAVE_ to _BACKEND_. This patch adopts the new naming convention. Signed-off-by: Maxime Coquelin --- hw/virtio/vhost-user.c | 30 +++--- hw/virtio/virtio-qmp.c | 12 ++-- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index e68daa35d4..8968541514 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -40,7 +40,7 @@ #define VHOST_MEMORY_BASELINE_NREGIONS8 #define VHOST_USER_F_PROTOCOL_FEATURES 30 -#define VHOST_USER_SLAVE_MAX_FDS 8 +#define VHOST_USER_BACKEND_MAX_FDS 8 /* * Set maximum number of RAM slots supported to @@ -71,12 +71,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5, +VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5, VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, VHOST_USER_PROTOCOL_F_CONFIG = 9, -VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD = 10, +VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10, VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, @@ -110,7 +110,7 @@ typedef enum VhostUserRequest { VHOST_USER_SET_VRING_ENABLE = 18, VHOST_USER_SEND_RARP = 19, VHOST_USER_NET_SET_MTU = 20, -VHOST_USER_SET_SLAVE_REQ_FD = 21, +VHOST_USER_SET_BACKEND_REQ_FD = 21, VHOST_USER_IOTLB_MSG = 22, VHOST_USER_SET_VRING_ENDIAN = 23, VHOST_USER_GET_CONFIG = 24, @@ -134,11 +134,11 @@ typedef enum VhostUserRequest { } VhostUserRequest; typedef enum VhostUserSlaveRequest { -VHOST_USER_SLAVE_NONE = 0, -VHOST_USER_SLAVE_IOTLB_MSG = 1, -VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2, -VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3, -VHOST_USER_SLAVE_MAX +VHOST_USER_BACKEND_NONE = 0, +VHOST_USER_BACKEND_IOTLB_MSG = 1, +VHOST_USER_BACKEND_CONFIG_CHANGE_MSG = 2, +VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3, +VHOST_USER_BACKEND_MAX } VhostUserSlaveRequest; typedef struct VhostUserMemoryRegion { @@ -1638,13 +1638,13 @@ static gboolean slave_read(QIOChannel *ioc, GIOCondition condition, } switch (hdr.request) { -case VHOST_USER_SLAVE_IOTLB_MSG: +case VHOST_USER_BACKEND_IOTLB_MSG: ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); break; -case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : +case VHOST_USER_BACKEND_CONFIG_CHANGE_MSG: ret = vhost_user_slave_handle_config_change(dev); break; -case VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG: +case VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG: ret = vhost_user_slave_handle_vring_host_notifier(dev, &payload.area, fd ? fd[0] : -1); break; @@ -1696,7 +1696,7 @@ fdcleanup: static int vhost_setup_slave_channel(struct vhost_dev *dev) { VhostUserMsg msg = { -.hdr.request = VHOST_USER_SET_SLAVE_REQ_FD, +.hdr.request = VHOST_USER_SET_BACKEND_REQ_FD, .hdr.flags = VHOST_USER_VERSION, }; struct vhost_user *u = dev->opaque; @@ -1707,7 +1707,7 @@ static int vhost_setup_slave_channel(struct vhost_dev *dev) QIOChannel *ioc; if (!virtio_has_feature(dev->protocol_features, -VHOST_USER_PROTOCOL_F_SLAVE_REQ)) { +VHOST_USER_PROTOCOL_F_BACKEND_REQ)) { return 0; } @@ -2065,7 +2065,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) && !(virtio_has_feature(dev->protocol_features, -VHOST_USER_PROTOCOL_F_SLAVE_REQ) && +VHOST_USER_PROTOCOL_F_BACKEND_REQ) && virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { error_setg(errp, "IOMMU support requires reply-ack and " diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c index e4d4bece2d..b70148aba9 100644 --- a/hw/virtio/virtio-qmp.c +++ b/hw/virtio/virtio-qmp.c @@ -42,12 +42,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5, +VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5, VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, VHOST_USER_PROTOCOL_F_CONFIG = 9, -VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD = 10, +VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10, VHOST_USER_PROTOCOL_F_HOST_NO
[PATCH v2 2/3] libvhost-user: Adopt new backend naming
The Vhost-user specification changed feature and request naming from _SLAVE_ to _BACKEND_. This patch adopts the new naming convention. Signed-off-by: Maxime Coquelin --- subprojects/libvhost-user/libvhost-user.c | 20 ++-- subprojects/libvhost-user/libvhost-user.h | 20 ++-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index fc69783d2b..f661af7c85 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -140,7 +140,7 @@ vu_request_to_string(unsigned int req) REQ(VHOST_USER_SET_VRING_ENABLE), REQ(VHOST_USER_SEND_RARP), REQ(VHOST_USER_NET_SET_MTU), -REQ(VHOST_USER_SET_SLAVE_REQ_FD), +REQ(VHOST_USER_SET_BACKEND_REQ_FD), REQ(VHOST_USER_IOTLB_MSG), REQ(VHOST_USER_SET_VRING_ENDIAN), REQ(VHOST_USER_GET_CONFIG), @@ -1365,7 +1365,7 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd, int qidx = vq - dev->vq; int fd_num = 0; VhostUserMsg vmsg = { -.request = VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG, +.request = VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG, .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK, .size = sizeof(vmsg.payload.area), .payload.area = { @@ -1383,7 +1383,7 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd, vmsg.fd_num = fd_num; -if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) { +if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD)) { return false; } @@ -1461,9 +1461,9 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) */ uint64_t features = 1ULL << VHOST_USER_PROTOCOL_F_MQ | 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | -1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | +1ULL << VHOST_USER_PROTOCOL_F_BACKEND_REQ | 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | -1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD | +1ULL << VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD | 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | 1ULL << VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS; @@ -1494,7 +1494,7 @@ vu_set_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS) && -(!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ) || +(!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ) || !vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { /* * The use case for using messages for kick/call is simulation, to make @@ -1507,7 +1507,7 @@ vu_set_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) * that actually enables the simulation case. */ vu_panic(dev, - "F_IN_BAND_NOTIFICATIONS requires F_SLAVE_REQ && F_REPLY_ACK"); + "F_IN_BAND_NOTIFICATIONS requires F_BACKEND_REQ && F_REPLY_ACK"); return false; } @@ -1910,7 +1910,7 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg) return vu_get_queue_num_exec(dev, vmsg); case VHOST_USER_SET_VRING_ENABLE: return vu_set_vring_enable_exec(dev, vmsg); -case VHOST_USER_SET_SLAVE_REQ_FD: +case VHOST_USER_SET_BACKEND_REQ_FD: return vu_set_slave_req_fd(dev, vmsg); case VHOST_USER_GET_CONFIG: return vu_get_config(dev, vmsg); @@ -2416,9 +2416,9 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync) if (vq->call_fd < 0 && vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS) && -vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ)) { +vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ)) { VhostUserMsg vmsg = { -.request = VHOST_USER_SLAVE_VRING_CALL, +.request = VHOST_USER_BACKEND_VRING_CALL, .flags = VHOST_USER_VERSION, .size = sizeof(vmsg.payload.state), .payload.state = { diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h index 8cda9b8f57..8c5a2719e3 100644 --- a/subprojects/libvhost-user/libvhost-user.h +++ b/subprojects/libvhost-user/libvhost-user.h @@ -54,12 +54,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5, +VHOST_
[PATCH v2 0/3] Vhost-user: replace _SLAVE_ with _BACKEND_
This series continues the work done to switch to a more inclusive terminology in the Vhost-user specification. While the spec texts were changed to replace slave with backend, the protocol features and messages names hadn't been changed. This series renames remaining occurences in the spec and make use of the new names in both libvhost-user and the Vhost-user frontend code. Changes in v2: == - Use positive wording (Michael) - Fix grammar mistakes (Michael) Maxime Coquelin (3): docs: vhost-user: replace _SLAVE_ with _BACKEND_ libvhost-user: Adopt new backend naming vhost-user: Adopt new backend naming docs/interop/vhost-user.rst | 40 +++ hw/virtio/vhost-user.c| 30 - hw/virtio/virtio-qmp.c| 12 +++ subprojects/libvhost-user/libvhost-user.c | 20 ++-- subprojects/libvhost-user/libvhost-user.h | 20 ++-- 5 files changed, 61 insertions(+), 61 deletions(-) -- 2.39.1
[PATCH v2 1/3] docs: vhost-user: replace _SLAVE_ with _BACKEND_
Backend's message and protocol features names were still using "_SLAVE_" naming. For consistency with the new naming convention, replace it with _BACKEND_. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.rst | 40 ++--- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3f18ab424e..8a5924ea75 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -315,7 +315,7 @@ in the ancillary data: * ``VHOST_USER_SET_VRING_KICK`` * ``VHOST_USER_SET_VRING_CALL`` * ``VHOST_USER_SET_VRING_ERR`` -* ``VHOST_USER_SET_SLAVE_REQ_FD`` +* ``VHOST_USER_SET_BACKEND_REQ_FD`` (previous name ``VHOST_USER_SET_SLAVE_REQ_FD``) * ``VHOST_USER_SET_INFLIGHT_FD`` (if ``VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD``) If *front-end* is unable to send the full message or receives a wrong @@ -516,7 +516,7 @@ expected to reply with a zero payload, non-zero otherwise. The back-end relies on the back-end communication channel (see :ref:`Back-end communication ` section below) to send IOTLB miss -and access failure events, by sending ``VHOST_USER_SLAVE_IOTLB_MSG`` +and access failure events, by sending ``VHOST_USER_BACKEND_IOTLB_MSG`` requests to the front-end with a ``struct vhost_iotlb_msg`` as payload. For miss events, the iotlb payload has to be filled with the miss message type (1), the I/O virtual address and the permissions @@ -540,15 +540,15 @@ Back-end communication -- An optional communication channel is provided if the back-end declares -``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` protocol feature, to allow the +``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` protocol feature, to allow the back-end to make requests to the front-end. -The fd is provided via ``VHOST_USER_SET_SLAVE_REQ_FD`` ancillary data. +The fd is provided via ``VHOST_USER_SET_BACKEND_REQ_FD`` ancillary data. -A back-end may then send ``VHOST_USER_SLAVE_*`` messages to the front-end +A back-end may then send ``VHOST_USER_BACKEND_*`` messages to the front-end using this fd communication channel. -If ``VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD`` protocol feature is +If ``VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD`` protocol feature is negotiated, back-end can send file descriptors (at most 8 descriptors in each message) to front-end via ancillary data using this fd communication channel. @@ -835,7 +835,7 @@ Note that due to the fact that too many messages on the sockets can cause the sending application(s) to block, it is not advised to use this feature unless absolutely necessary. It is also considered an error to negotiate this feature without also negotiating -``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, +``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, the former is necessary for getting a message channel from the back-end to the front-end, while the latter needs to be used with the in-band notification messages to block until they are processed, both to avoid @@ -855,12 +855,12 @@ Protocol features #define VHOST_USER_PROTOCOL_F_RARP 2 #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3 #define VHOST_USER_PROTOCOL_F_MTU 4 - #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 + #define VHOST_USER_PROTOCOL_F_BACKEND_REQ 5 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 #define VHOST_USER_PROTOCOL_F_CONFIG9 - #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD10 + #define VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD 10 #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER11 #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 @@ -1059,8 +1059,8 @@ Front-end message types in the ancillary data. This signals that polling will be used instead of waiting for the call. Note that if the protocol features ``VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS`` and - ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated this message - isn't necessary as the ``VHOST_USER_SLAVE_VRING_CALL`` message can be + ``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` have been negotiated this message + isn't necessary as the ``VHOST_USER_BACKEND_VRING_CALL`` message can be used, it may however still be used to set an event file descriptor or to enable polling. @@ -1077,8 +1077,8 @@ Front-end message types invalid FD flag. This flag is set when there is no file descriptor in the ancillary data. Note that if the protocol features ``VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS`` and - ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated this message - isn't necessary as the ``VHOST_USER_SLAVE_VRING_ERR`` message can be + ``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` hav
Re: Emulating device configuration / max_virtqueue_pairs in vhost-vdpa and vhost-user
Thanks Eugenio for working on this. On 1/31/23 20:10, Eugenio Perez Martin wrote: Hi, The current approach of offering an emulated CVQ to the guest and map the commands to vhost-user is not scaling well: * Some devices already offer it, so the transformation is redundant. * There is no support for commands with variable length (RSS?) We can solve both of them by offering it through vhost-user the same way as vhost-vdpa do. With this approach qemu needs to track the commands, for similar reasons as vhost-vdpa: qemu needs to track the device status for live migration. vhost-user should use the same SVQ code for this, so we avoid duplications. One of the challenges here is to know what virtqueue to shadow / isolate. The vhost-user device may not have the same queues as the device frontend: * The first depends on the actual vhost-user device, and qemu fetches it with VHOST_USER_GET_QUEUE_NUM at the moment. * The qemu device frontend's is set by netdev queues= cmdline parameter in qemu For the device, the CVQ is the last one it offers, but for the guest it is the last one offered in config space. To create a new vhost-user command to decrease that maximum number of queues may be an option. But we can do it without adding more commands, remapping the CVQ index at virtqueue setup. I think it should be doable using (struct vhost_dev).vq_index and maybe a few adjustments here and there. Thoughts? I am fine with both proposals. I think index remapping will require a bit more rework in the DPDK Vhost-user library, but nothing insurmountable. I am currently working on a PoC adding support for VDUSE in the DPDK Vhost library, and recently added control queue support. We can reuse it if we want to prototype your proposal. Maxime Thanks!
Re: [PATCH 0/3] Vhost-user: replace _SLAVE_ with _BACKEND_
On 1/30/23 12:08, Michael S. Tsirkin wrote: On Mon, Jan 30, 2023 at 11:45:45AM +0100, Maxime Coquelin wrote: This series continues the work done to get rid of harmful language in the Vhost-user specification. I prefer a positive "switch to a more inclusive terminology". To consider if you keep doing this work. Right, it is indeed better. I will post a new revision using positive wording. Thanks for the review, Maxime While the spec texts were changed to replace slave with backend, the protocol features and messages names hadn't been changed. This series renames remaining occurences in the spec and make use of the new names in both libvhost-user and the Vhost-user frontend code. Maxime Coquelin (3): docs: vhost-user: replace _SLAVE_ with _BACKEND_ libvhost-user: Adopt new backend naming vhost-user: Adopt new backend naming docs/interop/vhost-user.rst | 40 +++ hw/virtio/vhost-user.c| 30 - hw/virtio/virtio-qmp.c| 12 +++ subprojects/libvhost-user/libvhost-user.c | 20 ++-- subprojects/libvhost-user/libvhost-user.h | 20 ++-- 5 files changed, 61 insertions(+), 61 deletions(-) -- 2.39.1
[PATCH 3/3] vhost-user: Adopt new backend naming
In order to get rid of harmful language, the Vhost-user specification changed features and requests naming from _SLAVE_ to _BACKEND_. This patch adopts the new naming convention. Signed-off-by: Maxime Coquelin --- hw/virtio/vhost-user.c | 30 +++--- hw/virtio/virtio-qmp.c | 12 ++-- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index d9ce0501b2..9b623ecf4a 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -40,7 +40,7 @@ #define VHOST_MEMORY_BASELINE_NREGIONS8 #define VHOST_USER_F_PROTOCOL_FEATURES 30 -#define VHOST_USER_SLAVE_MAX_FDS 8 +#define VHOST_USER_BACKEND_MAX_FDS 8 /* * Set maximum number of RAM slots supported to @@ -71,12 +71,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5, +VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5, VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, VHOST_USER_PROTOCOL_F_CONFIG = 9, -VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD = 10, +VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10, VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, @@ -110,7 +110,7 @@ typedef enum VhostUserRequest { VHOST_USER_SET_VRING_ENABLE = 18, VHOST_USER_SEND_RARP = 19, VHOST_USER_NET_SET_MTU = 20, -VHOST_USER_SET_SLAVE_REQ_FD = 21, +VHOST_USER_SET_BACKEND_REQ_FD = 21, VHOST_USER_IOTLB_MSG = 22, VHOST_USER_SET_VRING_ENDIAN = 23, VHOST_USER_GET_CONFIG = 24, @@ -134,11 +134,11 @@ typedef enum VhostUserRequest { } VhostUserRequest; typedef enum VhostUserSlaveRequest { -VHOST_USER_SLAVE_NONE = 0, -VHOST_USER_SLAVE_IOTLB_MSG = 1, -VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2, -VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3, -VHOST_USER_SLAVE_MAX +VHOST_USER_BACKEND_NONE = 0, +VHOST_USER_BACKEND_IOTLB_MSG = 1, +VHOST_USER_BACKEND_CONFIG_CHANGE_MSG = 2, +VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3, +VHOST_USER_BACKEND_MAX } VhostUserSlaveRequest; typedef struct VhostUserMemoryRegion { @@ -1722,13 +1722,13 @@ static gboolean slave_read(QIOChannel *ioc, GIOCondition condition, } switch (hdr.request) { -case VHOST_USER_SLAVE_IOTLB_MSG: +case VHOST_USER_BACKEND_IOTLB_MSG: ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb); break; -case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG : +case VHOST_USER_BACKEND_CONFIG_CHANGE_MSG: ret = vhost_user_slave_handle_config_change(dev); break; -case VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG: +case VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG: ret = vhost_user_slave_handle_vring_host_notifier(dev, &payload.area, fd ? fd[0] : -1); break; @@ -1780,7 +1780,7 @@ fdcleanup: static int vhost_setup_slave_channel(struct vhost_dev *dev) { VhostUserMsg msg = { -.hdr.request = VHOST_USER_SET_SLAVE_REQ_FD, +.hdr.request = VHOST_USER_SET_BACKEND_REQ_FD, .hdr.flags = VHOST_USER_VERSION, }; struct vhost_user *u = dev->opaque; @@ -1791,7 +1791,7 @@ static int vhost_setup_slave_channel(struct vhost_dev *dev) QIOChannel *ioc; if (!virtio_has_feature(dev->protocol_features, -VHOST_USER_PROTOCOL_F_SLAVE_REQ)) { +VHOST_USER_PROTOCOL_F_BACKEND_REQ)) { return 0; } @@ -2147,7 +2147,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) && !(virtio_has_feature(dev->protocol_features, -VHOST_USER_PROTOCOL_F_SLAVE_REQ) && +VHOST_USER_PROTOCOL_F_BACKEND_REQ) && virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { error_setg(errp, "IOMMU support requires reply-ack and " diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c index e4d4bece2d..b70148aba9 100644 --- a/hw/virtio/virtio-qmp.c +++ b/hw/virtio/virtio-qmp.c @@ -42,12 +42,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5, +VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5, VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, VHOST_USER_PROTOCOL_F_CONFIG = 9, -VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD = 10, +VHOST_USER_PROTOCOL_F_BACKEND_SEND_
[PATCH 1/3] docs: vhost-user: replace _SLAVE_ with _BACKEND_
Backend's message and protocol features names were still using "_SLAVE_" naming. For consistency with the new naming convention and to get rid of the remaining harmful language, replace it with _BACKEND_. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.rst | 40 ++--- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3f18ab424e..8a5924ea75 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -315,7 +315,7 @@ in the ancillary data: * ``VHOST_USER_SET_VRING_KICK`` * ``VHOST_USER_SET_VRING_CALL`` * ``VHOST_USER_SET_VRING_ERR`` -* ``VHOST_USER_SET_SLAVE_REQ_FD`` +* ``VHOST_USER_SET_BACKEND_REQ_FD`` (previous name ``VHOST_USER_SET_SLAVE_REQ_FD``) * ``VHOST_USER_SET_INFLIGHT_FD`` (if ``VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD``) If *front-end* is unable to send the full message or receives a wrong @@ -516,7 +516,7 @@ expected to reply with a zero payload, non-zero otherwise. The back-end relies on the back-end communication channel (see :ref:`Back-end communication ` section below) to send IOTLB miss -and access failure events, by sending ``VHOST_USER_SLAVE_IOTLB_MSG`` +and access failure events, by sending ``VHOST_USER_BACKEND_IOTLB_MSG`` requests to the front-end with a ``struct vhost_iotlb_msg`` as payload. For miss events, the iotlb payload has to be filled with the miss message type (1), the I/O virtual address and the permissions @@ -540,15 +540,15 @@ Back-end communication -- An optional communication channel is provided if the back-end declares -``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` protocol feature, to allow the +``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` protocol feature, to allow the back-end to make requests to the front-end. -The fd is provided via ``VHOST_USER_SET_SLAVE_REQ_FD`` ancillary data. +The fd is provided via ``VHOST_USER_SET_BACKEND_REQ_FD`` ancillary data. -A back-end may then send ``VHOST_USER_SLAVE_*`` messages to the front-end +A back-end may then send ``VHOST_USER_BACKEND_*`` messages to the front-end using this fd communication channel. -If ``VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD`` protocol feature is +If ``VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD`` protocol feature is negotiated, back-end can send file descriptors (at most 8 descriptors in each message) to front-end via ancillary data using this fd communication channel. @@ -835,7 +835,7 @@ Note that due to the fact that too many messages on the sockets can cause the sending application(s) to block, it is not advised to use this feature unless absolutely necessary. It is also considered an error to negotiate this feature without also negotiating -``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, +``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` and ``VHOST_USER_PROTOCOL_F_REPLY_ACK``, the former is necessary for getting a message channel from the back-end to the front-end, while the latter needs to be used with the in-band notification messages to block until they are processed, both to avoid @@ -855,12 +855,12 @@ Protocol features #define VHOST_USER_PROTOCOL_F_RARP 2 #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3 #define VHOST_USER_PROTOCOL_F_MTU 4 - #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 + #define VHOST_USER_PROTOCOL_F_BACKEND_REQ 5 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 #define VHOST_USER_PROTOCOL_F_CONFIG9 - #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD10 + #define VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD 10 #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER11 #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 @@ -1059,8 +1059,8 @@ Front-end message types in the ancillary data. This signals that polling will be used instead of waiting for the call. Note that if the protocol features ``VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS`` and - ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated this message - isn't necessary as the ``VHOST_USER_SLAVE_VRING_CALL`` message can be + ``VHOST_USER_PROTOCOL_F_BACKEND_REQ`` have been negotiated this message + isn't necessary as the ``VHOST_USER_BACKEND_VRING_CALL`` message can be used, it may however still be used to set an event file descriptor or to enable polling. @@ -1077,8 +1077,8 @@ Front-end message types invalid FD flag. This flag is set when there is no file descriptor in the ancillary data. Note that if the protocol features ``VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS`` and - ``VHOST_USER_PROTOCOL_F_SLAVE_REQ`` have been negotiated this message - isn't necessary as the ``VHOST_USER_SL
[PATCH 2/3] libvhost-user: Adopt new backend naming
In order to get rid of harmful language, the Vhost-user specification changed features and requests naming from _SLAVE_ to _BACKEND_. This patch adopts the new naming convention. Signed-off-by: Maxime Coquelin --- subprojects/libvhost-user/libvhost-user.c | 20 ++-- subprojects/libvhost-user/libvhost-user.h | 20 ++-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index fc69783d2b..f661af7c85 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -140,7 +140,7 @@ vu_request_to_string(unsigned int req) REQ(VHOST_USER_SET_VRING_ENABLE), REQ(VHOST_USER_SEND_RARP), REQ(VHOST_USER_NET_SET_MTU), -REQ(VHOST_USER_SET_SLAVE_REQ_FD), +REQ(VHOST_USER_SET_BACKEND_REQ_FD), REQ(VHOST_USER_IOTLB_MSG), REQ(VHOST_USER_SET_VRING_ENDIAN), REQ(VHOST_USER_GET_CONFIG), @@ -1365,7 +1365,7 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd, int qidx = vq - dev->vq; int fd_num = 0; VhostUserMsg vmsg = { -.request = VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG, +.request = VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG, .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK, .size = sizeof(vmsg.payload.area), .payload.area = { @@ -1383,7 +1383,7 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd, vmsg.fd_num = fd_num; -if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) { +if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD)) { return false; } @@ -1461,9 +1461,9 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) */ uint64_t features = 1ULL << VHOST_USER_PROTOCOL_F_MQ | 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | -1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | +1ULL << VHOST_USER_PROTOCOL_F_BACKEND_REQ | 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | -1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD | +1ULL << VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD | 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | 1ULL << VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS; @@ -1494,7 +1494,7 @@ vu_set_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS) && -(!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ) || +(!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ) || !vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { /* * The use case for using messages for kick/call is simulation, to make @@ -1507,7 +1507,7 @@ vu_set_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) * that actually enables the simulation case. */ vu_panic(dev, - "F_IN_BAND_NOTIFICATIONS requires F_SLAVE_REQ && F_REPLY_ACK"); + "F_IN_BAND_NOTIFICATIONS requires F_BACKEND_REQ && F_REPLY_ACK"); return false; } @@ -1910,7 +1910,7 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg) return vu_get_queue_num_exec(dev, vmsg); case VHOST_USER_SET_VRING_ENABLE: return vu_set_vring_enable_exec(dev, vmsg); -case VHOST_USER_SET_SLAVE_REQ_FD: +case VHOST_USER_SET_BACKEND_REQ_FD: return vu_set_slave_req_fd(dev, vmsg); case VHOST_USER_GET_CONFIG: return vu_get_config(dev, vmsg); @@ -2416,9 +2416,9 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync) if (vq->call_fd < 0 && vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS) && -vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SLAVE_REQ)) { +vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ)) { VhostUserMsg vmsg = { -.request = VHOST_USER_SLAVE_VRING_CALL, +.request = VHOST_USER_BACKEND_VRING_CALL, .flags = VHOST_USER_VERSION, .size = sizeof(vmsg.payload.state), .payload.state = { diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h index 8cda9b8f57..8c5a2719e3 100644 --- a/subprojects/libvhost-user/libvhost-user.h +++ b/subprojects/libvhost-user/libvhost-user.h @@ -54,12 +54,12 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RARP = 2, VHOST_USER_PROTOCOL_F_REPLY_ACK = 3, VHOST_USER_PROTOCOL_F_NET_MTU = 4, -V
[PATCH 0/3] Vhost-user: replace _SLAVE_ with _BACKEND_
This series continues the work done to get rid of harmful language in the Vhost-user specification. While the spec texts were changed to replace slave with backend, the protocol features and messages names hadn't been changed. This series renames remaining occurences in the spec and make use of the new names in both libvhost-user and the Vhost-user frontend code. Maxime Coquelin (3): docs: vhost-user: replace _SLAVE_ with _BACKEND_ libvhost-user: Adopt new backend naming vhost-user: Adopt new backend naming docs/interop/vhost-user.rst | 40 +++ hw/virtio/vhost-user.c| 30 - hw/virtio/virtio-qmp.c| 12 +++ subprojects/libvhost-user/libvhost-user.c | 20 ++-- subprojects/libvhost-user/libvhost-user.h | 20 ++-- 5 files changed, 61 insertions(+), 61 deletions(-) -- 2.39.1
Re: [PATCH 2/2] Revert "vhost-user: Introduce nested event loop in vhost_user_read()"
On 1/19/23 18:24, Greg Kurz wrote: This reverts commit a7f523c7d114d445c5d83aecdba3efc038e5a692. The nested event loop is broken by design. It's only user was removed. Drop the code as well so that nobody ever tries to use it again. I had to fix a couple of trivial conflicts around return values because of 025faa872bcf ("vhost-user: stick to -errno error return convention"). Signed-off-by: Greg Kurz --- hw/virtio/vhost-user.c | 65 -- 1 file changed, 5 insertions(+), 60 deletions(-) Acked-by: Maxime Coquelin Thanks, Maxime
Re: [PATCH 1/2] Revert "vhost-user: Monitor slave channel in vhost_user_read()"
On 1/19/23 18:24, Greg Kurz wrote: This reverts commit db8a3772e300c1a656331a92da0785d81667dc81. Motivation : this is breaking vhost-user with DPDK as reported in [0]. Received unexpected msg type. Expected 22 received 40 Fail to update device iotlb Received unexpected msg type. Expected 40 received 22 Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 1 ring restore failed: -71: Protocol error (71) Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 0 ring restore failed: -71: Protocol error (71) unable to start vhost net: 71: falling back on userspace virtio The failing sequence that leads to the first error is : - QEMU sends a VHOST_USER_GET_STATUS (40) request to DPDK on the master socket - QEMU starts a nested event loop in order to wait for the VHOST_USER_GET_STATUS response and to be able to process messages from the slave channel - DPDK sends a couple of legitimate IOTLB miss messages on the slave channel - QEMU processes each IOTLB request and sends VHOST_USER_IOTLB_MSG (22) updates on the master socket - QEMU assumes to receive a response for the latest VHOST_USER_IOTLB_MSG but it gets the response for the VHOST_USER_GET_STATUS instead The subsequent errors have the same root cause : the nested event loop breaks the order by design. It lures QEMU to expect responses to the latest message sent on the master socket to arrive first. Since this was only needed for DAX enablement which is still not merged upstream, just drop the code for now. A working solution will have to be merged later on. Likely protect the master socket with a mutex and service the slave channel with a separate thread, as discussed with Maxime in the mail thread below. [0] https://lore.kernel.org/qemu-devel/43145ede-89dc-280e-b953-6a2b436de...@redhat.com/ Reported-by: Yanghang Liu Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2155173 Signed-off-by: Greg Kurz --- hw/virtio/vhost-user.c | 35 +++ 1 file changed, 3 insertions(+), 32 deletions(-) Acked-by: Maxime Coquelin Thanks, Maxime
Re: [RFC v2 06/13] vhost: delay set_vring_ready after DRIVER_OK
Hi Eugenio, On 1/13/23 11:03, Eugenio Perez Martin wrote: On Fri, Jan 13, 2023 at 10:51 AM Stefano Garzarella wrote: On Fri, Jan 13, 2023 at 09:19:00AM +0100, Eugenio Perez Martin wrote: On Fri, Jan 13, 2023 at 5:36 AM Jason Wang wrote: On Fri, Jan 13, 2023 at 1:25 AM Eugenio Pérez wrote: To restore the device at the destination of a live migration we send the commands through control virtqueue. For a device to read CVQ it must have received the DRIVER_OK status bit. This probably requires the support from the parent driver and requires some changes or fixes in the parent driver. Some drivers did: parent_set_status(): if (DRIVER_OK) if (queue_enable) write queue_enable to the device Examples are IFCVF or even vp_vdpa at least. MLX5 seems to be fine. I don't get your point here. No device should start reading CVQ (or any other VQ) without having received DRIVER_OK. Some parent drivers do not support sending the queue enable command after DRIVER_OK, usually because they clean part of the state like the set by set_vring_base. Even vdpa_net_sim needs fixes here. But my understanding is that it should be supported so I consider it a bug. Especially after queue_reset patches. Is that what you mean? However this opens a window where the device could start receiving packets in rx queue 0 before it receives the RSS configuration. To avoid that, we will not send vring_enable until all configuration is used by the device. As a first step, run vhost_set_vring_ready for all vhost_net backend after all of them are started (with DRIVER_OK). This code should not affect vdpa. Signed-off-by: Eugenio Pérez --- hw/net/vhost_net.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index c4eecc6f36..3900599465 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -399,6 +399,18 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, } else { peer = qemu_get_peer(ncs, n->max_queue_pairs); } +r = vhost_net_start_one(get_vhost_net(peer), dev); +if (r < 0) { +goto err_start; +} +} + +for (int j = 0; j < nvhosts; j++) { +if (j < data_queue_pairs) { +peer = qemu_get_peer(ncs, j); +} else { +peer = qemu_get_peer(ncs, n->max_queue_pairs); +} I fail to understand why we need to change the vhost_net layer? This is vhost-vDPA specific, so I wonder if we can limit the changes to e.g vhost_vdpa_dev_start()? The vhost-net layer explicitly calls vhost_set_vring_enable before vhost_dev_start, and this is exactly the behavior we want to avoid. Even if we make changes to vhost_dev, this change is still needed. I'm working on something similar since I'd like to re-work the following commit we merged just before 7.2 release: 4daa5054c5 vhost: enable vrings in vhost_dev_start() for vhost-user devices vhost-net wasn't the only one who enabled vrings independently, but it was easy enough for others devices to avoid it and enable them in vhost_dev_start(). Do you think can we avoid in some way this special behaviour of vhost-net and enable the vrings in vhost_dev_start? Actually looking forward to it :). If that gets merged before this series, I think we could drop this patch. If I'm not wrong the enable/disable dance is used just by vhost-user at the moment. Maxime, could you give us some hints about the tests to use to check that changes do not introduce regressions in vhost-user? You can use DPDK's testpmd [0] tool with Vhost PMD, e.g.: #single queue pair # dpdk-testpmd -l --no-pci --vdev=net_vhost0,iface=/tmp/vhost-user1 -- -i #multiqueue # dpdk-testpmd -l --no-pci --vdev=net_vhost0,iface=/tmp/vhost-user1,queues=4 -- -i --rxq=4 --txq=4 [0]: https://doc.dpdk.org/guides/testpmd_app_ug/index.html Maxime Thanks! Thanks, Stefano And we want to explicitly enable CVQ first, which "only" vhost_net knows which is. To perform that in vhost_vdpa_dev_start would require quirks, involving one or more of: * Ignore vq enable calls if the device is not the CVQ one. How to signal what is the CVQ? Can we trust it will be the last one for all kind of devices? * Enable queues that do not belong to the last vhost_dev from the enable call. * Enable the rest of the queues from the last enable in reverse order. * Intercalate the "net load" callback between enabling the last vhost_vdpa device and enabling the rest of devices. * Add an "enable priority" order? Thanks! Thanks if (peer->vring_enable) { /* restore vring enable state */ @@ -408,11 +420,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, goto err_start; } } - -r = vhost_net_start_one(get_vhost_net(peer), dev); -if (r < 0) { -goto err_start; -} } return 0; -- 2.31.1
Re: [PULL v4 76/83] vhost-user: Support vhost_dev_start
On 1/17/23 13:36, Greg Kurz wrote: On Tue, 17 Jan 2023 13:12:57 +0100 Greg Kurz wrote: Hi Maxime, On Tue, 17 Jan 2023 10:49:37 +0100 Maxime Coquelin wrote: Hi Yajun, On 1/16/23 08:14, Yajun Wu wrote: Not quite sure about the whole picture. Seems while qemu waiting response of vhost_user_get_status, dpdk send out VHOST_USER_SLAVE_IOTLB_MSG and trigger qemu function vhost_backend_update_device_iotlb. Qemu wait on reply of VHOST_USER_IOTLB_MSG but get VHOST_USER_GET_STATUS reply. Thanks for the backtrace, that helps a lot. The issue happens because: 1. Introduction of nested event loop in vhost_user_read() [0] features that enables handling slave channel request while waiting for reply on the masyer channel. 2. Slave VHOST_USER_SLAVE_IOTLB_MSG slave request handling ends-up sending a VHOST_USER_IOTLB_MSG on the master channel. So while waiting for VHOST_USER_IOTLB_MSG reply, it receives the reply for the first request sent on the master channel, here the VHOST_USER_GET_STATUS reply. I don't see an easy way to fix it. One option would be to have the slave channel being handled by another thread, and protect master channel with a lock to enforce the synchronization. But this may induce other issues, so that's not a light change. This is going to be tough because the back-end might have set the VHOST_USER_NEED_REPLY_MASK flag on the VHOST_USER_SLAVE_IOTLB_MSG request and thus might be waiting for a response on the slave channel. In order to emit such a response, the front-end must send VHOST_USER_SLAVE_IOTLB_MSG updates on the master channel *first* according to the protocol specification. This means that we really cannot handle VHOST_USER_SLAVE_IOTLB_MSG requests while there's an on-going transaction on the master channel. Since the slave channel would be handled on another thread, it means the on-going transaction on the master channel can continue. Once done, it will release the mutex, and the thread handling the slave channel can take it send the IOTLB update on the master channel. That would work with DPDK, which does not request reply-ack on IOTLB misses. For the DAX enablement case, my understanding is that the handling of the slave requests by QEMU does not induce sending requests on the master channel, so if I'm not mistaken, it should work too. The thread handling the slave requests can send the reply on the slave channel, while the thread handling the master channel is blocked waiting for the GET_VRING_BASE reply. Is that correct? (Adding Greg and Stefan, who worked on the nested event loop series.) Simply reverting nested event loop support may not be an option, since it would break virtiofsd, as if my understanding is correct, waits for some slave channel request to complete in order to complete a request made by QEMU on the master channel. Any thougths? Well... the nested even loop was added as preparatory work for "the upcoming enablement of DAX with virtio-fs". This requires changes on the QEMU side that haven't been merged yet. Technically, it seems that reverting the nested event loop won't break anything in upstream QEMU at this point (but this will bite again as soon as DAX enablement gets merged). Cc'ing Dave to know about the DAX enablement status. AFAIK the event loop is only needed for the VHOST_USER_GET_VRING_BASE message. Another possibility might be to create the nested event loop in this case only : this would allow VHOST_USER_GET_STATUS to complete before QEMU starts processing the VHOST_USER_SLAVE_IOTLB_MSG requests. Cheers, -- Greg Maxime [0]: https://patchwork.ozlabs.org/project/qemu-devel/patch/20210312092212.782255-6-gr...@kaod.org/ Break on first error message("Received unexpected msg type. Expected 22 received 40") #0 0x55b72ed4 in process_message_reply (dev=0x584dd600, msg=0x7fffa330) at ../hw/virtio/vhost-user.c:445 #1 0x55b77c26 in vhost_user_send_device_iotlb_msg (dev=0x584dd600, imsg=0x7fffa600) at ../hw/virtio/vhost-user.c:2341 #2 0x55b7179e in vhost_backend_update_device_iotlb (dev=0x584dd600, iova=10442706944, uaddr=140736119902208, len=4096, perm=IOMMU_RW) at ../hw/virtio/vhost-backend.c:361 #3 0x55b6e34c in vhost_device_iotlb_miss (dev=0x584dd600, iova=10442706944, write=1) at ../hw/virtio/vhost.c:1113 #4 0x55b718d9 in vhost_backend_handle_iotlb_msg (dev=0x584dd600, imsg=0x7fffa7b0) at ../hw/virtio/vhost-backend.c:393 #5 0x55b76144 in slave_read (ioc=0x57a38680, condition=G_IO_IN, opaque=0x584dd600) at ../hw/virtio/vhost-user.c:1726 #6 0x55c797a5 in qio_channel_fd_source_dispatch (source=0x56a06fb0, callback=0x55b75f86 , user_data=0x584dd600) at ../io/channel-watch.c:84 #7 0x7554895d in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #8 0x75548d18 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
Re: [PULL v4 76/83] vhost-user: Support vhost_dev_start
andle_iotlb_msg (dev=0x584dd600, imsg=0x7fffbf70) at ../hw/virtio/vhost-backend.c:393 #27 0x55b76144 in slave_read (ioc=0x57a38680, condition=G_IO_IN, opaque=0x584dd600) at ../hw/virtio/vhost-user.c:1726 #28 0x55c797a5 in qio_channel_fd_source_dispatch (source=0x56f1a530, callback=0x55b75f86 , user_data=0x584dd600) at ../io/channel-watch.c:84 #29 0x7554895d in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #30 0x75548d18 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0 #31 0x75549042 in g_main_loop_run () at /lib64/libglib-2.0.so.0 #32 0x55b72de7 in vhost_user_read (dev=0x584dd600, msg=0x7fffc420) at ../hw/virtio/vhost-user.c:413 #33 0x55b754b1 in vhost_user_get_u64 (dev=0x584dd600, request=40, u64=0x7fffc6e0) at ../hw/virtio/vhost-user.c:1349 #34 0x55b758ff in vhost_user_get_status (dev=0x584dd600, status=0x7fffc713 "W\020") at ../hw/virtio/vhost-user.c:1474 #35 0x55b75967 in vhost_user_add_status (dev=0x584dd600, status=7 '\a') at ../hw/virtio/vhost-user.c:1488 #36 0x55b78bf6 in vhost_user_dev_start (dev=0x584dd600, started=true) at ../hw/virtio/vhost-user.c:2758 #37 0x55b709ad in vhost_dev_start (hdev=0x584dd600, vdev=0x57b965d0, vrings=false) at ../hw/virtio/vhost.c:1988 #38 0x5584291c in vhost_net_start_one (net=0x584dd600, dev=0x57b965d0) at ../hw/net/vhost_net.c:271 #39 0x55842f1e in vhost_net_start (dev=0x57b965d0, ncs=0x57bc09e0, data_queue_pairs=1, cvq=0) at ../hw/net/vhost_net.c:412 #40 0x55b1bf61 in virtio_net_vhost_status (n=0x57b965d0, status=15 '\017') at ../hw/net/virtio-net.c:311 #41 0x55b1c20c in virtio_net_set_status (vdev=0x57b965d0, status=15 '\017') at ../hw/net/virtio-net.c:392 #42 0x55b1ed04 in virtio_net_handle_mq (n=0x57b965d0, cmd=0 '\000', iov=0x56c7ef50, iov_cnt=1) at ../hw/net/virtio-net.c:1497 #43 0x55b1eef0 in virtio_net_handle_ctrl_iov (vdev=0x57b965d0, in_sg=0x56a09880, in_num=1, out_sg=0x56a09890, out_num=1) at ../hw/net/virtio-net.c:1534 #44 0x55b1efe9 in virtio_net_handle_ctrl (vdev=0x57b965d0, vq=0x7fffc04ac140) at ../hw/net/virtio-net.c:1557 #45 0x55b63776 in virtio_queue_notify_vq (vq=0x7fffc04ac140) at ../hw/virtio/virtio.c:2249 #46 0x55b669dc in virtio_queue_host_notifier_read (n=0x7fffc04ac1b4) at ../hw/virtio/virtio.c:3529 #47 0x55e3f458 in aio_dispatch_handler (ctx=0x56a016c0, node=0x7ffd8800e430) at ../util/aio-posix.c:369 #48 0x55e3f613 in aio_dispatch_handlers (ctx=0x56a016c0) at ../util/aio-posix.c:412 #49 0x55e3f669 in aio_dispatch (ctx=0x56a016c0) at ../util/aio-posix.c:422 #50 0x55e585de in aio_ctx_dispatch (source=0x56a016c0, callback=0x0, user_data=0x0) at ../util/async.c:321 #51 0x7554895d in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #52 0x55e5abea in glib_pollfds_poll () at ../util/main-loop.c:295 #53 0x55e5ac64 in os_host_main_loop_wait (timeout=0) at ../util/main-loop.c:318 #54 0x55e5ad69 in main_loop_wait (nonblocking=0) at ../util/main-loop.c:604 #55 0x559693de in qemu_main_loop () at ../softmmu/runstate.c:731 #56 0x556e7c06 in qemu_default_main () at ../softmmu/main.c:37 #57 0x556e7c3c in main (argc=71, argv=0x7fffcda8) at ../softmmu/main.c:48 -Original Message- From: Maxime Coquelin Sent: Thursday, January 12, 2023 5:26 PM To: Laurent Vivier Cc: qemu-devel@nongnu.org; Peter Maydell ; Yajun Wu ; Parav Pandit ; Michael S. Tsirkin Subject: Re: [PULL v4 76/83] vhost-user: Support vhost_dev_start External email: Use caution opening links or attachments Hi Laurent, On 1/11/23 10:50, Laurent Vivier wrote: On 1/9/23 11:55, Michael S. Tsirkin wrote: On Fri, Jan 06, 2023 at 03:21:43PM +0100, Laurent Vivier wrote: Hi, it seems this patch breaks vhost-user with DPDK. See https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu gzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D2155173&data=05%7C01%7Cyajun w%40nvidia.com%7C47e6e0fabd044383fd3308daf47f0253%7C43083d15727340c1 b7db39efd9ccc17a%7C0%7C0%7C638091123577559319%7CUnknown%7CTWFpbGZsb3 d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D %7C3000%7C%7C%7C&sdata=1pjChYTKHVmBoempNitiZHBdrlPIMFjKoD6FeOVSay0%3 D&reserved=0 it seems QEMU doesn't receive the expected commands sequence: Received unexpected msg type. Expected 22 received 40 Fail to update device iotlb Received unexpected msg type. Expected 40 received 22 Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 1 ring restore failed: -71: Protocol error (71) Received unexpected msg type. Expected 22 received 11 Fail to up
Re: [PULL v4 76/83] vhost-user: Support vhost_dev_start
Hi Laurent, On 1/11/23 10:50, Laurent Vivier wrote: On 1/9/23 11:55, Michael S. Tsirkin wrote: On Fri, Jan 06, 2023 at 03:21:43PM +0100, Laurent Vivier wrote: Hi, it seems this patch breaks vhost-user with DPDK. See https://bugzilla.redhat.com/show_bug.cgi?id=2155173 it seems QEMU doesn't receive the expected commands sequence: Received unexpected msg type. Expected 22 received 40 Fail to update device iotlb Received unexpected msg type. Expected 40 received 22 Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 1 ring restore failed: -71: Protocol error (71) Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 0 ring restore failed: -71: Protocol error (71) unable to start vhost net: 71: falling back on userspace virtio It receives VHOST_USER_GET_STATUS (40) when it expects VHOST_USER_IOTLB_MSG (22) and VHOST_USER_IOTLB_MSG when it expects VHOST_USER_GET_STATUS. and VHOST_USER_GET_VRING_BASE (11) when it expect VHOST_USER_GET_STATUS and so on. Any idea? We only have a single thread on DPDK side to handle Vhost-user requests, it will read a request, handle it and reply to it. Then it reads the next one, etc... So I don't think it is possible to mix request replies order on DPDK side. Maybe there are two threads concurrently sending requests on QEMU side? Regards, Maxime Thanks, Laurent So I am guessing it's coming from: if (msg.hdr.request != request) { error_report("Received unexpected msg type. Expected %d received %d", request, msg.hdr.request); return -EPROTO; } in process_message_reply and/or in vhost_user_get_u64. On 11/7/22 23:53, Michael S. Tsirkin wrote: From: Yajun Wu The motivation of adding vhost-user vhost_dev_start support is to improve backend configuration speed and reduce live migration VM downtime. Today VQ configuration is issued one by one. For virtio net with multi-queue support, backend needs to update RSS (Receive side scaling) on every rx queue enable. Updating RSS is time-consuming (typical time like 7ms). Implement already defined vhost status and message in the vhost specification [1]. (a) VHOST_USER_PROTOCOL_F_STATUS (b) VHOST_USER_SET_STATUS (c) VHOST_USER_GET_STATUS Send message VHOST_USER_SET_STATUS with VIRTIO_CONFIG_S_DRIVER_OK for device start and reset(0) for device stop. On reception of the DRIVER_OK message, backend can apply the needed setting only once (instead of incremental) and also utilize parallelism on enabling queues. This improves QEMU's live migration downtime with vhost user backend implementation by great margin, specially for the large number of VQs of 64 from 800 msec to 250 msec. [1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html Signed-off-by: Yajun Wu Acked-by: Parav Pandit Message-Id: <20221017064452.1226514-3-yaj...@nvidia.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Probably easiest to debug from dpdk side. Does the problem go away if you disable the feature VHOST_USER_PROTOCOL_F_STATUS in dpdk? Maxime could you help to debug this? Thanks, Laurent
Re: [PATCH v6 5/8] libvduse: Add VDUSE (vDPA Device in Userspace) library
Hi Yongji, On 5/23/22 10:46, Xie Yongji wrote: VDUSE [1] is a linux framework that makes it possible to implement software-emulated vDPA devices in userspace. This adds a library as a subproject to help implementing VDUSE backends in QEMU. [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html Signed-off-by: Xie Yongji --- MAINTAINERS |5 + meson.build | 15 + meson_options.txt |2 + scripts/meson-buildoptions.sh |3 + subprojects/libvduse/include/atomic.h |1 + subprojects/libvduse/include/compiler.h |1 + subprojects/libvduse/libvduse.c | 1167 +++ subprojects/libvduse/libvduse.h | 235 subprojects/libvduse/linux-headers/linux|1 + subprojects/libvduse/meson.build| 10 + subprojects/libvduse/standard-headers/linux |1 + 11 files changed, 1441 insertions(+) create mode 12 subprojects/libvduse/include/atomic.h create mode 12 subprojects/libvduse/include/compiler.h create mode 100644 subprojects/libvduse/libvduse.c create mode 100644 subprojects/libvduse/libvduse.h create mode 12 subprojects/libvduse/linux-headers/linux create mode 100644 subprojects/libvduse/meson.build create mode 12 subprojects/libvduse/standard-headers/linux ... diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c new file mode 100644 index 00..fa4822b9a9 --- /dev/null +++ b/subprojects/libvduse/libvduse.c @@ -0,0 +1,1167 @@ ... + +int vduse_dev_destroy(VduseDev *dev) +{ +int ret = 0; + +free(dev->vqs); +if (dev->fd > 0) { if (dev->fd >= 0) { +close(dev->fd); +dev->fd = -1; +} +if (dev->ctrl_fd > 0) { if (dev->ctrl_fd >= 0) { +if (ioctl(dev->ctrl_fd, VDUSE_DESTROY_DEV, dev->name)) { +ret = -errno; +} +close(dev->ctrl_fd); +dev->ctrl_fd = -1; +} +free(dev->name); +free(dev); + +return ret; +} Maxime
[PATCH 2/5] virtio-net: prepare for variable RSS key and indir table lengths
This patch is a preliminary rework to support RSS with Vhost-user backends. It enables supporting different types of hashes, key lengths and indirection table lengths. This patch does not introduces behavioral changes. Signed-off-by: Maxime Coquelin --- ebpf/ebpf_rss.c| 8 hw/net/virtio-net.c| 35 +- include/hw/virtio/virtio-net.h | 16 +--- include/migration/vmstate.h| 10 ++ 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c index 4a63854175..f03be5f919 100644 --- a/ebpf/ebpf_rss.c +++ b/ebpf/ebpf_rss.c @@ -96,7 +96,7 @@ static bool ebpf_rss_set_indirections_table(struct EBPFRSSContext *ctx, uint32_t i = 0; if (!ebpf_rss_is_loaded(ctx) || indirections_table == NULL || - len > VIRTIO_NET_RSS_MAX_TABLE_LEN) { + len > VIRTIO_NET_RSS_DEFAULT_TABLE_LEN) { return false; } @@ -116,13 +116,13 @@ static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx, uint32_t map_key = 0; /* prepare toeplitz key */ -uint8_t toe[VIRTIO_NET_RSS_MAX_KEY_SIZE] = {}; +uint8_t toe[VIRTIO_NET_RSS_DEFAULT_KEY_SIZE] = {}; if (!ebpf_rss_is_loaded(ctx) || toeplitz_key == NULL || -len != VIRTIO_NET_RSS_MAX_KEY_SIZE) { +len != VIRTIO_NET_RSS_DEFAULT_KEY_SIZE) { return false; } -memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_MAX_KEY_SIZE); +memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_DEFAULT_KEY_SIZE); *(uint32_t *)toe = ntohl(*(uint32_t *)toe); if (bpf_map_update_elem(ctx->map_toeplitz_key, &map_key, toe, diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 73145d6390..38436e472b 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -137,12 +137,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) memcpy(netcfg.mac, n->mac, ETH_ALEN); virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed); netcfg.duplex = n->net_conf.duplex; -netcfg.rss_max_key_size = VIRTIO_NET_RSS_MAX_KEY_SIZE; +netcfg.rss_max_key_size = n->rss_capa.max_key_size; virtio_stw_p(vdev, &netcfg.rss_max_indirection_table_length, - virtio_host_has_feature(vdev, VIRTIO_NET_F_RSS) ? - VIRTIO_NET_RSS_MAX_TABLE_LEN : 1); + n->rss_capa.max_indirection_len); virtio_stl_p(vdev, &netcfg.supported_hash_types, - VIRTIO_NET_RSS_SUPPORTED_HASHES); + n->rss_capa.supported_hashes); memcpy(config, &netcfg, n->config_size); /* @@ -1202,7 +1201,7 @@ static bool virtio_net_attach_epbf_rss(VirtIONet *n) if (!ebpf_rss_set_all(&n->ebpf_rss, &config, n->rss_data.indirections_table, n->rss_data.key, - VIRTIO_NET_RSS_MAX_KEY_SIZE)) { + n->rss_data.key_len)) { return false; } @@ -1277,7 +1276,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, err_value = n->rss_data.indirections_len; goto error; } -if (n->rss_data.indirections_len > VIRTIO_NET_RSS_MAX_TABLE_LEN) { +if (n->rss_data.indirections_len > n->rss_capa.max_indirection_len) { err_msg = "Too large indirection table"; err_value = n->rss_data.indirections_len; goto error; @@ -1323,7 +1322,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, err_value = queue_pairs; goto error; } -if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) { +if (temp.b > n->rss_capa.max_key_size) { err_msg = "Invalid key size"; err_value = temp.b; goto error; @@ -1339,6 +1338,14 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, } offset += size_get; size_get = temp.b; +n->rss_data.key_len = temp.b; +g_free(n->rss_data.key); +n->rss_data.key = g_malloc(size_get); +if (!n->rss_data.key) { +err_msg = "Can't allocate key"; +err_value = n->rss_data.key_len; +goto error; +} s = iov_to_buf(iov, iov_cnt, offset, n->rss_data.key, size_get); if (s != size_get) { err_msg = "Can get key buffer"; @@ -3093,8 +3100,9 @@ static const VMStateDescription vmstate_virtio_net_rss = { VMSTATE_UINT32(rss_data.hash_types, VirtIONet), VMSTATE_UINT16(rss_data.indirections_len, VirtIONet), VMSTATE_UINT16(rss_data.default_queue, VirtIONet), -VMSTATE_UINT8_ARRAY(rss_data.key, VirtIONet, -VIRTIO_NET_RSS_MAX_KEY_SIZE), +VMSTATE_VARRAY_UINT8_ALLOC(rss_data.key, VirtIONet, + rss_data.key_len, 0, + vmstate_info_uint8, uint8_t), VMSTATE_VARRAY_UINT16_ALLOC(rss_data.indirections_t
[PATCH 3/5] virtio-net: add RSS support for Vhost backends
This patch introduces new Vhost backend callbacks to support RSS, and makes them called in Virtio-net device. It will be used by Vhost-user backend implementation to support RSS feature. Signed-off-by: Maxime Coquelin --- hw/net/vhost_net-stub.c | 10 ++ hw/net/vhost_net.c| 22 + hw/net/virtio-net.c | 53 +-- include/hw/virtio/vhost-backend.h | 7 include/net/vhost_net.h | 4 +++ 5 files changed, 79 insertions(+), 17 deletions(-) diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c index 89d71cfb8e..cc05e07c1f 100644 --- a/hw/net/vhost_net-stub.c +++ b/hw/net/vhost_net-stub.c @@ -101,3 +101,13 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu) { return 0; } + +int vhost_net_get_rss(struct vhost_net *net, VirtioNetRssCapa *rss_capa) +{ +return 0; +} + +int vhost_net_set_rss(struct vhost_net *net, VirtioNetRssData *rss_data) +{ +return 0; +} diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 30379d2ca4..aa2a1e8e5f 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -512,3 +512,25 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu) return vhost_ops->vhost_net_set_mtu(&net->dev, mtu); } + +int vhost_net_get_rss(struct vhost_net *net, VirtioNetRssCapa *rss_capa) +{ +const VhostOps *vhost_ops = net->dev.vhost_ops; + +if (!vhost_ops->vhost_net_get_rss) { +return 0; +} + +return vhost_ops->vhost_net_get_rss(&net->dev, rss_capa); +} + +int vhost_net_set_rss(struct vhost_net *net, VirtioNetRssData *rss_data) +{ +const VhostOps *vhost_ops = net->dev.vhost_ops; + +if (!vhost_ops->vhost_net_set_rss) { +return 0; +} + +return vhost_ops->vhost_net_set_rss(&net->dev, rss_data); +} diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 38436e472b..237bbdb1b3 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -741,8 +741,10 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, return features; } -if (!ebpf_rss_is_loaded(&n->ebpf_rss)) { -virtio_clear_feature(&features, VIRTIO_NET_F_RSS); +if (nc->peer->info->type == NET_CLIENT_DRIVER_TAP) { +if (!ebpf_rss_is_loaded(&n->ebpf_rss)) { +virtio_clear_feature(&features, VIRTIO_NET_F_RSS); +} } features = vhost_net_get_features(get_vhost_net(nc->peer), features); vdev->backend_features = features; @@ -1161,11 +1163,17 @@ static void virtio_net_detach_epbf_rss(VirtIONet *n); static void virtio_net_disable_rss(VirtIONet *n) { +NetClientState *nc = qemu_get_queue(n->nic); + if (n->rss_data.enabled) { trace_virtio_net_rss_disable(); } n->rss_data.enabled = false; +if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) { +vhost_net_set_rss(get_vhost_net(nc->peer), &n->rss_data); +} + virtio_net_detach_epbf_rss(n); } @@ -1239,6 +1247,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, bool do_rss) { VirtIODevice *vdev = VIRTIO_DEVICE(n); +NetClientState *nc = qemu_get_queue(n->nic); struct virtio_net_rss_config cfg; size_t s, offset = 0, size_get; uint16_t queue_pairs, i; @@ -1354,22 +1363,29 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, } n->rss_data.enabled = true; -if (!n->rss_data.populate_hash) { -if (!virtio_net_attach_epbf_rss(n)) { -/* EBPF must be loaded for vhost */ -if (get_vhost_net(qemu_get_queue(n->nic)->peer)) { -warn_report("Can't load eBPF RSS for vhost"); -goto error; +if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) { +if (vhost_net_set_rss(get_vhost_net(nc->peer), &n->rss_data)) { +warn_report("Failed to configure RSS for vhost-user"); +goto error; +} +} else { +if (!n->rss_data.populate_hash) { +if (!virtio_net_attach_epbf_rss(n)) { +/* EBPF must be loaded for vhost */ +if (get_vhost_net(nc->peer)) { +warn_report("Can't load eBPF RSS for vhost"); +goto error; +} +/* fallback to software RSS */ +warn_report("Can't load eBPF RSS - fallback to software RSS"); +n->rss_data.enabled_software_rss = true; } -/* fallback to software RSS */ -warn_report("Can't load eBPF RSS - fallback to software RSS"); +} else { +/* use software RSS for hash populating */ +/* and detach eBPF if wa
[PATCH 5/5] vhost-user: add RSS support
This patch implements the RSS feature to the Vhost-user backend. The implementation supports up to 52 bytes RSS key length, and 512 indirection table entries. Signed-off-by: Maxime Coquelin --- hw/virtio/vhost-user.c | 146 - 1 file changed, 145 insertions(+), 1 deletion(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6abbc9da32..d047da81ba 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -81,6 +81,8 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, /* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */ VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, +/* Feature 16 reserved for VHOST_USER_PROTOCOL_F_STATUS. */ +VHOST_USER_PROTOCOL_F_NET_RSS = 17, VHOST_USER_PROTOCOL_F_MAX }; @@ -126,6 +128,10 @@ typedef enum VhostUserRequest { VHOST_USER_GET_MAX_MEM_SLOTS = 36, VHOST_USER_ADD_MEM_REG = 37, VHOST_USER_REM_MEM_REG = 38, +/* Message number 39 reserved for VHOST_USER_SET_STATUS. */ +/* Message number 40 reserved for VHOST_USER_GET_STATUS. */ +VHOST_USER_NET_GET_RSS = 41, +VHOST_USER_NET_SET_RSS = 42, VHOST_USER_MAX } VhostUserRequest; @@ -196,6 +202,24 @@ typedef struct VhostUserInflight { uint16_t queue_size; } VhostUserInflight; +typedef struct VhostUserRSSCapa { +uint32_t supported_hash_types; +uint8_t max_key_len; +uint16_t max_indir_len; +} VhostUserRSSCapa; + +#define VHOST_USER_RSS_MAX_KEY_LEN52 +#define VHOST_USER_RSS_MAX_INDIR_LEN 512 + +typedef struct VhostUserRSSData { +uint32_t hash_types; +uint8_t key_len; +uint8_t key[VHOST_USER_RSS_MAX_KEY_LEN]; +uint16_t indir_len; +uint16_t indir_table[VHOST_USER_RSS_MAX_INDIR_LEN]; +uint16_t default_queue; +} VhostUserRSSData; + typedef struct { VhostUserRequest request; @@ -220,6 +244,8 @@ typedef union { VhostUserCryptoSession session; VhostUserVringArea area; VhostUserInflight inflight; +VhostUserRSSCapa rss_capa; +VhostUserRSSData rss_data; } VhostUserPayload; typedef struct VhostUserMsg { @@ -2178,7 +2204,123 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) return ret; } -/* If reply_ack supported, slave has to ack specified MTU is valid */ +if (reply_supported) { +return process_message_reply(dev, &msg); +} + +return 0; +} + +static int vhost_user_net_get_rss(struct vhost_dev *dev, + VirtioNetRssCapa *rss_capa) +{ +int ret; +VhostUserMsg msg = { +.hdr.request = VHOST_USER_NET_GET_RSS, +.hdr.flags = VHOST_USER_VERSION, +}; + +if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_RSS))) { +return -EPROTO; +} + +ret = vhost_user_write(dev, &msg, NULL, 0); +if (ret < 0) { +return ret; +} + +ret = vhost_user_read(dev, &msg); +if (ret < 0) { +return ret; +} + +if (msg.hdr.request != VHOST_USER_NET_GET_RSS) { +error_report("Received unexpected msg type. Expected %d received %d", + VHOST_USER_NET_GET_RSS, msg.hdr.request); +return -EPROTO; +} + +if (msg.hdr.size != sizeof(msg.payload.rss_capa)) { +error_report("Received bad msg size."); +return -EPROTO; +} + +if (msg.payload.rss_capa.max_key_len < VIRTIO_NET_RSS_MIN_KEY_SIZE) { +error_report("Invalid max RSS key len (%uB, minimum %uB).", + msg.payload.rss_capa.max_key_len, + VIRTIO_NET_RSS_MIN_KEY_SIZE); +return -EINVAL; +} + +if (msg.payload.rss_capa.max_indir_len < VIRTIO_NET_RSS_MIN_TABLE_LEN) { +error_report("Invalid max RSS indir table entries (%u, minimum %u).", + msg.payload.rss_capa.max_indir_len, + VIRTIO_NET_RSS_MIN_TABLE_LEN); +return -EINVAL; +} + +rss_capa->supported_hashes = msg.payload.rss_capa.supported_hash_types; +rss_capa->max_key_size = MIN(msg.payload.rss_capa.max_key_len, + VHOST_USER_RSS_MAX_KEY_LEN); +rss_capa->max_indirection_len = MIN(msg.payload.rss_capa.max_indir_len, +VHOST_USER_RSS_MAX_INDIR_LEN); + +return 0; +} + +static int vhost_user_net_set_rss(struct vhost_dev *dev, + VirtioNetRssData *rss_data) +{ +VhostUserMsg msg; +bool reply_supported = virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_REPLY_ACK); +int ret; + +if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_RSS))) { +return -EPROTO; +} + +msg.hdr.request = VHOST_USER_NET_SET_RSS; +msg.hdr.
[PATCH 4/5] docs: introduce RSS support in Vhost-user specification
This patch documents RSS feature in Vhost-user specification. Two new requests are introduced backed by a dedicated protocol feature. First one is to query the Vhost-user slave RSS capabilities such as supported hash types, maximum key length and indirection table size. The second one is to provide the slave with driver's RSS configuration. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.rst | 57 + 1 file changed, 57 insertions(+) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 4dbc84fd00..9de6297568 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -258,6 +258,42 @@ Inflight description :queue size: a 16-bit size of virtqueues +RSS capabilities description + + ++--+-+---+ +| supported hash types | max key len | max indir len | ++--+-+---+ + +:supported hash types: a 32-bit bitfield of supported hash types as defined + in the Virtio specification + +:max key len: a 8-bit maximum size of the RSS key + +:max indir len: a 16-bits maximum size of the RSS indirection table + +RSS data description + + +++-+-+---+-+---+ +| hash types | key len | key | indir len | indir table | default queue | +++-+-+---+-+---+ + +:hash types: a 32-bit bitfield of supported hash types as defined in the + Virtio specification + +:key len: 8-bit size of the RSS key + +:key: a 8-bit array of 52 elements containing the RSS key + +:indir len: a 16-bit size of the RSS indirection table + +:indir table: a 16-bit array of 512 elements containing the hash indirection + table + +:default queue: the default queue index for flows not matching requested hash +types + C structure --- @@ -858,6 +894,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 #define VHOST_USER_PROTOCOL_F_STATUS 16 + #define VHOST_USER_PROTOCOL_F_NET_RSS 17 Master message types @@ -1371,6 +1408,26 @@ Master message types query the backend for its device status as defined in the Virtio specification. +``VHOST_USER_NET_GET_RSS`` + :id: 41 + :equivalent ioctl: N/A + :slave payload: RSS capabilities description + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_NET_RSS`` protocol has been successfully + negotiated, this message is submitted by the master to get the RSS + capabilities of the slave. + +``VHOST_USER_NET_SET_RSS`` + :id: 42 + :equivalent ioctl: N/A + :slave payload: N/A + :master payload: RSS data description + + When the ``VHOST_USER_PROTOCOL_F_NET_RSS`` protocol has been successfully + negotiated, this message is submitted by the master to set the RSS + configuration defined by the Virtio driver. + Slave message types --- -- 2.35.1
[PATCH 1/5] ebpf: pass and check RSS key length to the loader
This patch is preliminary rework to support RSS with Vhost-user backends. The Vhost-user implementation will allow RSS hash key of 40 bytes or more as allowed by the Virtio specification, whereas the eBPF-based Vhost-kernel solution only supports 40 bytes keys. This patch adds the RSS key length to the loader, and validate it is 40 bytes before copying it. Signed-off-by: Maxime Coquelin --- ebpf/ebpf_rss-stub.c | 3 ++- ebpf/ebpf_rss.c | 11 +++ ebpf/ebpf_rss.h | 3 ++- hw/net/virtio-net.c | 3 ++- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/ebpf/ebpf_rss-stub.c b/ebpf/ebpf_rss-stub.c index e71e229190..ffc5c5574f 100644 --- a/ebpf/ebpf_rss-stub.c +++ b/ebpf/ebpf_rss-stub.c @@ -29,7 +29,8 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx) } bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config, - uint16_t *indirections_table, uint8_t *toeplitz_key) + uint16_t *indirections_table, uint8_t *toeplitz_key, + uint8_t key_len) { return false; } diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c index 118c68da83..4a63854175 100644 --- a/ebpf/ebpf_rss.c +++ b/ebpf/ebpf_rss.c @@ -110,14 +110,16 @@ static bool ebpf_rss_set_indirections_table(struct EBPFRSSContext *ctx, } static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx, - uint8_t *toeplitz_key) + uint8_t *toeplitz_key, + size_t len) { uint32_t map_key = 0; /* prepare toeplitz key */ uint8_t toe[VIRTIO_NET_RSS_MAX_KEY_SIZE] = {}; -if (!ebpf_rss_is_loaded(ctx) || toeplitz_key == NULL) { +if (!ebpf_rss_is_loaded(ctx) || toeplitz_key == NULL || +len != VIRTIO_NET_RSS_MAX_KEY_SIZE) { return false; } memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_MAX_KEY_SIZE); @@ -131,7 +133,8 @@ static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx, } bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config, - uint16_t *indirections_table, uint8_t *toeplitz_key) + uint16_t *indirections_table, uint8_t *toeplitz_key, + uint8_t key_len) { if (!ebpf_rss_is_loaded(ctx) || config == NULL || indirections_table == NULL || toeplitz_key == NULL) { @@ -147,7 +150,7 @@ bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config, return false; } -if (!ebpf_rss_set_toepliz_key(ctx, toeplitz_key)) { +if (!ebpf_rss_set_toepliz_key(ctx, toeplitz_key, key_len)) { return false; } diff --git a/ebpf/ebpf_rss.h b/ebpf/ebpf_rss.h index bf3f2572c7..db23ccd25f 100644 --- a/ebpf/ebpf_rss.h +++ b/ebpf/ebpf_rss.h @@ -37,7 +37,8 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx); bool ebpf_rss_load(struct EBPFRSSContext *ctx); bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config, - uint16_t *indirections_table, uint8_t *toeplitz_key); + uint16_t *indirections_table, uint8_t *toeplitz_key, + uint8_t key_len); void ebpf_rss_unload(struct EBPFRSSContext *ctx); diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 1067e72b39..73145d6390 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1201,7 +1201,8 @@ static bool virtio_net_attach_epbf_rss(VirtIONet *n) rss_data_to_rss_config(&n->rss_data, &config); if (!ebpf_rss_set_all(&n->ebpf_rss, &config, - n->rss_data.indirections_table, n->rss_data.key)) { + n->rss_data.indirections_table, n->rss_data.key, + VIRTIO_NET_RSS_MAX_KEY_SIZE)) { return false; } -- 2.35.1
[PATCH 0/5] Vhost-user: add Virtio RSS support
The goal of this series is to add support for Virtio RSS feature to the Vhost-user backend. First patches are preliminary reworks to support variable RSS key and indirection table length. eBPF change only adds checks on whether the key length is 40B, it does not add support for longer keys. Vhost-user implementation supports up to 52B RSS key, in order to match with the maximum supported by physical NICs (Intel E810). Idea is that it could be used for application like Virtio-forwarder, by programming the Virtio device RSS key into the physical NIC and let the physical NIC do the packets distribution. DPDK Vhost-user backend PoC implementing the new requests can be found here [0], it only implements the messages handling, it does not perform any RSS for now. [0]: https://gitlab.com/mcoquelin/dpdk-next-virtio/-/commits/vhost_user_rss_poc/ Maxime Coquelin (5): ebpf: pass and check RSS key length to the loader virtio-net: prepare for variable RSS key and indir table lengths virtio-net: add RSS support for Vhost backends docs: introduce RSS support in Vhost-user specification vhost-user: add RSS support docs/interop/vhost-user.rst | 57 ebpf/ebpf_rss-stub.c | 3 +- ebpf/ebpf_rss.c | 17 ++-- ebpf/ebpf_rss.h | 3 +- hw/net/vhost_net-stub.c | 10 ++ hw/net/vhost_net.c| 22 + hw/net/virtio-net.c | 87 +- hw/virtio/vhost-user.c| 146 +- include/hw/virtio/vhost-backend.h | 7 ++ include/hw/virtio/virtio-net.h| 16 +++- include/migration/vmstate.h | 10 ++ include/net/vhost_net.h | 4 + 12 files changed, 344 insertions(+), 38 deletions(-) -- 2.35.1
Re: [PATCH v3] vdpa: reset the backend device in the end of vhost_net_stop()
Hi, On 3/31/22 10:55, Jason Wang wrote: On Thu, Mar 31, 2022 at 1:20 PM <08005...@163.com> wrote: Hi: For some reason, I see the patch as an attachment. We are starting to see this more and more since yesterday on DPDK mailing list. It seems like an issue with mimecast, when the From: tag is different from the sender. Maxime Thanks
Re: VHOST_USER_PROTOCOL_F_VDPA
Hi Stefan, On 8/21/20 1:08 PM, Stefan Hajnoczi wrote: > The first vDPA ioctls have been added to the vhost-user protocol and I > wonder if it's time to fully change the vhost-user protocol's focus to > providing a full VIRTIO device model like vDPA does. > > Initially vhost-user was just used for vhost-net. As a result it didn't > need the full VIRTIO device model including the configuration space and > device status register. > > Over the years device-specific messages were added to extend vhost-user > to cover more of the VIRTIO device model. vhost-user-blk needed > configuration space support, for example. > > The problem for VMMs and device backend implementors is that the > protocol is currently positioned halfway between the original vhost-net > approach and the full VIRTIO device model. Even if a VMM implements > VHOST_USER_GET_CONFIG, it can only expect it to work with > vhost-user-blk, not vhost-user-net. Similarly, a vhost-user-net device > backend cannot implement VHOST_USER_GET_CONFIG and expect all VMMs to > allow it to participate in configuration space emulation because > existing VMMs won't send that message. > > The current approach where each device type uses a undocumented subset > of vhost-user messages is really messy. VMM and device backend > implementors have to look at existing implementations to know what is > expected for a given device type. It would be nice to switch to the > VIRTIO device model so that the VIRTIO specification can be used as the > reference for how device types work. > > Now that vDPA is here and extends the kernel vhost ioctls with a full > VIRTIO device model, it might be a good time to revise the vhost-user > protocol. > > A vdpa-user protocol (or vhost-user 2.0) would replace the current mess > with a full VIRTIO device model. Both VMMs and device backends would > require changes to support this, but it would be a cleaner path forward > for the vhost-user protocol. > > One way of doing this would be a new VHOST_USER_PROTOCOL_F_VDPA feature > bit that indicates all the currently existing Linux vDPA ioctl messages > are available. Legacy vhost-user messages with overlapping functionality > must not be used when this bit is set. Most importantly, device backends > need to implement the full VIRTIO device model, regardless of device > type (net, blk, scsi, etc). > > The device type most affected by this change would be virtio-net. The > other device types are already closer to the full VIRTIO device model. > > I wanted to share this idea in case someone is currently trying to > figure out how to add more VIRTIO device model functionality to > vhost-user. > > Stefan > I understand the need and like the idea. Maxime signature.asc Description: OpenPGP digital signature
Re: [PATCH v1 09/10] vhost-vdpa: introduce vhost-vdpa backend
On 6/22/20 5:37 PM, Cindy Lu wrote: > Currently we have 2 types of vhost backends in QEMU: vhost kernel and > vhost-user. The above patch provides a generic device for vDPA purpose, > this vDPA device exposes to user space a non-vendor-specific configuration > interface for setting up a vhost HW accelerator, this patch set introduces > a third vhost backend called vhost-vdpa based on the vDPA interface. > > Vhost-vdpa usage: > > qemu-system-x86_64 -cpu host -enable-kvm \ > .. > -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-id,id=vhost-vdpa0 \ > -device virtio-net-pci,netdev=vhost-vdpa0,page-per-vq=on \ > > Signed-off-by: Lingshan zhu > Signed-off-by: Tiwei Bie > Signed-off-by: Cindy Lu > --- > configure | 21 ++ > hw/net/vhost_net.c| 19 +- > hw/net/virtio-net.c | 19 +- > hw/virtio/Makefile.objs | 1 + > hw/virtio/vhost-backend.c | 22 +- > hw/virtio/vhost-vdpa.c| 406 ++ > hw/virtio/vhost.c | 42 +++- > include/hw/virtio/vhost-backend.h | 6 +- > include/hw/virtio/vhost-vdpa.h| 26 ++ > include/hw/virtio/vhost.h | 6 + > qemu-options.hx | 12 + > 11 files changed, 555 insertions(+), 25 deletions(-) > create mode 100644 hw/virtio/vhost-vdpa.c > create mode 100644 include/hw/virtio/vhost-vdpa.h > > diff --git a/configure b/configure > index 23b5e93752..53679ee57f 100755 > --- a/configure > +++ b/configure > @@ -1557,6 +1557,10 @@ for opt do >;; >--enable-vhost-user) vhost_user="yes" >;; > + --disable-vhost-vdpa) vhost_vdpa="no" > + ;; > + --enable-vhost-vdpa) vhost_vdpa="yes" > + ;; >--disable-vhost-kernel) vhost_kernel="no" >;; >--enable-vhost-kernel) vhost_kernel="yes" > @@ -1846,6 +1850,7 @@ disabled with --disable-FEATURE, default is enabled if > available: >vhost-cryptovhost-user-crypto backend support >vhost-kernelvhost kernel backend support >vhost-user vhost-user backend support > + vhost-vdpa vhost-vdpa kernel backend support >spice spice >rbd rados block device (rbd) >libiscsiiscsi support > @@ -2336,6 +2341,10 @@ test "$vhost_user" = "" && vhost_user=yes > if test "$vhost_user" = "yes" && test "$mingw32" = "yes"; then >error_exit "vhost-user isn't available on win32" > fi > +test "$vhost_vdpa" = "" && vhost_vdpa=$linux > +if test "$vhost_vdpa" = "yes" && test "$linux" != "yes"; then > + error_exit "vhost-vdpa is only available on Linux" > +fi > test "$vhost_kernel" = "" && vhost_kernel=$linux > if test "$vhost_kernel" = "yes" && test "$linux" != "yes"; then >error_exit "vhost-kernel is only available on Linux" > @@ -2364,6 +2373,11 @@ test "$vhost_user_fs" = "" && vhost_user_fs=$vhost_user > if test "$vhost_user_fs" = "yes" && test "$vhost_user" = "no"; then >error_exit "--enable-vhost-user-fs requires --enable-vhost-user" > fi > +#vhost-vdpa backends > +test "$vhost_net_vdpa" = "" && vhost_net_vdpa=$vhost_vdpa > +if test "$vhost_net_vdpa" = "yes" && test "$vhost_vdpa" = "no"; then > + error_exit "--enable-vhost-net-vdpa requires --enable-vhost-vdpa" > +fi > > # OR the vhost-kernel and vhost-user values for simplicity > if test "$vhost_net" = ""; then > @@ -6673,6 +6687,7 @@ echo "vhost-scsi support $vhost_scsi" > echo "vhost-vsock support $vhost_vsock" > echo "vhost-user support $vhost_user" > echo "vhost-user-fs support $vhost_user_fs" > +echo "vhost-vdpa support $vhost_vdpa" > echo "Trace backends$trace_backends" > if have_backend "simple"; then > echo "Trace output file $trace_file-" > @@ -7170,6 +7185,9 @@ fi > if test "$vhost_net_user" = "yes" ; then >echo "CONFIG_VHOST_NET_USER=y" >> $config_host_mak > fi > +if test "$vhost_net_vdpa" = "yes" ; then > + echo "CONFIG_VHOST_NET_VDPA=y" >> $config_host_mak > +fi > if test "$vhost_crypto" = "yes" ; then >echo "CONFIG_VHOST_CRYPTO=y" >> $config_host_mak > fi > @@ -7182,6 +7200,9 @@ fi > if test "$vhost_user" = "yes" ; then >echo "CONFIG_VHOST_USER=y" >> $config_host_mak > fi > +if test "$vhost_vdpa" = "yes" ; then > + echo "CONFIG_VHOST_VDPA=y" >> $config_host_mak > +fi > if test "$vhost_user_fs" = "yes" ; then >echo "CONFIG_VHOST_USER_FS=y" >> $config_host_mak > fi > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 04cc3db264..cc259e571d 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -17,8 +17,10 @@ > #include "net/net.h" > #include "net/tap.h" > #include "net/vhost-user.h" > +#include "net/vhost-vdpa.h" > > #include "standard-headers/linux/vhost_types.h" > +#include "linux-headers/linux/vhost.h" > #include "hw/virtio/virtio-net.h" > #include "net/vhost_net.h" > #include "qemu/error-report.h" > @@ -33,12 +35,6 @@ > #include "hw/virtio/vhost.h" > #include "hw/virtio/virtio-bus.h" > > -struct vhost_net { > -struct vhost_dev dev; > -stru
Re: [PATCH] docs: vhost-user: add Virtio status protocol feature
On 6/18/20 2:50 PM, Jason Wang wrote: > > On 2020/6/18 下午7:29, Maxime Coquelin wrote: >> This patch specifies the VHOST_USER_SET_STATUS and >> VHOST_USER_GET_STATUS requests, which are sent by >> the master to update and query the Virtio status >> in the backend. >> >> Signed-off-by: Maxime Coquelin >> --- >> >> Changes since v1: >> = >> - Only keep the spec part in this patch, the implementation will >> be part of Cindy's Vhost vDPA series it depends on. The goal is >> to be able to implement it in next DPDK release even if Qemu part >> is not merged. >> - Add GET_STATUS after discussions with Michael and Jason. It can >> be used by the master to ensure FEATURES_OK bit set is >> acknowledged by the backend. >> >> docs/interop/vhost-user.rst | 24 >> 1 file changed, 24 insertions(+) >> >> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >> index 688b7c6900..866d7c2fb7 100644 >> --- a/docs/interop/vhost-user.rst >> +++ b/docs/interop/vhost-user.rst >> @@ -816,6 +816,7 @@ Protocol features >> #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >> #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >> #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 >> + define VHOST_USER_PROTOCOL_F_STATUS 16 > > > Miss a '#"? Indeed, I will fix that. Thanks, Maxime > Other looks good. > > Thanks > > >> Master message types >> >> @@ -1307,6 +1308,29 @@ Master message types >> ``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and >> update the memory tables of the slave device. >> +``VHOST_USER_SET_STATUS`` >> + :id: 39 >> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >> + :slave payload: N/A >> + :master payload: ``u64`` >> + >> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >> + successfully negotiated, this message is submitted by the master to >> + notify the backend with updated device status as defined in the Virtio >> + specification. >> + >> +``VHOST_USER_GET_STATUS`` >> + :id: 40 >> + :equivalent ioctl: VHOST_VDPA_GET_STATUS >> + :slave payload: ``u64`` >> + :master payload: N/A >> + >> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >> + successfully negotiated, this message is submitted by the master to >> + query the backend for its device status as defined in the Virtio >> + specification. >> + >> + >> Slave message types >> --- >>
[PATCH v3] docs: vhost-user: add Virtio status protocol feature
This patch specifies the VHOST_USER_SET_STATUS and VHOST_USER_GET_STATUS requests, which are sent by the master to update and query the Virtio status in the backend. Signed-off-by: Maxime Coquelin --- Changes since v2: = - Typo: fix missing # (Jason) Changes since v1: = - Only keep the spec part in this patch, the implementation will be part of Cindy's Vhost vDPA series it depends on. The goal is to be able to implement it in next DPDK release even if Qemu part is not merged. - Add GET_STATUS after discussions with Michael and Jason. It can be used by the master to ensure FEATURES_OK bit set is acknowledged by the backend. docs/interop/vhost-user.rst | 24 1 file changed, 24 insertions(+) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 688b7c6900..10e3e3475e 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -816,6 +816,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 + #define VHOST_USER_PROTOCOL_F_STATUS 16 Master message types @@ -1307,6 +1308,29 @@ Master message types ``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and update the memory tables of the slave device. +``VHOST_USER_SET_STATUS`` + :id: 39 + :equivalent ioctl: VHOST_VDPA_SET_STATUS + :slave payload: N/A + :master payload: ``u64`` + + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been + successfully negotiated, this message is submitted by the master to + notify the backend with updated device status as defined in the Virtio + specification. + +``VHOST_USER_GET_STATUS`` + :id: 40 + :equivalent ioctl: VHOST_VDPA_GET_STATUS + :slave payload: ``u64`` + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been + successfully negotiated, this message is submitted by the master to + query the backend for its device status as defined in the Virtio + specification. + + Slave message types --- -- 2.26.2
[PATCH] docs: vhost-user: add Virtio status protocol feature
This patch specifies the VHOST_USER_SET_STATUS and VHOST_USER_GET_STATUS requests, which are sent by the master to update and query the Virtio status in the backend. Signed-off-by: Maxime Coquelin --- Changes since v1: = - Only keep the spec part in this patch, the implementation will be part of Cindy's Vhost vDPA series it depends on. The goal is to be able to implement it in next DPDK release even if Qemu part is not merged. - Add GET_STATUS after discussions with Michael and Jason. It can be used by the master to ensure FEATURES_OK bit set is acknowledged by the backend. docs/interop/vhost-user.rst | 24 1 file changed, 24 insertions(+) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 688b7c6900..866d7c2fb7 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -816,6 +816,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 + define VHOST_USER_PROTOCOL_F_STATUS16 Master message types @@ -1307,6 +1308,29 @@ Master message types ``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and update the memory tables of the slave device. +``VHOST_USER_SET_STATUS`` + :id: 39 + :equivalent ioctl: VHOST_VDPA_SET_STATUS + :slave payload: N/A + :master payload: ``u64`` + + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been + successfully negotiated, this message is submitted by the master to + notify the backend with updated device status as defined in the Virtio + specification. + +``VHOST_USER_GET_STATUS`` + :id: 40 + :equivalent ioctl: VHOST_VDPA_GET_STATUS + :slave payload: ``u64`` + :master payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been + successfully negotiated, this message is submitted by the master to + query the backend for its device status as defined in the Virtio + specification. + + Slave message types --- -- 2.26.2
Re: [PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
On 5/14/20 4:48 PM, Michael S. Tsirkin wrote: > On Thu, May 14, 2020 at 04:39:26PM +0200, Maxime Coquelin wrote: >> >> >> On 5/14/20 12:51 PM, Michael S. Tsirkin wrote: >>> On Thu, May 14, 2020 at 12:14:32PM +0200, Maxime Coquelin wrote: >>>> >>>> >>>> On 5/14/20 9:53 AM, Jason Wang wrote: >>>>> >>>>> On 2020/5/14 下午3:33, Maxime Coquelin wrote: >>>>>> It is usefull for the Vhost-user backend to know >>>>>> about about the Virtio device status updates, >>>>>> especially when the driver sets the DRIVER_OK bit. >>>>>> >>>>>> With that information, no more need to do hazardous >>>>>> assumptions on when the driver is done with the >>>>>> device configuration. >>>>>> >>>>>> Signed-off-by: Maxime Coquelin >>>>>> --- >>>>>> >>>>>> This patch applies on top of Cindy's "vDPA support in qemu" >>>>>> series, which introduces the .vhost_set_state vhost-backend >>>>>> ops. >>>>>> >>>>>> docs/interop/vhost-user.rst | 12 >>>>>> hw/net/vhost_net.c | 10 +- >>>>>> hw/virtio/vhost-user.c | 35 +++ >>>>>> 3 files changed, 52 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >>>>>> index 3b1b6602c7..f108de7458 100644 >>>>>> --- a/docs/interop/vhost-user.rst >>>>>> +++ b/docs/interop/vhost-user.rst >>>>>> @@ -815,6 +815,7 @@ Protocol features >>>>>> #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 >>>>>> #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >>>>>> #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >>>>>> + #define VHOST_USER_PROTOCOL_F_STATUS 15 >>>>>> Master message types >>>>>> >>>>>> @@ -1263,6 +1264,17 @@ Master message types >>>>>> The state.num field is currently reserved and must be set to 0. >>>>>> +``VHOST_USER_SET_STATUS`` >>>>>> + :id: 36 >>>>>> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >>>>>> + :slave payload: N/A >>>>>> + :master payload: ``u64`` >>>>>> + >>>>>> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >>>>>> + successfully negotiated, this message is submitted by the master to >>>>>> + notify the backend with updated device status as defined in the Virtio >>>>>> + specification. >>>>>> + >>>>>> Slave message types >>>>>> --- >>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >>>>>> index 463e333531..37f3156dbc 100644 >>>>>> --- a/hw/net/vhost_net.c >>>>>> +++ b/hw/net/vhost_net.c >>>>>> @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) >>>>>> { >>>>>> struct vhost_net *net = get_vhost_net(nc); >>>>>> struct vhost_dev *hdev = &net->dev; >>>>>> - if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >>>>>> - if (hdev->vhost_ops->vhost_set_state) { >>>>>> - return hdev->vhost_ops->vhost_set_state(hdev, state); >>>>>> - } >>>>>> - } >>>>>> + >>>>>> + if (hdev->vhost_ops->vhost_set_state) { >>>>>> + return hdev->vhost_ops->vhost_set_state(hdev, state); >>>>>> + } >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>>>>> index ec21e8fbe8..b7e52d97fc 100644 >>>>>> --- a/hw/virtio/vhost-user.c >>>>>> +++ b/hw/virtio/vhost-user.c >>>>>> @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { >>>>>> VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, >>>>>> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, >>>>
Re: [PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
On 5/14/20 4:20 PM, Michael S. Tsirkin wrote: > On Thu, May 14, 2020 at 04:12:32PM +0200, Maxime Coquelin wrote: >> >> >> >> On 5/14/20 12:44 PM, Michael S. Tsirkin wrote: >>> On Thu, May 14, 2020 at 09:33:32AM +0200, Maxime Coquelin wrote: >>>> It is usefull for the Vhost-user backend to know >>>> about about the Virtio device status updates, >>>> especially when the driver sets the DRIVER_OK bit. >>>> >>>> With that information, no more need to do hazardous >>>> assumptions on when the driver is done with the >>>> device configuration. >>>> >>>> Signed-off-by: Maxime Coquelin >>>> --- >>>> >>>> This patch applies on top of Cindy's "vDPA support in qemu" >>>> series, which introduces the .vhost_set_state vhost-backend >>>> ops. >>>> >>>> docs/interop/vhost-user.rst | 12 >>>> hw/net/vhost_net.c | 10 +- >>>> hw/virtio/vhost-user.c | 35 +++ >>>> 3 files changed, 52 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >>>> index 3b1b6602c7..f108de7458 100644 >>>> --- a/docs/interop/vhost-user.rst >>>> +++ b/docs/interop/vhost-user.rst >>>> @@ -815,6 +815,7 @@ Protocol features >>>>#define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 >>>>#define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >>>>#define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >>>> + #define VHOST_USER_PROTOCOL_F_STATUS 15 >>>> >>>> Master message types >>>> >>>> @@ -1263,6 +1264,17 @@ Master message types >>>> >>>>The state.num field is currently reserved and must be set to 0. >>>> >>>> +``VHOST_USER_SET_STATUS`` >>>> + :id: 36 >>>> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >>>> + :slave payload: N/A >>>> + :master payload: ``u64`` >>>> + >>>> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >>>> + successfully negotiated, this message is submitted by the master to >>>> + notify the backend with updated device status as defined in the Virtio >>>> + specification. >>>> + >>>> Slave message types >>>> --- >>>> >>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >>>> index 463e333531..37f3156dbc 100644 >>>> --- a/hw/net/vhost_net.c >>>> +++ b/hw/net/vhost_net.c >>>> @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) >>>> { >>>> struct vhost_net *net = get_vhost_net(nc); >>>> struct vhost_dev *hdev = &net->dev; >>>> -if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >>>> -if (hdev->vhost_ops->vhost_set_state) { >>>> -return hdev->vhost_ops->vhost_set_state(hdev, state); >>>> - } >>>> -} >>>> + >>>> +if (hdev->vhost_ops->vhost_set_state) { >>>> +return hdev->vhost_ops->vhost_set_state(hdev, state); >>>> +} >>>> + >>>> return 0; >>>> } >>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>>> index ec21e8fbe8..b7e52d97fc 100644 >>>> --- a/hw/virtio/vhost-user.c >>>> +++ b/hw/virtio/vhost-user.c >>>> @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { >>>> VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, >>>> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, >>>> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, >>>> +VHOST_USER_PROTOCOL_F_STATUS = 15, >>>> VHOST_USER_PROTOCOL_F_MAX >>>> }; >>>> >>>> @@ -100,6 +101,7 @@ typedef enum VhostUserRequest { >>>> VHOST_USER_SET_INFLIGHT_FD = 32, >>>> VHOST_USER_GPU_SET_SOCKET = 33, >>>> VHOST_USER_RESET_DEVICE = 34, >>>> +VHOST_USER_SET_STATUS = 36, >>>> VHOST_USER_MAX >>>> } VhostUserRequest; >>>> >>>> @@ -1886,6 +1888,38 @@ static int vhost_user_set_inflight_fd(struct >>>> vhost_dev *dev, >>>> return 0; >>&g
Re: [PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
On 5/14/20 12:51 PM, Michael S. Tsirkin wrote: > On Thu, May 14, 2020 at 12:14:32PM +0200, Maxime Coquelin wrote: >> >> >> On 5/14/20 9:53 AM, Jason Wang wrote: >>> >>> On 2020/5/14 下午3:33, Maxime Coquelin wrote: >>>> It is usefull for the Vhost-user backend to know >>>> about about the Virtio device status updates, >>>> especially when the driver sets the DRIVER_OK bit. >>>> >>>> With that information, no more need to do hazardous >>>> assumptions on when the driver is done with the >>>> device configuration. >>>> >>>> Signed-off-by: Maxime Coquelin >>>> --- >>>> >>>> This patch applies on top of Cindy's "vDPA support in qemu" >>>> series, which introduces the .vhost_set_state vhost-backend >>>> ops. >>>> >>>> docs/interop/vhost-user.rst | 12 >>>> hw/net/vhost_net.c | 10 +- >>>> hw/virtio/vhost-user.c | 35 +++ >>>> 3 files changed, 52 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >>>> index 3b1b6602c7..f108de7458 100644 >>>> --- a/docs/interop/vhost-user.rst >>>> +++ b/docs/interop/vhost-user.rst >>>> @@ -815,6 +815,7 @@ Protocol features >>>> #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 >>>> #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >>>> #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >>>> + #define VHOST_USER_PROTOCOL_F_STATUS 15 >>>> Master message types >>>> >>>> @@ -1263,6 +1264,17 @@ Master message types >>>> The state.num field is currently reserved and must be set to 0. >>>> +``VHOST_USER_SET_STATUS`` >>>> + :id: 36 >>>> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >>>> + :slave payload: N/A >>>> + :master payload: ``u64`` >>>> + >>>> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >>>> + successfully negotiated, this message is submitted by the master to >>>> + notify the backend with updated device status as defined in the Virtio >>>> + specification. >>>> + >>>> Slave message types >>>> --- >>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >>>> index 463e333531..37f3156dbc 100644 >>>> --- a/hw/net/vhost_net.c >>>> +++ b/hw/net/vhost_net.c >>>> @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) >>>> { >>>> struct vhost_net *net = get_vhost_net(nc); >>>> struct vhost_dev *hdev = &net->dev; >>>> - if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >>>> - if (hdev->vhost_ops->vhost_set_state) { >>>> - return hdev->vhost_ops->vhost_set_state(hdev, state); >>>> - } >>>> - } >>>> + >>>> + if (hdev->vhost_ops->vhost_set_state) { >>>> + return hdev->vhost_ops->vhost_set_state(hdev, state); >>>> + } >>>> + >>>> return 0; >>>> } >>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>>> index ec21e8fbe8..b7e52d97fc 100644 >>>> --- a/hw/virtio/vhost-user.c >>>> +++ b/hw/virtio/vhost-user.c >>>> @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { >>>> VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, >>>> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, >>>> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, >>>> + VHOST_USER_PROTOCOL_F_STATUS = 15, >>>> VHOST_USER_PROTOCOL_F_MAX >>>> }; >>>> @@ -100,6 +101,7 @@ typedef enum VhostUserRequest { >>>> VHOST_USER_SET_INFLIGHT_FD = 32, >>>> VHOST_USER_GPU_SET_SOCKET = 33, >>>> VHOST_USER_RESET_DEVICE = 34, >>>> + VHOST_USER_SET_STATUS = 36, >>>> VHOST_USER_MAX >>>> } VhostUserRequest; >>>> @@ -1886,6 +1888,38 @@ static int vhost_user_set_inflight_fd(struct >>>> vhost_dev *dev, >>>> return 0; >>>> } >>>> +static int vhost_user_set_state(struct vhost_dev *dev, int state) &
Re: [PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
On 5/14/20 12:44 PM, Michael S. Tsirkin wrote: > On Thu, May 14, 2020 at 09:33:32AM +0200, Maxime Coquelin wrote: >> It is usefull for the Vhost-user backend to know >> about about the Virtio device status updates, >> especially when the driver sets the DRIVER_OK bit. >> >> With that information, no more need to do hazardous >> assumptions on when the driver is done with the >> device configuration. >> >> Signed-off-by: Maxime Coquelin >> --- >> >> This patch applies on top of Cindy's "vDPA support in qemu" >> series, which introduces the .vhost_set_state vhost-backend >> ops. >> >> docs/interop/vhost-user.rst | 12 >> hw/net/vhost_net.c | 10 +- >> hw/virtio/vhost-user.c | 35 +++ >> 3 files changed, 52 insertions(+), 5 deletions(-) >> >> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >> index 3b1b6602c7..f108de7458 100644 >> --- a/docs/interop/vhost-user.rst >> +++ b/docs/interop/vhost-user.rst >> @@ -815,6 +815,7 @@ Protocol features >>#define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 >>#define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >>#define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >> + #define VHOST_USER_PROTOCOL_F_STATUS 15 >> >> Master message types >> >> @@ -1263,6 +1264,17 @@ Master message types >> >>The state.num field is currently reserved and must be set to 0. >> >> +``VHOST_USER_SET_STATUS`` >> + :id: 36 >> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >> + :slave payload: N/A >> + :master payload: ``u64`` >> + >> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >> + successfully negotiated, this message is submitted by the master to >> + notify the backend with updated device status as defined in the Virtio >> + specification. >> + >> Slave message types >> --- >> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >> index 463e333531..37f3156dbc 100644 >> --- a/hw/net/vhost_net.c >> +++ b/hw/net/vhost_net.c >> @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) >> { >> struct vhost_net *net = get_vhost_net(nc); >> struct vhost_dev *hdev = &net->dev; >> -if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >> -if (hdev->vhost_ops->vhost_set_state) { >> -return hdev->vhost_ops->vhost_set_state(hdev, state); >> - } >> -} >> + >> +if (hdev->vhost_ops->vhost_set_state) { >> +return hdev->vhost_ops->vhost_set_state(hdev, state); >> +} >> + >> return 0; >> } >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >> index ec21e8fbe8..b7e52d97fc 100644 >> --- a/hw/virtio/vhost-user.c >> +++ b/hw/virtio/vhost-user.c >> @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { >> VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, >> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, >> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, >> +VHOST_USER_PROTOCOL_F_STATUS = 15, >> VHOST_USER_PROTOCOL_F_MAX >> }; >> >> @@ -100,6 +101,7 @@ typedef enum VhostUserRequest { >> VHOST_USER_SET_INFLIGHT_FD = 32, >> VHOST_USER_GPU_SET_SOCKET = 33, >> VHOST_USER_RESET_DEVICE = 34, >> +VHOST_USER_SET_STATUS = 36, >> VHOST_USER_MAX >> } VhostUserRequest; >> >> @@ -1886,6 +1888,38 @@ static int vhost_user_set_inflight_fd(struct >> vhost_dev *dev, >> return 0; >> } >> >> +static int vhost_user_set_state(struct vhost_dev *dev, int state) >> +{ >> +bool reply_supported = virtio_has_feature(dev->protocol_features, >> + >> VHOST_USER_PROTOCOL_F_REPLY_ACK); >> + >> +VhostUserMsg msg = { >> +.hdr.request = VHOST_USER_SET_STATUS, >> +.hdr.flags = VHOST_USER_VERSION, >> +.hdr.size = sizeof(msg.payload.u64), >> +.payload.u64 = (uint64_t)state, >> +}; >> + >> +if (!virtio_has_feature(dev->protocol_features, >> +VHOST_USER_PROTOCOL_F_STATUS)) { >> +return -1; >> +} >> + >> +if (reply_supported) { >> +msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; >> +} >> + >> +if (vhost_user_writ
Re: [PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
On 5/14/20 9:53 AM, Jason Wang wrote: > > On 2020/5/14 下午3:33, Maxime Coquelin wrote: >> It is usefull for the Vhost-user backend to know >> about about the Virtio device status updates, >> especially when the driver sets the DRIVER_OK bit. >> >> With that information, no more need to do hazardous >> assumptions on when the driver is done with the >> device configuration. >> >> Signed-off-by: Maxime Coquelin >> --- >> >> This patch applies on top of Cindy's "vDPA support in qemu" >> series, which introduces the .vhost_set_state vhost-backend >> ops. >> >> docs/interop/vhost-user.rst | 12 >> hw/net/vhost_net.c | 10 +- >> hw/virtio/vhost-user.c | 35 +++ >> 3 files changed, 52 insertions(+), 5 deletions(-) >> >> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >> index 3b1b6602c7..f108de7458 100644 >> --- a/docs/interop/vhost-user.rst >> +++ b/docs/interop/vhost-user.rst >> @@ -815,6 +815,7 @@ Protocol features >> #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 >> #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 >> #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 >> + #define VHOST_USER_PROTOCOL_F_STATUS 15 >> Master message types >> >> @@ -1263,6 +1264,17 @@ Master message types >> The state.num field is currently reserved and must be set to 0. >> +``VHOST_USER_SET_STATUS`` >> + :id: 36 >> + :equivalent ioctl: VHOST_VDPA_SET_STATUS >> + :slave payload: N/A >> + :master payload: ``u64`` >> + >> + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been >> + successfully negotiated, this message is submitted by the master to >> + notify the backend with updated device status as defined in the Virtio >> + specification. >> + >> Slave message types >> --- >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >> index 463e333531..37f3156dbc 100644 >> --- a/hw/net/vhost_net.c >> +++ b/hw/net/vhost_net.c >> @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) >> { >> struct vhost_net *net = get_vhost_net(nc); >> struct vhost_dev *hdev = &net->dev; >> - if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >> - if (hdev->vhost_ops->vhost_set_state) { >> - return hdev->vhost_ops->vhost_set_state(hdev, state); >> - } >> - } >> + >> + if (hdev->vhost_ops->vhost_set_state) { >> + return hdev->vhost_ops->vhost_set_state(hdev, state); >> + } >> + >> return 0; >> } >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >> index ec21e8fbe8..b7e52d97fc 100644 >> --- a/hw/virtio/vhost-user.c >> +++ b/hw/virtio/vhost-user.c >> @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { >> VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, >> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, >> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, >> + VHOST_USER_PROTOCOL_F_STATUS = 15, >> VHOST_USER_PROTOCOL_F_MAX >> }; >> @@ -100,6 +101,7 @@ typedef enum VhostUserRequest { >> VHOST_USER_SET_INFLIGHT_FD = 32, >> VHOST_USER_GPU_SET_SOCKET = 33, >> VHOST_USER_RESET_DEVICE = 34, >> + VHOST_USER_SET_STATUS = 36, >> VHOST_USER_MAX >> } VhostUserRequest; >> @@ -1886,6 +1888,38 @@ static int vhost_user_set_inflight_fd(struct >> vhost_dev *dev, >> return 0; >> } >> +static int vhost_user_set_state(struct vhost_dev *dev, int state) >> +{ >> + bool reply_supported = virtio_has_feature(dev->protocol_features, >> + >> VHOST_USER_PROTOCOL_F_REPLY_ACK); >> + >> + VhostUserMsg msg = { >> + .hdr.request = VHOST_USER_SET_STATUS, >> + .hdr.flags = VHOST_USER_VERSION, >> + .hdr.size = sizeof(msg.payload.u64), >> + .payload.u64 = (uint64_t)state, >> + }; >> + >> + if (!virtio_has_feature(dev->protocol_features, >> + VHOST_USER_PROTOCOL_F_STATUS)) { >> + return -1; >> + } >> + >> + if (reply_supported) { >> + msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; >> + } >> + >> + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { >>
[PATCH] vhost-user: add support for VHOST_USER_SET_STATUS
It is usefull for the Vhost-user backend to know about about the Virtio device status updates, especially when the driver sets the DRIVER_OK bit. With that information, no more need to do hazardous assumptions on when the driver is done with the device configuration. Signed-off-by: Maxime Coquelin --- This patch applies on top of Cindy's "vDPA support in qemu" series, which introduces the .vhost_set_state vhost-backend ops. docs/interop/vhost-user.rst | 12 hw/net/vhost_net.c | 10 +- hw/virtio/vhost-user.c | 35 +++ 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3b1b6602c7..f108de7458 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -815,6 +815,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12 #define VHOST_USER_PROTOCOL_F_RESET_DEVICE 13 #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 + #define VHOST_USER_PROTOCOL_F_STATUS 15 Master message types @@ -1263,6 +1264,17 @@ Master message types The state.num field is currently reserved and must be set to 0. +``VHOST_USER_SET_STATUS`` + :id: 36 + :equivalent ioctl: VHOST_VDPA_SET_STATUS + :slave payload: N/A + :master payload: ``u64`` + + When the ``VHOST_USER_PROTOCOL_F_STATUS`` protocol feature has been + successfully negotiated, this message is submitted by the master to + notify the backend with updated device status as defined in the Virtio + specification. + Slave message types --- diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 463e333531..37f3156dbc 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -517,10 +517,10 @@ int vhost_set_state(NetClientState *nc, int state) { struct vhost_net *net = get_vhost_net(nc); struct vhost_dev *hdev = &net->dev; -if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { -if (hdev->vhost_ops->vhost_set_state) { -return hdev->vhost_ops->vhost_set_state(hdev, state); - } -} + +if (hdev->vhost_ops->vhost_set_state) { +return hdev->vhost_ops->vhost_set_state(hdev, state); +} + return 0; } diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index ec21e8fbe8..b7e52d97fc 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -59,6 +59,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12, VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13, +VHOST_USER_PROTOCOL_F_STATUS = 15, VHOST_USER_PROTOCOL_F_MAX }; @@ -100,6 +101,7 @@ typedef enum VhostUserRequest { VHOST_USER_SET_INFLIGHT_FD = 32, VHOST_USER_GPU_SET_SOCKET = 33, VHOST_USER_RESET_DEVICE = 34, +VHOST_USER_SET_STATUS = 36, VHOST_USER_MAX } VhostUserRequest; @@ -1886,6 +1888,38 @@ static int vhost_user_set_inflight_fd(struct vhost_dev *dev, return 0; } +static int vhost_user_set_state(struct vhost_dev *dev, int state) +{ +bool reply_supported = virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_REPLY_ACK); + +VhostUserMsg msg = { +.hdr.request = VHOST_USER_SET_STATUS, +.hdr.flags = VHOST_USER_VERSION, +.hdr.size = sizeof(msg.payload.u64), +.payload.u64 = (uint64_t)state, +}; + +if (!virtio_has_feature(dev->protocol_features, +VHOST_USER_PROTOCOL_F_STATUS)) { +return -1; +} + +if (reply_supported) { +msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; +} + +if (vhost_user_write(dev, &msg, NULL, 0) < 0) { +return -1; +} + +if (reply_supported) { +return process_message_reply(dev, &msg); +} + +return 0; +} + bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp) { if (user->chr) { @@ -1947,4 +1981,5 @@ const VhostOps user_ops = { .vhost_backend_mem_section_filter = vhost_user_mem_section_filter, .vhost_get_inflight_fd = vhost_user_get_inflight_fd, .vhost_set_inflight_fd = vhost_user_set_inflight_fd, +.vhost_set_state = vhost_user_set_state, }; -- 2.25.4
Re: [RFC v1 3/4] vhost-vdpa: implement vhost-vdpa backend
On 4/20/20 11:32 AM, Cindy Lu wrote: > diff --git a/include/hw/virtio/vhost-backend.h > b/include/hw/virtio/vhost-backend.h > index 6f6670783f..d81bd9885f 100644 > --- a/include/hw/virtio/vhost-backend.h > +++ b/include/hw/virtio/vhost-backend.h > @@ -17,7 +17,8 @@ typedef enum VhostBackendType { > VHOST_BACKEND_TYPE_NONE = 0, > VHOST_BACKEND_TYPE_KERNEL = 1, > VHOST_BACKEND_TYPE_USER = 2, > -VHOST_BACKEND_TYPE_MAX = 3, > +VHOST_BACKEND_TYPE_VDPA = 3, > +VHOST_BACKEND_TYPE_MAX = 4, > } VhostBackendType; > > typedef enum VhostSetConfigType { > @@ -112,6 +113,7 @@ typedef int (*vhost_get_inflight_fd_op)(struct vhost_dev > *dev, > typedef int (*vhost_set_inflight_fd_op)(struct vhost_dev *dev, > struct vhost_inflight *inflight); > > +typedef int (*vhost_set_state_op)(struct vhost_dev *dev, int state); I think state should be of type uint8_t.
Re: [RFC v1 3/4] vhost-vdpa: implement vhost-vdpa backend
On 4/20/20 11:32 AM, Cindy Lu wrote: > Currently we have 2 types of vhost backends in QEMU: vhost kernel and > vhost-user. The above patch provides a generic device for vDPA purpose, > this vDPA device exposes to user space a non-vendor-specific configuration > interface for setting up a vhost HW accelerator, this patch set introduces > a third vhost backend called vhost-vdpa based on the vDPA interface. > > Vhost-vdpa usage: > > qemu-system-x86_64 -cpu host -enable-kvm \ > .. > -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-id,id=vhost-vdpa0 \ > -device virtio-net-pci,netdev=vhost-vdpa0,page-per-vq=on \ > > Author: Tiwei Bie > Signed-off-by: Cindy Lu > --- > hw/net/vhost_net.c| 43 > hw/net/virtio-net.c | 9 + > hw/virtio/Makefile.objs | 2 +- > hw/virtio/vhost-backend.c | 3 + > hw/virtio/vhost-vdpa.c| 379 ++ > hw/virtio/vhost.c | 5 + > include/hw/virtio/vhost-backend.h | 6 +- > include/hw/virtio/vhost-vdpa.h| 14 ++ > 8 files changed, 459 insertions(+), 2 deletions(-) > create mode 100644 hw/virtio/vhost-vdpa.c > create mode 100644 include/hw/virtio/vhost-vdpa.h > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 4096d64aaf..0d13fda2fc 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -17,8 +17,10 @@ > #include "net/net.h" > #include "net/tap.h" > #include "net/vhost-user.h" > +#include "net/vhost-vdpa.h" > > #include "standard-headers/linux/vhost_types.h" > +#include "linux-headers/linux/vhost.h" > #include "hw/virtio/virtio-net.h" > #include "net/vhost_net.h" > #include "qemu/error-report.h" > @@ -85,6 +87,29 @@ static const int user_feature_bits[] = { > VHOST_INVALID_FEATURE_BIT > }; > > +static const int vdpa_feature_bits[] = { > +VIRTIO_F_NOTIFY_ON_EMPTY, > +VIRTIO_RING_F_INDIRECT_DESC, > +VIRTIO_RING_F_EVENT_IDX, > +VIRTIO_F_ANY_LAYOUT, > +VIRTIO_F_VERSION_1, > +VIRTIO_NET_F_CSUM, > +VIRTIO_NET_F_GUEST_CSUM, > +VIRTIO_NET_F_GSO, > +VIRTIO_NET_F_GUEST_TSO4, > +VIRTIO_NET_F_GUEST_TSO6, > +VIRTIO_NET_F_GUEST_ECN, > +VIRTIO_NET_F_GUEST_UFO, > +VIRTIO_NET_F_HOST_TSO4, > +VIRTIO_NET_F_HOST_TSO6, > +VIRTIO_NET_F_HOST_ECN, > +VIRTIO_NET_F_HOST_UFO, > +VIRTIO_NET_F_MRG_RXBUF, > +VIRTIO_NET_F_MTU, > +VIRTIO_F_IOMMU_PLATFORM, > +VIRTIO_NET_F_GUEST_ANNOUNCE, > +VHOST_INVALID_FEATURE_BIT > +}; > static const int *vhost_net_get_feature_bits(struct vhost_net *net) > { > const int *feature_bits = 0; > @@ -96,6 +121,9 @@ static const int *vhost_net_get_feature_bits(struct > vhost_net *net) > case NET_CLIENT_DRIVER_VHOST_USER: > feature_bits = user_feature_bits; > break; > +case NET_CLIENT_DRIVER_VHOST_VDPA: > +feature_bits = vdpa_feature_bits; > +break; > default: > error_report("Feature bits not defined for this type: %d", > net->nc->info->type); > @@ -434,6 +462,10 @@ VHostNetState *get_vhost_net(NetClientState *nc) > assert(vhost_net); > break; > #endif > +case NET_CLIENT_DRIVER_VHOST_VDPA: > +vhost_net = vhost_vdpa_get_vhost_net(nc); > +assert(vhost_net); > +break; > default: > break; > } > @@ -465,3 +497,14 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t > mtu) > > return vhost_ops->vhost_net_set_mtu(&net->dev, mtu); > } > +int vhost_set_state(NetClientState *nc, int state) > +{ > +struct vhost_net *net = get_vhost_net(nc); > +struct vhost_dev *hdev = &net->dev; > +if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { Maybe checking the vhost_set_state callback is implemented is enough, and it is not need to restrict that to Vhost-vDPA? > +if (hdev->vhost_ops->vhost_set_state) { > +return hdev->vhost_ops->vhost_set_state(hdev, state); > + } > +} > +return 0; > +}
Re: [Qemu-devel] [PATCH v1 09/16] virtio: fill/flush/pop for packed ring
Hi Wei, On 11/22/18 3:06 PM, w...@redhat.com wrote: +void virtqueue_flush(VirtQueue *vq, unsigned int count) +{ +if (unlikely(vq->vdev->broken)) { +vq->inuse -= count; +return; +} + +if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { +virtqueue_packed_flush(vq, count); +} else { +virtqueue_split_flush(vq, count); +} +} + void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len) { @@ -1074,7 +1180,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu return elem; } -void *virtqueue_pop(VirtQueue *vq, size_t sz) +static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) { unsigned int i, head, max; VRingMemoryRegionCaches *caches; @@ -1089,9 +1195,6 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) VRingDesc desc; int rc; -if (unlikely(vdev->broken)) { -return NULL; -} rcu_read_lock(); if (virtio_queue_empty_rcu(vq)) { goto done; @@ -1209,6 +1312,159 @@ err_undo_map: goto done; } +static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz) +{ +unsigned int i, head, max; +VRingMemoryRegionCaches *caches; +MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; +MemoryRegionCache *cache; +int64_t len; +VirtIODevice *vdev = vq->vdev; +VirtQueueElement *elem = NULL; +unsigned out_num, in_num, elem_entries; +hwaddr addr[VIRTQUEUE_MAX_SIZE]; +struct iovec iov[VIRTQUEUE_MAX_SIZE]; +VRingPackedDesc desc; +uint16_t id; + +rcu_read_lock(); +if (virtio_queue_packed_empty_rcu(vq)) { +goto done; +} + +/* When we start there are none of either input nor output. */ +out_num = in_num = elem_entries = 0; + +max = vq->vring.num; + +if (vq->inuse >= vq->vring.num) { +virtio_error(vdev, "Virtqueue size exceeded"); +goto done; +} + +head = vq->last_avail_idx; +i = head; + +caches = vring_get_region_caches(vq); +cache = &caches->desc; + +/* Empty check has been done at the beginning, so it is an available + * entry already, make sure all fields has been exposed by guest */ +smp_rmb(); +vring_packed_desc_read(vdev, &desc, cache, i); + +id = desc.id; +if (desc.flags & VRING_DESC_F_INDIRECT) { + +if (desc.len % sizeof(VRingPackedDesc)) { +virtio_error(vdev, "Invalid size for indirect buffer table"); +goto done; +} + +/* loop over the indirect descriptor table */ +len = address_space_cache_init(&indirect_desc_cache, vdev->dma_as, + desc.addr, desc.len, false); +cache = &indirect_desc_cache; +if (len < desc.len) { +virtio_error(vdev, "Cannot map indirect buffer"); +goto done; +} + +max = desc.len / sizeof(VRingPackedDesc); +i = 0; +vring_packed_desc_read(vdev, &desc, cache, i); +/* Make sure we see all the fields*/ +smp_rmb(); +} + +/* Collect all the descriptors */ +while (1) { +bool map_ok; + +if (desc.flags & VRING_DESC_F_WRITE) { +map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num, +iov + out_num, +VIRTQUEUE_MAX_SIZE - out_num, true, +desc.addr, desc.len); +} else { +if (in_num) { +virtio_error(vdev, "Incorrect order for descriptors"); +goto err_undo_map; +} +map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov, +VIRTQUEUE_MAX_SIZE, false, +desc.addr, desc.len); +} +if (!map_ok) { +goto err_undo_map; +} + +/* If we've got too many, that implies a descriptor loop. */ +if (++elem_entries > max) { +virtio_error(vdev, "Looped descriptor"); +goto err_undo_map; +} + +if (++i >= vq->vring.num) { +i -= vq->vring.num; +} + +if (desc.flags & VRING_DESC_F_NEXT) { +vring_packed_desc_read(vq->vdev, &desc, cache, i); +} else { +break; +} +} + +/* Now copy what we have collected and mapped */ +elem = virtqueue_alloc_element(sz, out_num, in_num); +elem->index = id; +for (i = 0; i < out_num; i++) { +elem->out_addr[i] = addr[i]; +elem->out_sg[i] = iov[i]; +} +for (i = 0; i < in_num; i++) { +elem->in_addr[i] = addr[head + out_num + i]; +elem->in_sg[i] = iov[out_num + i]; +} + +vq->last_avail_idx += (cache == &indirect_desc_cache) ? + 1 : out_num + in_num; I think you cannot rely on out_num and in_num to deduce
Re: [Qemu-devel] [RFC v2 6/8] virtio: flush/push for packed ring
Hi Wei, On 9/20/18 4:13 PM, Maxime Coquelin wrote: } @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) vq->signalled_used_valid = false; } +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) +{ + if (unlikely(!vq->vring.desc)) { + return; + } + + vq->inuse -= count; + vq->used_idx += count; I think it is wrong, because count seems to be representing the number of descriptor chains. But in case of packed ring layout, used index must be incremented by the number of descriptors. For example, I'm having trouble with the ctrl queue, where the commands sent by the guest use 3 descriptors, but count is 1. It seems you didn't fixed this issue in your last revision. I guess it works for virtio-net Kernel driver (with the fix you proposed) in guest because it is using indirect descs. But Virtio PMD does not use indirect descs for the control vq. Thanks, Maxime
Re: [Qemu-devel] [PATCH v1 13/16] virtio: add vhost-net migration of packed ring
On 11/22/18 3:06 PM, w...@redhat.com wrote: From: Wei Xu tweaked vhost-net code to test migration. @@ -1414,64 +1430,20 @@ long vhost_vring_ioctl(struct vhost_dev r = -EFAULT; break; } + vq->last_avail_idx = s.num & 0x7FFF; + /* Forget the cached index value. */ + vq->avail_idx = vq->last_avail_idx; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + vq->last_avail_wrap_counter = !!(s.num & 0x8000); + vq->avail_wrap_counter = vq->last_avail_wrap_counter; + + vq->last_used_idx = (s.num & 0x7fFF) >> 16; + vq->last_used_wrap_counter = !!(s.num & 0x8000); + } + break; + case VHOST_GET_VRING_BASE: + s.index = idx; +s.num = vq->last_avail_idx; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + s.num |= vq->last_avail_wrap_counter << 15; + s.num |= vq->last_used_idx << 16; + s.num |= vq->last_used_wrap_counter << 31; + } + if (copy_to_user(argp, &s, sizeof(s))) + r = -EFAULT; + break; Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 35 ++- include/hw/virtio/virtio.h | 4 ++-- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 64d5c04..7487d3d 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2963,19 +2963,40 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) } } -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) +int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { -return vdev->vq[n].last_avail_idx; +int idx; + +if (virtio_host_has_feature(vdev, VIRTIO_F_RING_PACKED)) { Also, I think you want to use virtio_vdev_has_feature() here instead, else it will set wrap counter in the case ring_packed=on in the QEMU command line but the feature has not been negotiated. For example, with ring_packed=on and with stock Fedora 28 kernel, which does not support packed ring, I get this warning with DPDK vhost user backend: VHOST_CONFIG: last_used_idx (32768) and vq->used->idx (0) mismatches; some packets maybe resent for Tx and dropped for Rx +idx = vdev->vq[n].last_avail_idx; +idx |= ((int)vdev->vq[n].avail_wrap_counter) << 15; +idx |= (vdev->vq[n].used_idx) << 16; +idx |= ((int)vdev->vq[n].used_wrap_counter) << 31; +} else { +idx = (int)vdev->vq[n].last_avail_idx; +} +return idx; } -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, int idx) { -vdev->vq[n].last_avail_idx = idx; -vdev->vq[n].shadow_avail_idx = idx; +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +vdev->vq[n].last_avail_idx = idx & 0x7fff; +vdev->vq[n].avail_wrap_counter = !!(idx & 0x8000); +vdev->vq[n].used_idx = (idx & 0x7fff) >> 16; +vdev->vq[n].used_wrap_counter = !!(idx & 0x8000); +} else { +vdev->vq[n].last_avail_idx = idx; +vdev->vq[n].shadow_avail_idx = idx; +} } void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) { +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return; +} + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); @@ -2986,6 +3007,10 @@ void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) { +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return; +} + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]); diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 9c1fa07..a6fdf3f 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -272,8 +272,8 @@ hwaddr virtio_queue_get_used_addr(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n); -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx); +int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, int idx); void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n); void virtio_queue_invalidate_signalled_used(VirtIODevice *vdev, int n); void virtio_queue_update_used_idx(VirtIODev
Re: [Qemu-devel] [PATCH v1 13/16] virtio: add vhost-net migration of packed ring
On 11/22/18 3:06 PM, w...@redhat.com wrote: From: Wei Xu tweaked vhost-net code to test migration. @@ -1414,64 +1430,20 @@ long vhost_vring_ioctl(struct vhost_dev r = -EFAULT; break; } + vq->last_avail_idx = s.num & 0x7FFF; + /* Forget the cached index value. */ + vq->avail_idx = vq->last_avail_idx; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + vq->last_avail_wrap_counter = !!(s.num & 0x8000); + vq->avail_wrap_counter = vq->last_avail_wrap_counter; + + vq->last_used_idx = (s.num & 0x7fFF) >> 16; + vq->last_used_wrap_counter = !!(s.num & 0x8000); + } + break; + case VHOST_GET_VRING_BASE: + s.index = idx; +s.num = vq->last_avail_idx; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + s.num |= vq->last_avail_wrap_counter << 15; + s.num |= vq->last_used_idx << 16; + s.num |= vq->last_used_wrap_counter << 31; We agreed to only have avail idx and its wrap counter for vhost-user, I guess we should have the same for Kernel backend? This is what we have for vhost-user backend: bit[0:14] last_avail_idx bit[15] last_avail_wrap_counter + } + if (copy_to_user(argp, &s, sizeof(s))) + r = -EFAULT; + break; Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 35 ++- include/hw/virtio/virtio.h | 4 ++-- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 64d5c04..7487d3d 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2963,19 +2963,40 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) } } -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) +int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { -return vdev->vq[n].last_avail_idx; +int idx; + +if (virtio_host_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +idx = vdev->vq[n].last_avail_idx; +idx |= ((int)vdev->vq[n].avail_wrap_counter) << 15; +idx |= (vdev->vq[n].used_idx) << 16; +idx |= ((int)vdev->vq[n].used_wrap_counter) << 31; +} else { +idx = (int)vdev->vq[n].last_avail_idx; +} +return idx; } -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, int idx) { -vdev->vq[n].last_avail_idx = idx; -vdev->vq[n].shadow_avail_idx = idx; +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +vdev->vq[n].last_avail_idx = idx & 0x7fff; +vdev->vq[n].avail_wrap_counter = !!(idx & 0x8000); +vdev->vq[n].used_idx = (idx & 0x7fff) >> 16; +vdev->vq[n].used_wrap_counter = !!(idx & 0x8000); +} else { +vdev->vq[n].last_avail_idx = idx; +vdev->vq[n].shadow_avail_idx = idx; +} } void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) { +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return; +} + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); @@ -2986,6 +3007,10 @@ void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) { +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return; +} + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]); diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 9c1fa07..a6fdf3f 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -272,8 +272,8 @@ hwaddr virtio_queue_get_used_addr(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n); -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx); +int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, int idx); void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n); void virtio_queue_invalidate_signalled_used(VirtIODevice *vdev, int n); void virtio_queue_update_used_idx(VirtIODevice *vdev, int n);
Re: [Qemu-devel] [PATCH v1 00/16] packed ring virtio-net backend support
Hi Wei, I just tested your series with Tiwei's v3, and it fails with ctrl vq enabled: qemu-system-x86_64: virtio-net ctrl missing headers Regards, Maxime On 11/22/18 3:06 PM, w...@redhat.com wrote: From: Wei Xu Code base: https://github.com/Whishay/qemu.git rfc v3 -> v1 - migration support for both userspace and vhost-net, need tweak vhost ioctl() to make it work(the code is pasted in the commit message of vhost migration patch #13). Note: the high 32-bit guest feature bit is saved as a subsection for virtio devices which makes packed ring feature bit check unusable when loading the saved per-queue variables(this is done before loading subsection which is the last action for device during migration), so I save and load all the things generally for now, any idea to fix this? - Fixed comments from Jason for rfc v3 sorted by patch #, two comments I didn't take were(from patch) listed here: 09: - introduce new API(virtqueue_fill_n()). - Didn't take it since userspace backend does not support batching, so only one element is popped and current API should be enough. 06 & 07: Refactor split and packed pop()/get_avail_bytes(). - the duplicated code interwined with split/packed ring specific things and it might make it unclear, so I only extracted the few common parts out side rcu and keep the others separate. The other revised comments: 02: - reuse current 'avail/used' for 'driver/device' in VRingMemoryRegionCache. - remove event_idx since shadow_avail_idx works. 03: - move size recalculation to a separate patch. - keep 'avail/used' in current calculation function name. - initialize 'desc' memory region as 'false' for 1.0('true' for 1.1) 04: - delete 'event_idx' 05: - rename 'wc' to wrap_counter. 06: - converge common part outside rcu section for 1.0/1.1. - move memory barrier for the first 'desc' in between checking flag and read other fields. - remove unnecessary memory barriers for indirect descriptors. - no need to destroy indirect memory cache since it is generally done before return from the function. - remove redundant maximum chained descriptors limitation check. - there are some differences(desc name, wrap idx/counter, flags) between split and packed rings, so keep them separate for now. - amend the comment when recording index and wrap counter for a kick from guest. 07: - calculate fields in descriptor instead of read it when filling. - put memory barrier correctly before filling the flags in descriptor. - replace full memory barrier with a write barrier in fill. - shift to read descriptor flags and descriptor necessarily and separately in packed_pop(). - correct memory barrier in packed_pop() as in packed_fill(). 08: - reuse 'shadow_avail_idx' instead of adding a new 'event_idx'. - use the compact and verified vring_packed_need_event() version for vhost net/user. 12: - remove the odd cherry-pick comment. - used bit '15' for wrap_counters. rfc v2->v3 - addressed performance issue - fixed feedback from v2 rfc v1->v2 - sync to tiwei's v5 - reuse memory cache function with 1.0 - dropped detach patch and notification helper(04 & 05 in v1) - guest virtio-net driver unload/reload support - event suppression support(not tested) - addressed feedback from v1 Wei Xu (15): virtio: introduce packed ring definitions virtio: redefine structure & memory cache for packed ring virtio: expand offset calculation for packed ring virtio: add memory region init for packed ring virtio: init wrap counter for packed ring virtio: init and desc empty check for packed ring virtio: get avail bytes check for packed ring virtio: fill/flush/pop for packed ring virtio: event suppression support for packed ring virtio-net: fill head desc after done all in a chain virtio: add userspace migration of packed ring virtio: add vhost-net migration of packed ring virtio: packed ring feature bit for userspace backend vhost: enable packed ring virtio: enable packed ring via a new command line VERSION| 2 +- hw/net/vhost_net.c | 2 + hw/net/virtio-net.c| 11 +- hw/virtio/virtio.c | 756 +++-- include/hw/virtio/virtio.h | 8 +- include/standard-headers/linux/virtio_config.h | 15 + include/standard-headers/linux/virtio_ring.h | 43 ++ 7 files changed, 783 insertions(+), 54 deletions(-)
Re: [Qemu-devel] [[RFC v3 12/12] virtio: feature vhost-net support for packed ring
Hi Wei, On 10/11/18 4:08 PM, w...@redhat.com wrote: From: Wei Xu (cherry picked from commit 305a2c4640c15c5717245067ab937fd10f478ee6) Signed-off-by: Wei Xu (cherry picked from commit 46476dae6f44c6fef8802a4a0ac7d0d79fe399e3) Signed-off-by: Wei Xu --- hw/virtio/vhost.c | 3 +++ hw/virtio/virtio.c | 4 include/hw/virtio/virtio.h | 1 + 3 files changed, 8 insertions(+) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 9df2da3..de06d55 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -974,6 +974,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev, } state.num = virtio_queue_get_last_avail_idx(vdev, idx); +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +state.num |= ((int)virtio_queue_packed_get_wc(vdev, idx)) << 31; For next version, please note that we agreed to move the wrap counter value at bit 15. DPDK vhost lib implemented it that way.
Re: [Qemu-devel] [RFC 0/2] vhost+postcopy fixes
On 10/08/2018 06:05 PM, Ilya Maximets wrote: Sending as RFC because it's not fully tested yet. Ilya Maximets (2): migration: Stop postcopy fault thread before notifying vhost-user: Fix userfaultfd leak hw/virtio/vhost-user.c | 7 +++ migration/postcopy-ram.c | 11 ++- 2 files changed, 13 insertions(+), 5 deletions(-) For the series: Reviewed-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [PATCH v3 0/2] intel_iommu: handle invalid ce for shadow sync
On 10/09/2018 09:45 AM, Peter Xu wrote: v3: - pick r-b - return when -VTD_FR_CONTEXT_ENTRY_P is detected (v1 is correct here, but I did wrong thing when splitting the patch in v2) [Eric] v2: - split patch into more, remove useless comment [Eric] - remove one error_report_once() when rework the code [Jason] This series fixes a QEMU crash reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1627272 Please review, thanks. Peter Xu (2): intel_iommu: move ce fetching out when sync shadow intel_iommu: handle invalid ce for shadow sync dtc | 2 +- hw/i386/intel_iommu.c | 55 +++ 2 files changed, 30 insertions(+), 27 deletions(-) For the series: Reviewed-by: Maxime Coquelin
Re: [Qemu-devel] [PATCH] vhost-user: Don't ask for reply on postcopy mem table set
On 10/02/2018 04:09 PM, Ilya Maximets wrote: According to documentation, NEED_REPLY_MASK should not be set for VHOST_USER_SET_MEM_TABLE request in postcopy mode. This restriction was mistakenly applied to 'reply_supported' variable, which is local and used only for non-postcopy case. CC: Dr. David Alan Gilbert Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu") Signed-off-by: Ilya Maximets --- hw/virtio/vhost-user.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) Reviewed-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [RFC v2 8/8] virtio: guest driver reload for vhost-net
On 09/21/2018 04:33 AM, Jason Wang wrote: On 2018年09月21日 04:39, Maxime Coquelin wrote: Hi Wei, Jason, On 06/19/2018 09:53 AM, Wei Xu wrote: On Wed, Jun 06, 2018 at 11:48:19AM +0800, Jason Wang wrote: On 2018å¹´06月06æ—¥ 03:08, w...@redhat.com wrote: From: Wei Xu last_avail, avail_wrap_count, used_idx and used_wrap_count are needed to support vhost-net backend, all these are either 16 or bool variables, since state.num is 64bit wide, so here it is possible to put them to the 'num' without introducing a new case while handling ioctl. Unload/Reload test has been done successfully with a patch in vhost kernel. You need a patch to enable vhost. And I think you can only do it for vhost-kenrel now since vhost-user protocol needs some extension I believe. OK. Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 42 ++ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 4543974..153f6d7 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2862,33 +2862,59 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) } } -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) +uint64_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { - return vdev->vq[n].last_avail_idx; + uint64_t num; + + num = vdev->vq[n].last_avail_idx; + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 16; + num |= ((uint64_t)vdev->vq[n].used_idx) << 32; + num |= ((uint64_t)vdev->vq[n].used_wrap_counter) << 48; So s.num is 32bit, I don't think this can even work. I mistakenly checked out s.num is 64bit, will add a new case in next version. Wouldn't be enough to just get/set avail_wrap_counter? Something like this so that it fits into 32 bits: if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 31; } Regards, Maxime Yes, actually, that's what I did for vhost kernel (https://github.com/jasowang/net/blob/packed-host/drivers/vhost/vhost.c#L1418) Oh yes, I forgot on what I based my implementation on. Wei, Michael, do you agree with this? I will need to merge the Vhost patch in DPDK in the coming two weeks. Regards, Maxime Thanks
Re: [Qemu-devel] [RFC v2 8/8] virtio: guest driver reload for vhost-net
Hi Wei, Jason, On 06/19/2018 09:53 AM, Wei Xu wrote: On Wed, Jun 06, 2018 at 11:48:19AM +0800, Jason Wang wrote: On 2018年06月06日 03:08, w...@redhat.com wrote: From: Wei Xu last_avail, avail_wrap_count, used_idx and used_wrap_count are needed to support vhost-net backend, all these are either 16 or bool variables, since state.num is 64bit wide, so here it is possible to put them to the 'num' without introducing a new case while handling ioctl. Unload/Reload test has been done successfully with a patch in vhost kernel. You need a patch to enable vhost. And I think you can only do it for vhost-kenrel now since vhost-user protocol needs some extension I believe. OK. Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 42 ++ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 4543974..153f6d7 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2862,33 +2862,59 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) } } -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) +uint64_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { -return vdev->vq[n].last_avail_idx; +uint64_t num; + +num = vdev->vq[n].last_avail_idx; +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 16; +num |= ((uint64_t)vdev->vq[n].used_idx) << 32; +num |= ((uint64_t)vdev->vq[n].used_wrap_counter) << 48; So s.num is 32bit, I don't think this can even work. I mistakenly checked out s.num is 64bit, will add a new case in next version. Wouldn't be enough to just get/set avail_wrap_counter? Something like this so that it fits into 32 bits: if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 31; } Regards, Maxime Wei Thanks +} + +return num; } -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint64_t num) { -vdev->vq[n].last_avail_idx = idx; -vdev->vq[n].shadow_avail_idx = idx; +vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx = (uint16_t)(num); + +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +vdev->vq[n].avail_wrap_counter = (uint16_t)(num >> 16); +vdev->vq[n].used_idx = (uint16_t)(num >> 32); +vdev->vq[n].used_wrap_counter = (uint16_t)(num >> 48); +} } void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) { rcu_read_lock(); -if (vdev->vq[n].vring.desc) { +if (!vdev->vq[n].vring.desc) { +goto out; +} + +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); -vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx; } +vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx; + +out: rcu_read_unlock(); } void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) { rcu_read_lock(); -if (vdev->vq[n].vring.desc) { +if (!vdev->vq[n].vring.desc) { +goto out; +} + +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]); } + +out: rcu_read_unlock(); }
Re: [Qemu-devel] [RFC v2 2/8] virtio: memory cache for packed ring
On 06/05/2018 09:07 PM, w...@redhat.com wrote: From: Wei Xu Mostly reuse memory cache with 1.0 except for the offset calculation. Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 29 - 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index e192a9a..f6c0689 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -150,11 +150,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) VRingMemoryRegionCaches *old = vq->vring.caches; VRingMemoryRegionCaches *new; hwaddr addr, size; -int event_size; int64_t len; -event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0; - addr = vq->vring.desc; if (!addr) { return; @@ -168,7 +165,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) goto err_desc; } In the case of packed ring layout, the region's cache for the descs has to be initialized as write-able, as the descriptors are written-back by the device. Following patch fixes the assert I'm facing, but we might want to differentiate the split and packed cases as it is read-only in split case: diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 012c0925f2..4b165aaf2c 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -173,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) new = g_new0(VRingMemoryRegionCaches, 1); size = virtio_queue_get_desc_size(vdev, n); len = address_space_cache_init(&new->desc, vdev->dma_as, - addr, size, false); + addr, size, true); if (len < size) { virtio_error(vdev, "Cannot map desc"); goto err_desc; -size = virtio_queue_get_used_size(vdev, n) + event_size; +size = virtio_queue_get_used_size(vdev, n); len = address_space_cache_init(&new->used, vdev->dma_as, vq->vring.used, size, true); if (len < size) { @@ -176,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) goto err_used; } -size = virtio_queue_get_avail_size(vdev, n) + event_size; +size = virtio_queue_get_avail_size(vdev, n); len = address_space_cache_init(&new->avail, vdev->dma_as, vq->vring.avail, size, false); if (len < size) { @@ -2320,14 +2317,28 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n) hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n) { -return offsetof(VRingAvail, ring) + -sizeof(uint16_t) * vdev->vq[n].vring.num; +int s; + +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return sizeof(struct VRingPackedDescEvent); +} else { +s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0; +return offsetof(VRingAvail, ring) + +sizeof(uint16_t) * vdev->vq[n].vring.num + s; +} } hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) { -return offsetof(VRingUsed, ring) + -sizeof(VRingUsedElem) * vdev->vq[n].vring.num; +int s; + +if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { +return sizeof(struct VRingPackedDescEvent); +} else { +s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0; +return offsetof(VRingUsed, ring) + +sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s; +} } uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
Re: [Qemu-devel] [RFC v2 6/8] virtio: flush/push for packed ring
On 06/05/2018 09:08 PM, w...@redhat.com wrote: From: Wei Xu Signed-off-by: Wei Xu Signed-off-by: Wei Xu --- hw/virtio/virtio.c | 109 ++--- 1 file changed, 96 insertions(+), 13 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 0160d03..6f2da83 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc, virtio_tswap16s(vdev, &desc->flags); } +static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc, +MemoryRegionCache *cache, int i) +{ +virtio_tswap64s(vdev, &desc->addr); +virtio_tswap32s(vdev, &desc->len); +virtio_tswap16s(vdev, &desc->id); +virtio_tswap16s(vdev, &desc->flags); +address_space_write_cached(cache, + sizeof(VRingDescPacked) * i, desc, + sizeof(VRingDescPacked)); +address_space_cache_invalidate(cache, + sizeof(VRingDescPacked) * i, + sizeof(VRingDescPacked)); +} + static inline bool is_desc_avail(struct VRingDescPacked *desc) { return !!(desc->flags & AVAIL_DESC_PACKED(1)) != @@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num) } /* Called within rcu_read_lock(). */ -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, +static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len, unsigned int idx) { VRingUsedElem uelem; -trace_virtqueue_fill(vq, elem, len, idx); - -virtqueue_unmap_sg(vq, elem, len); - -if (unlikely(vq->vdev->broken)) { -return; -} - if (unlikely(!vq->vring.used)) { return; } @@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, vring_used_write(vq, &uelem, idx); } -/* Called within rcu_read_lock(). */ -void virtqueue_flush(VirtQueue *vq, unsigned int count) +static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem, +unsigned int len, unsigned int idx) { -uint16_t old, new; +uint16_t w, head; +VRingMemoryRegionCaches *caches; +VRingDescPacked desc = { +.addr = 0, +.flags = 0, +}; + +if (unlikely(!vq->vring.desc)) { +return; +} + +caches = vring_get_region_caches(vq); +head = vq->used_idx + idx; +head = head >= vq->vring.num ? (head - vq->vring.num) : head; +vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head); + +w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7; +desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1)); +desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w); +if (!(desc.flags & VRING_DESC_F_INDIRECT)) { +if (!(desc.flags & VRING_DESC_F_WRITE)) { +desc.len = 0; +} else { +desc.len = len; +} +} +vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head); + +/* Make sure flags has been updated */ +smp_mb(); +} + +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, +unsigned int len, unsigned int idx) +{ +trace_virtqueue_fill(vq, elem, len, idx); + +virtqueue_unmap_sg(vq, elem, len); if (unlikely(vq->vdev->broken)) { -vq->inuse -= count; return; } +if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { +virtqueue_packed_fill(vq, elem, len, idx); +} else { +virtqueue_split_fill(vq, elem, len, idx); +} +} + +/* Called within rcu_read_lock(). */ +static void virtqueue_split_flush(VirtQueue *vq, unsigned int count) +{ +uint16_t old, new; + if (unlikely(!vq->vring.used)) { return; } @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) vq->signalled_used_valid = false; } +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) +{ +if (unlikely(!vq->vring.desc)) { +return; +} + +vq->inuse -= count; +vq->used_idx += count; I think it is wrong, because count seems to be representing the number of descriptor chains. But in case of packed ring layout, used index must be incremented by the number of descriptors. For example, I'm having trouble with the ctrl queue, where the commands sent by the guest use 3 descriptors, but count is 1. +if (vq->used_idx >= vq->vring.num) { +vq->used_idx -= vq->vring.num; +vq->used_wrap_counter = !vq->used_wrap_counter; +} +} + +void virtqueue_flush(VirtQueue *vq, unsigned int count) +{ +if (unlikely(vq->vdev->broken)) { +vq->inuse -= count; +return; +} + +if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { +virtqueue_packed_flush(vq, count); +} el
Re: [Qemu-devel] [PATCH] intel_iommu: handle invalid ce for shadow sync
Hi Peter, On 09/13/2018 09:55 AM, Peter Xu wrote: There are two callers for vtd_sync_shadow_page_table_range(), one provided a valid context entry and one not. Move that fetching operation into the caller vtd_sync_shadow_page_table() where we need to fetch the context entry. Meanwhile, we should handle VTD_FR_CONTEXT_ENTRY_P properly when synchronizing shadow page tables. Having invalid context entry there is perfectly valid when we move a device out of an existing domain. When that happens, instead of posting an error we invalidate the whole region. Without this patch, QEMU will crash if we do these steps: (1) start QEMU with VT-d IOMMU and two 10G NICs (ixgbe) (2) bind the NICs with vfio-pci in the guest (3) start testpmd with the NICs applied (4) stop testpmd (5) rebind the NIC back to ixgbe kernel driver The patch should fix it. Reported-by: Pei Zhang Tested-by: Pei Zhang CC: Pei Zhang CC: Alex Williamson CC: Jason Wang CC: Maxime Coquelin CC: Michael S. Tsirkin CC: QEMU Stable Fixes:https://bugzilla.redhat.com/show_bug.cgi?id=1627272 It seems like a regression as it wasn't reported earlier, isn't it? If it is, do you know what is the faulty commit? Signed-off-by: Peter Xu --- hw/i386/intel_iommu.c | 54 ++- 1 file changed, 33 insertions(+), 21 deletions(-) Other than that, the patch looks good to me. FWIW: Acked-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
On 03/29/2018 10:05 AM, Liu, Changpeng wrote: -Original Message- From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] Sent: Thursday, March 29, 2018 3:56 PM To: Liu, Changpeng ; m...@redhat.com; marcandre.lur...@redhat.com; qemu-devel@nongnu.org Subject: Re: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature On 03/29/2018 02:57 AM, Liu, Changpeng wrote: -Original Message- From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] Sent: Thursday, March 29, 2018 3:28 AM To: m...@redhat.com; Liu, Changpeng ; marcandre.lur...@redhat.com; qemu-devel@nongnu.org Cc: Maxime Coquelin Subject: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. Vhost user init fails if the device support the feature but the backend doesn't. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin Passed our own vhost-user-blk target with the patch, I can submit a fix to QEMU vhost-user-blk example after this commit. Tested-by: Changpeng Liu Thanks for having tested, and for proposing to implement the example part. Just notice I forgot to apply your Tested-by on the series. I have added the fix to the example based on your patch, I'm wondering I can send it out for review now or wait for your commit being merged ? I think you can post, and add a note in comments that it depends on my series. Thanks, Maxime Maxime --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 22 ++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..cc8a24aa31 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@
[Qemu-devel] [PATCH v3 1/2] vhost-user-blk: set config ops before vhost-user init
As soon as vhost-user init is done, the backend may send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the notification callback before it. Also, it will be used to know whether the device supports the config feature to advertize it or not. Signed-off-by: Maxime Coquelin --- hw/block/vhost-user-blk.c | 4 ++-- hw/virtio/vhost.c | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f840f07dfe..262baca432 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->dev.vq_index = 0; s->dev.backend_features = 0; +vhost_dev_set_config_notifier(&s->dev, &blk_ops); + ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->blkcfg.num_queues = s->num_queues; } -vhost_dev_set_config_notifier(&s->dev, &blk_ops); - return; vhost_err: diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 250f886acb..b6c314e350 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1451,7 +1451,6 @@ int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, void vhost_dev_set_config_notifier(struct vhost_dev *hdev, const VhostDevConfigOps *ops) { -assert(hdev->vhost_ops); hdev->config_ops = ops; } -- 2.14.3
Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
On 03/29/2018 02:57 AM, Liu, Changpeng wrote: -Original Message- From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] Sent: Thursday, March 29, 2018 3:28 AM To: m...@redhat.com; Liu, Changpeng ; marcandre.lur...@redhat.com; qemu-devel@nongnu.org Cc: Maxime Coquelin Subject: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. Vhost user init fails if the device support the feature but the backend doesn't. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin Passed our own vhost-user-blk target with the patch, I can submit a fix to QEMU vhost-user-blk example after this commit. Tested-by: Changpeng Liu Thanks for having tested, and for proposing to implement the example part. Just notice I forgot to apply your Tested-by on the series. Maxime --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 22 ++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..cc8a24aa31 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, +VHOST_USER_PROTOCOL_F_CONFIG = 9, VHOST_USER_PROTOCOL_F_MAX }; @@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) dev->protocol_features = protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK; + +if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { +/* Dont acknowledge CONFIG feature if device doesn't support it */ +dev->protocol_features &= ~(1ULL
[Qemu-devel] [PATCH v3 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature
V3 removes the assert of vhost_dev's vhost_ops, as it is not yet set at that time. While reviewing DPDK series adding support to VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG request, I found that it was not backed with a dedicated protocol feature. This series addresses this by adding a new protocol feature bit, and by only negotiating it if the device supports it, as suggested by Michael. Indeed, if the feature is supported by other type of devices in the future, it would confuse the backends as it couldn't know whether the device really support it or not. To know whether the vhost device support config feature, the trick is to check whether it implemented the config_ops. That's the reason why the first patch moves setting the config ops in vhost-user-blk befoire calling vhost_user_init(). The series targets v2.12 release, else we may have to disable these requests in this release. *NOTE*: The series has only been tested as I don't have the environment to try it. Changpeng, can you please test it? Thanks, Maxime Changes since v2: = - Remove assert (Changpeng) - Fix typo in comment Changes since v1: = - Fail vhost-user init if device implements config feature but the backend doesn't. (mst) Maxime Coquelin (2): vhost-user-blk: set config ops before vhost-user init vhost-user: back SET/GET_CONFIG requests with a protocol feature docs/interop/vhost-user.txt | 21 - hw/block/vhost-user-blk.c | 4 ++-- hw/virtio/vhost-user.c | 22 ++ hw/virtio/vhost.c | 1 - 4 files changed, 36 insertions(+), 12 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH v3 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. Vhost user init fails if the device support the feature but the backend doesn't. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 22 ++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..38da8692bb 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, +VHOST_USER_PROTOCOL_F_CONFIG = 9, VHOST_USER_PROTOCOL_F_MAX }; @@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) dev->protocol_features = protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK; + +if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { +/* Don't acknowledge CONFIG feature if device doesn't support it */ +dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); +} else if (!(protocol_features & +(1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) { +error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG " +"but backend does not support it."); +return -1; +} + err = vhost_user_set_protocol_features(dev, dev->protocol_features); if (err < 0) { return err; @@ -1405,6 +1417,11 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, }; +if (!virtio_has_feature(dev->pr
Re: [Qemu-devel] [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init
On 03/29/2018 02:53 AM, Liu, Changpeng wrote: -Original Message- From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] Sent: Thursday, March 29, 2018 3:28 AM To: m...@redhat.com; Liu, Changpeng ; marcandre.lur...@redhat.com; qemu-devel@nongnu.org Cc: Maxime Coquelin Subject: [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init As soon as vhost-user init is done, the backend may send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the notification callback before it. Also, it will be used to know whether the device supports the config feature to advertize it or not. Signed-off-by: Maxime Coquelin --- hw/block/vhost-user-blk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f840f07dfe..262baca432 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->dev.vq_index = 0; s->dev.backend_features = 0; +vhost_dev_set_config_notifier(&s->dev, &blk_ops); Please also remove the line "assert(hdev->vhost_ops);" in function vhost_dev_set_config_notifier at vhost.c file. Oh right, posting v3 removing this. Thanks, Maxime + ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->blkcfg.num_queues = s->num_queues; } -vhost_dev_set_config_notifier(&s->dev, &blk_ops); - return; vhost_err: -- 2.14.3
[Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. Vhost user init fails if the device support the feature but the backend doesn't. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 22 ++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..cc8a24aa31 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, +VHOST_USER_PROTOCOL_F_CONFIG = 9, VHOST_USER_PROTOCOL_F_MAX }; @@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) dev->protocol_features = protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK; + +if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { +/* Dont acknowledge CONFIG feature if device doesn't support it */ +dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); +} else if (!(protocol_features & +(1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) { +error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG " +"but backend does not support it."); +return -1; +} + err = vhost_user_set_protocol_features(dev, dev->protocol_features); if (err < 0) { return err; @@ -1405,6 +1417,11 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, }; +if (!virtio_has_feature(dev->protoco
[Qemu-devel] [PATCH v2 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature
V2 makes vhost-user init to fail if the device implements config feature but the backend doesn't. While reviewing DPDK series adding support to VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG request, I found that it was not backed with a dedicated protocol feature. This series addresses this by adding a new protocol feature bit, and by only negotiating it if the device supports it, as suggested by Michael. Indeed, if the feature is supported by other type of devices in the future, it would confuse the backends as it couldn't know whether the device really support it or not. To know whether the vhost device support config feature, the trick is to check whether it implemented the config_ops. That's the reason why the first patch moves setting the config ops in vhost-user-blk befoire calling vhost_user_init(). The series targets v2.12 release, else we may have to disable these requests in this release. *NOTE*: The series has only been tested as I don't have the environment to try it. Changpeng, can you please test it? Thanks, Maxime Changes since v1: = - Fail vhost-user init if device implements config feature but the backend doesn't. (mst) Maxime Coquelin (2): vhost-user-blk: set config ops before vhost-user init vhost-user: back SET/GET_CONFIG requests with a protocol feature docs/interop/vhost-user.txt | 21 - hw/block/vhost-user-blk.c | 4 ++-- hw/virtio/vhost-user.c | 22 ++ 3 files changed, 36 insertions(+), 11 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH v2 1/2] vhost-user-blk: set config ops before vhost-user init
As soon as vhost-user init is done, the backend may send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the notification callback before it. Also, it will be used to know whether the device supports the config feature to advertize it or not. Signed-off-by: Maxime Coquelin --- hw/block/vhost-user-blk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f840f07dfe..262baca432 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->dev.vq_index = 0; s->dev.backend_features = 0; +vhost_dev_set_config_notifier(&s->dev, &blk_ops); + ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->blkcfg.num_queues = s->num_queues; } -vhost_dev_set_config_notifier(&s->dev, &blk_ops); - return; vhost_err: -- 2.14.3
Re: [Qemu-devel] [PATCH 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
On 03/28/2018 06:55 PM, Michael S. Tsirkin wrote: On Wed, Mar 28, 2018 at 05:56:57PM +0200, Maxime Coquelin wrote: Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin I presume vhost user blk should fail init if the protocol feature isn't negotiated then. I did that and finally removed it. In the future, if for example we add config support for net device, we will want init to succeed even with old backend version that does not support it, right? For the vhost user blk case, its init will fail right after, because it tries to get config, but will get an error instead. As we only have vhost-user-blk supporting it for now, and since it is a mandatory feature, I fine to post a v2 that makes vhost_user_init() to fail. --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 17 + 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..a045203b26 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, +VHOST_USER_PROTOCOL_F_CONFIG = 9, VHOST_USER_PROTOCOL_F_MAX }; @@ -1211,6 +1212,12 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) dev->protocol_features = protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK; + +if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { +/* Dont acknowledge CONFIG feature if device doesn't support it */ +dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); +}
Re: [Qemu-devel] [PATCH 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature
On 03/28/2018 05:56 PM, Maxime Coquelin wrote: Hi, While reviewing DPDK series adding support to VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG request, I found that it was not backed with a dedicated protocol feature. This series addresses this by adding a new protocol feature bit, and by only negotiating it if the device supports it, as suggested by Michael. Indeed, if the feature is supported by other type of devices in the future, it would confuse the backends as it couldn't know whether the device really support it or not. To know whether the vhost device support config feature, the trick is to check whether it implemented the config_ops. That's the reason why the first patch moves setting the config ops in vhost-user-blk befoire calling vhost_user_init(). The series targets v2.12 release, else we may have to disable these requests in this release. *NOTE*: The series has only been tested as I don't have the s/tested/build tested/ environment to try it. Changpeng, can you please test it? Thanks, Maxime Maxime Coquelin (2): vhost-user-blk: set config ops before vhost-user init vhost-user: back SET/GET_CONFIG requests with a protocol feature docs/interop/vhost-user.txt | 21 - hw/block/vhost-user-blk.c | 4 ++-- hw/virtio/vhost-user.c | 17 + 3 files changed, 31 insertions(+), 11 deletions(-)
[Qemu-devel] [PATCH 1/2] vhost-user-blk: set config ops before vhost-user init
As soon as vhost-user init is done, the backend may send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the notification callback before it. Also, it will be used to know whether the device supports the config feature to advertize it or not. Signed-off-by: Maxime Coquelin --- hw/block/vhost-user-blk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f840f07dfe..262baca432 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->dev.vq_index = 0; s->dev.backend_features = 0; +vhost_dev_set_config_notifier(&s->dev, &blk_ops); + ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", @@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->blkcfg.num_queues = s->num_queues; } -vhost_dev_set_config_notifier(&s->dev, &blk_ops); - return; vhost_err: -- 2.14.3
[Qemu-devel] [PATCH 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature
Without a dedicated protocol feature, QEMU cannot know whether the backend can handle VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG messages. This patch adds a protocol feature that is only advertised by QEMU if the device implements the config ops. The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests if the protocol feature has been negotiated. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 21 - hw/virtio/vhost-user.c | 17 + 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index c058c407df..534caab18a 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -379,6 +379,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 Master message types @@ -664,7 +665,8 @@ Master message types Master payload: virtio device config space Slave payload: virtio device config space - Submitted by the vhost-user master to fetch the contents of the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may @@ -677,7 +679,8 @@ Master message types Master payload: virtio device config space Slave payload: N/A - Submitted by the vhost-user master when the Guest changes the virtio + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is + submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only @@ -766,13 +769,13 @@ Slave message types Slave payload: N/A Master payload: N/A - Vhost-user slave sends such messages to notify that the virtio device's - configuration space has changed, for those host devices which can support - such feature, host driver can send VHOST_USER_GET_CONFIG message to slave - to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is - negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must - respond with zero when operation is successfully completed, or non-zero - otherwise. + When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends + such messages to notify that the virtio device's configuration space has + changed, for those host devices which can support such feature, host + driver can send VHOST_USER_GET_CONFIG message to slave to get the latest + content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set + the VHOST_USER_NEED_REPLY flag, master must respond with zero when + operation is successfully completed, or non-zero otherwise. VHOST_USER_PROTOCOL_F_REPLY_ACK: --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 44aea5c0a8..a045203b26 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7, VHOST_USER_PROTOCOL_F_PAGEFAULT = 8, +VHOST_USER_PROTOCOL_F_CONFIG = 9, VHOST_USER_PROTOCOL_F_MAX }; @@ -1211,6 +1212,12 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) dev->protocol_features = protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK; + +if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { +/* Dont acknowledge CONFIG feature if device doesn't support it */ +dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); +} + err = vhost_user_set_protocol_features(dev, dev->protocol_features); if (err < 0) { return err; @@ -1405,6 +1412,11 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len, }; +if (!virtio_has_feature(dev->protocol_features, +VHOST_USER_PROTOCOL_F_CONFIG)) { +return -1; +} + if (config_len > VHOST_USER_MAX_CONFIG_SIZE) { return -1; } @@ -1448,6 +1460,11 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data, .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + size, }; +if (!virtio_has_feature(dev->protocol_featur
[Qemu-devel] [PATCH 0/2] vhost-user: Back SET/GET_CONFIG with a protocol feature
Hi, While reviewing DPDK series adding support to VHOST_USER_SET_CONFIG and VHOST_USER_GET_CONFIG request, I found that it was not backed with a dedicated protocol feature. This series addresses this by adding a new protocol feature bit, and by only negotiating it if the device supports it, as suggested by Michael. Indeed, if the feature is supported by other type of devices in the future, it would confuse the backends as it couldn't know whether the device really support it or not. To know whether the vhost device support config feature, the trick is to check whether it implemented the config_ops. That's the reason why the first patch moves setting the config ops in vhost-user-blk befoire calling vhost_user_init(). The series targets v2.12 release, else we may have to disable these requests in this release. *NOTE*: The series has only been tested as I don't have the environment to try it. Changpeng, can you please test it? Thanks, Maxime Maxime Coquelin (2): vhost-user-blk: set config ops before vhost-user init vhost-user: back SET/GET_CONFIG requests with a protocol feature docs/interop/vhost-user.txt | 21 - hw/block/vhost-user-blk.c | 4 ++-- hw/virtio/vhost-user.c | 17 + 3 files changed, 31 insertions(+), 11 deletions(-) -- 2.14.3
Re: [Qemu-devel] [PULL v4 02/29] vhost-user: add new vhost user messages to support virtio config space
Hi, On 01/18/2018 09:44 PM, Michael S. Tsirkin wrote: From: Changpeng Liu 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_SLAVE_CONFIG_CHANGE_MSG message is added as the event notifier in case virtio config space change in the slave I/O target. Signed-off-by: Changpeng Liu Reviewed-by: Marc-André Lureau Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- docs/interop/vhost-user.txt | 55 ++ include/hw/virtio/vhost-backend.h | 12 include/hw/virtio/vhost.h | 15 + hw/virtio/vhost-user.c| 118 ++ hw/virtio/vhost.c | 32 +++ 5 files changed, 232 insertions(+) As discussed with Changpeng on DPDK ML, we need a vhost-user protocol feature bit to back these new requests, else QEMU cannot whether the user backend implement these. With current and older DPDK version, if an unknown request is received, the socket connection is closed. Do we still have time before QEMU v2.12 is out? Thanks, Maxime
Re: [Qemu-devel] [RFC 2/5] vhost-user: Introduce new request to send virtio device status
On 02/27/2018 04:01 PM, Michael S. Tsirkin wrote: On Fri, Feb 16, 2018 at 06:29:07PM +0100, Maxime Coquelin wrote: diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 9fcf48d611..daa452bd36 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -368,6 +368,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_MTU4 #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 +#define VHOST_USER_PROTOCOL_F_VIRTIO_STATUS 7 Master message types @@ -663,6 +664,19 @@ Master message types field, and slaves MUST NOT accept SET_CONFIG for read-only configuration space fields unless the live migration bit is set. +* VHOST_USER_SET_VIRTIO_STATUS + + Id: 26 + Equivalent ioctl: N/A + Master payload: u64 + Slave payload: N/A + + Sent by the vhost-user master to notify of virtio device status change. + The payload is a u64 representing the virtio device status as defined in + the virtio specification. + The request should be sent only when VHOST_USER_PROTOCOL_F_VIRTIO_STATUS + protocol feature has been negotiated. + Slave message types --- So for now backend was only activated after DRIVER_OK. Does this message mean that we must send updates such as _DRIVER as well? Yes, even if I don't see a use for _ACKNOWLEDGE and _DRIVER today. Further, this is kind of one-way, but there are several cases where device modifies the status. One is NEEDS_RESET. Another is clearing of FEATURES_OK. Do you mean we should also notify the backend in case of NEEDS_RESET, or clearing of FEATURES_OK? Or you mean we should provide a way for the backend to update the device status, e.g. by having a slave-initiated VHOST_USER_SET_VIRTIO_STATUS request? Thanks, Maxime
Re: [Qemu-devel] [PATCH 3/6] vhost-user-test: add back memfd check
On 02/15/2018 10:25 PM, Marc-André Lureau wrote: This revert commit fb68096da3d35e64c88cd610c1fa42766c58e92a, and modify test_read_guest_mem() to use different chardev names, when using memfd (_test_server_free(), where the chardev is removed, runs in idle). Signed-off-by: Marc-André Lureau --- tests/vhost-user-test.c | 93 +++-- 1 file changed, 66 insertions(+), 27 deletions(-) Looks good to me: Acked-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [PATCH 4/6] vhost-user-test: do not hang if chardev creation failed
On 02/15/2018 10:25 PM, Marc-André Lureau wrote: Before the chardev name fix, the following error may happen: "attempt to add duplicate property 'chr-test' to object (type 'container')", due to races. Sadly, error_vprintf() uses g_test_message(), so you have to use read the cryptic --debug-log to see it. Later, it would make sense to use g_critical() instead, and catch errors with g_test_expect_message() (in glib 2.34). Signed-off-by: Marc-André Lureau --- tests/vhost-user-test.c | 1 + 1 file changed, 1 insertion(+) Acked-by: Maxime Coquelin Thanks, Maxime
[Qemu-devel] [RFC 3/5] vhost_net: send virtio device status update to the backend
This patch adds ans uses a new function to send virtio device status updates to the backend. Suggested-by: Stefan Hajnoczi Signed-off-by: Maxime Coquelin --- hw/net/vhost_net.c | 10 ++ hw/net/virtio-net.c | 7 ++- include/net/vhost_net.h | 2 ++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index e037db63a3..75e2165163 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -450,6 +450,11 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu) return vhost_ops->vhost_net_set_mtu(&net->dev, mtu); } +int vhost_net_set_virtio_status(struct vhost_net *net, uint8_t status) +{ +return vhost_dev_set_virtio_status(&net->dev, status); +} + #else uint64_t vhost_net_get_max_queues(VHostNetState *net) { @@ -521,4 +526,9 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu) { return 0; } + +int vhost_net_set_virtio_status(struct vhost_net *net, uint8_t status) +{ +return 0; +} #endif diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 38674b08aa..06c431ea28 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -136,7 +136,7 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) if ((virtio_net_started(n, status) && !nc->peer->link_down) == !!n->vhost_started) { -return; +goto out; } if (!n->vhost_started) { int r, i; @@ -175,11 +175,16 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) error_report("unable to start vhost net: %d: " "falling back on userspace virtio", -r); n->vhost_started = 0; + +return; } } else { vhost_net_stop(vdev, n->nic->ncs, queues); n->vhost_started = 0; } + +out: +vhost_net_set_virtio_status(get_vhost_net(nc->peer), status); } static int virtio_net_set_vnet_endian_one(VirtIODevice *vdev, diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h index afc1499eb9..bf083e2d2e 100644 --- a/include/net/vhost_net.h +++ b/include/net/vhost_net.h @@ -37,4 +37,6 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net); int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu); +int vhost_net_set_virtio_status(struct vhost_net *net, uint8_t status); + #endif -- 2.14.3
[Qemu-devel] [RFC 2/5] vhost-user: Introduce new request to send virtio device status
This patch implements the .vhost_set_virtio_status() backend callback for user backend by intooducing a new vhost-user VHOST_USER_SET_VIRTIO_STATUS request. Suggested-by: Stefan Hajnoczi Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 14 ++ hw/virtio/vhost-user.c | 35 +++ 2 files changed, 49 insertions(+) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 9fcf48d611..daa452bd36 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -368,6 +368,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_MTU4 #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 +#define VHOST_USER_PROTOCOL_F_VIRTIO_STATUS 7 Master message types @@ -663,6 +664,19 @@ Master message types field, and slaves MUST NOT accept SET_CONFIG for read-only configuration space fields unless the live migration bit is set. +* VHOST_USER_SET_VIRTIO_STATUS + + Id: 26 + Equivalent ioctl: N/A + Master payload: u64 + Slave payload: N/A + + Sent by the vhost-user master to notify of virtio device status change. + The payload is a u64 representing the virtio device status as defined in + the virtio specification. + The request should be sent only when VHOST_USER_PROTOCOL_F_VIRTIO_STATUS + protocol feature has been negotiated. + Slave message types --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6eb97980ad..519646799b 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -39,6 +39,7 @@ 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_VIRTIO_STATUS = 7, VHOST_USER_PROTOCOL_F_MAX }; @@ -72,6 +73,7 @@ typedef enum VhostUserRequest { VHOST_USER_SET_VRING_ENDIAN = 23, VHOST_USER_GET_CONFIG = 24, VHOST_USER_SET_CONFIG = 25, +VHOST_USER_SET_VIRTIO_STATUS = 26, VHOST_USER_MAX } VhostUserRequest; @@ -1054,6 +1056,38 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data, return 0; } +static int vhost_user_set_virtio_status(struct vhost_dev *dev, uint8_t status) +{ +bool reply_supported = virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_REPLY_ACK); + +VhostUserMsg msg = { +.hdr.request = VHOST_USER_SET_VIRTIO_STATUS, +.hdr.flags = VHOST_USER_VERSION, +.hdr.size = sizeof(msg.payload.u64), +.payload.u64 = status, +}; + +if (!virtio_has_feature(dev->protocol_features, +VHOST_USER_PROTOCOL_F_VIRTIO_STATUS)) { +return 0; +} + +if (reply_supported) { +msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; +} + +if (vhost_user_write(dev, &msg, NULL, 0) < 0) { +return -1; +} + +if (reply_supported) { +return process_message_reply(dev, &msg); +} + +return 0; +} + const VhostOps user_ops = { .backend_type = VHOST_BACKEND_TYPE_USER, .vhost_backend_init = vhost_user_init, @@ -1082,4 +1116,5 @@ const VhostOps user_ops = { .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_virtio_status = vhost_user_set_virtio_status, }; -- 2.14.3
[Qemu-devel] [RFC 5/5] vhost-user-scsi: send virtio status to the backend
Suggested-by: Stefan Hajnoczi Signed-off-by: Maxime Coquelin --- hw/scsi/vhost-user-scsi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index 9389ed48e0..da36a3b7f5 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -58,6 +58,8 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) } else { vhost_scsi_common_stop(vsc); } + +vhost_dev_set_virtio_status(&vsc->dev, status); } static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq) -- 2.14.3
[Qemu-devel] [RFC 4/5] vhost-user-blk: send virtio status to the backend
Suggested-by: Stefan Hajnoczi Signed-off-by: Maxime Coquelin --- hw/block/vhost-user-blk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index b53b4c9c57..a88b6f13a4 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -190,6 +190,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) vhost_user_blk_stop(vdev); } +vhost_dev_set_virtio_status(&s->dev, status); } static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, -- 2.14.3
[Qemu-devel] [RFC 0/5] vhost-user: Forward virtio device status updates
This series introduces a new vhost-user request to notify the backend with virtio device status updates. This is done to address the case when the guest driver only intializes a subset of the virtqueues. For example, it happens with Windows virtio-net driver, when the virtio-net device has more queue pairs than vCPUs. With Virtio 1.0 devices, the driver sets DRIVER_OK after having intialized all queues, so the backend can use this information to start the vhost port. With legacy devices, this is not guaranteed as mentionned in the spec, so the backend should not rely on DRIVER_OK. A solution has yet to be found for legacy devices. Maxime Coquelin (5): vhost: send virtio device status update to the backend vhost-user: Introduce new request to send virtio device status vhost_net: send virtio device status update to the backend vhost-user-blk: send virtio status to the backend vhost-user-scsi: send virtio status to the backend docs/interop/vhost-user.txt | 14 ++ hw/block/vhost-user-blk.c | 1 + hw/net/vhost_net.c| 10 ++ hw/net/virtio-net.c | 7 ++- hw/scsi/vhost-user-scsi.c | 2 ++ hw/virtio/vhost-user.c| 35 +++ hw/virtio/vhost.c | 11 +++ include/hw/virtio/vhost-backend.h | 3 +++ include/hw/virtio/vhost.h | 3 +++ include/net/vhost_net.h | 2 ++ 10 files changed, 87 insertions(+), 1 deletion(-) -- 2.14.3
[Qemu-devel] [RFC 1/5] vhost: send virtio device status update to the backend
This patch adds a function to notify the vhost backend with virtio device status updates. Suggested-by: Stefan Hajnoczi Signed-off-by: Maxime Coquelin --- hw/virtio/vhost.c | 11 +++ include/hw/virtio/vhost-backend.h | 3 +++ include/hw/virtio/vhost.h | 3 +++ 3 files changed, 17 insertions(+) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 338e4395b7..95981e5a32 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1546,6 +1546,17 @@ void vhost_dev_set_config_notifier(struct vhost_dev *hdev, hdev->config_ops = ops; } +int vhost_dev_set_virtio_status(struct vhost_dev *hdev, uint8_t status) +{ +const VhostOps *vhost_ops = hdev->vhost_ops; + +if (vhost_ops && vhost_ops->vhost_set_virtio_status) { +return vhost_ops->vhost_set_virtio_status(hdev, status); +} + +return 0; +} + /* Host notifiers must be enabled at this point. */ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) { diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index 592254f40d..45f6b8f6c5 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -94,6 +94,8 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data, uint32_t flags); typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, uint32_t config_len); +typedef int (*vhost_set_virtio_status_op)(struct vhost_dev *dev, + uint8_t status); typedef struct VhostOps { VhostBackendType backend_type; @@ -130,6 +132,7 @@ typedef struct VhostOps { 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_virtio_status_op vhost_set_virtio_status; } VhostOps; extern const VhostOps user_ops; diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 1dc2d73d76..fe055b2bd5 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -121,4 +121,7 @@ int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, */ void vhost_dev_set_config_notifier(struct vhost_dev *dev, const VhostDevConfigOps *ops); + +int vhost_dev_set_virtio_status(struct vhost_dev *hdev, uint8_t status); + #endif -- 2.14.3
Re: [Qemu-devel] [PATCH] libvhost-user: Support across-memory-boundary access
On 01/18/2018 05:04 PM, Yongji Xie wrote: The sg list/indirect descriptor table may be contigious in GPA but not in HVA address space. But libvhost-user wasn't aware of that. This would cause out-of-bounds access. Even a malicious guest could use it to get information from the vhost-user backend. Introduce a plen parameter in vu_gpa_to_va() so we can handle this case, returning the actual mapped length. Signed-off-by: Yongji Xie --- contrib/libvhost-user/libvhost-user.c | 133 + contrib/libvhost-user/libvhost-user.h |3 +- 2 files changed, 122 insertions(+), 14 deletions(-) Reviewed-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [PATCH] libvhost-user: Fix resource leak
On 01/18/2018 04:41 PM, Yongji Xie wrote: Free the mmaped memory when we need to mmap new memory space on vu_set_mem_table_exec() and vu_set_log_base_exec() to avoid memory leak. Also close the corresponding fd after mmap() on vu_set_log_base_exec() to avoid fd leak. Signed-off-by: Yongji Xie --- contrib/libvhost-user/libvhost-user.c | 14 ++ 1 file changed, 14 insertions(+) Reviewed-by: Maxime Coquelin Thanks, Maxime
Re: [Qemu-devel] [PATCH 2/4] vhost-user: specify and implement VHOST_USER_SET_QUEUE_NUM request
On 01/16/2018 04:05 AM, Michael S. Tsirkin wrote: On Fri, Jan 12, 2018 at 03:56:56PM +0100, Maxime Coquelin wrote: When the slave cannot add queues dynamically, Could you please clarify the motivation a bit Why is it such a big deal to resize the queue array? We know no queues are used before all of them are initialized, so you don't need to worry about synchronizing with threads processing the queues at the same time. The problem is to know how many queues to wait for being initialized. We need this information in DPDK to start the "vhost device". Before the guest driver is initialized, the slave receives from QEMU VHOST_USER_SET_VRING_CALL and VHOST_USER_SET_VRING_ENABLE for all queues declared in QEMU. In DPDK, queues metadata is allocated each time a message targets a new vring index, and vring count is incremented accordingly. Currently, it seems to work, because QEMU calls vhost_virtqueue_start() for all queues, even ones not configured by the guest. But with patch "[PATCH] vhost: fix corrupting GPA 0 when using uninitialized queues" from Zheng Xiang, QEMU will not more send _SET_VRING_ADDR and _SET_VRING_KICK for un-configured queues, but the backend will wait forever for these messages to start the device. I pasted below a log of messages received by the slave, so you can make an idea of the sequence. it needs to know for how many queues to wait to be initialized. This patch introduce new vhost-user protocol feature & request for the master to send the number of queue pairs allocated by the driver. Assuming we can fix the previous message, I think specifying the # of queues would be better. Thanks, Maxime EAL: Detected 4 lcore(s) EAL: No free hugepages reported in hugepages-1048576kB EAL: Probing VFIO support... EAL: PCI device :00:1f.6 on NUMA socket -1 EAL: Invalid NUMA socket, default to 0 EAL: probe driver: 8086:156f net_e1000_em PMD: Initializing pmd_vhost for net_vhost0 PMD: Creating VHOST-USER backend on numa socket 0 VHOST_CONFIG: vhost-user server: socket created, fd: 13 VHOST_CONFIG: bind to /tmp/vhost-user2 Interactive-mode selected testpmd: create a new mbuf pool : n=155456, size=2176, socket=0 Warning! Cannot handle an odd number of ports with the current port topology. Configuration must be changed to have an even number of ports, or relaunch application with --port-topology=chained Configuring Port 0 (socket 0) Port 0: 56:48:4F:53:54:00 Checking link statuses... Done testpmd> VHOST_CONFIG: new vhost user connection is 15 VHOST_CONFIG: new device, handle is 0 VHOST_CONFIG: read message VHOST_USER_GET_FEATURES VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM VHOST_CONFIG: read message VHOST_USER_SET_SLAVE_REQ_FD VHOST_CONFIG: read message VHOST_USER_SET_OWNER VHOST_CONFIG: read message VHOST_USER_GET_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL VHOST_CONFIG: vring call idx:0 file:17 VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL VHOST_CONFIG: vring call idx:1 file:18 VHOST_CONFIG: read message VHOST_USER_GET_FEATURES VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_SLAVE_REQ_FD VHOST_CONFIG: read message VHOST_USER_GET_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL VHOST_CONFIG: vring call idx:2 file:20 VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL VHOST_CONFIG: vring call idx:3 file:21 VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 1 to qp idx: 0 PMD: vring0 is enabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 1 to qp idx: 1 PMD: vring1 is enabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 0 to qp idx: 2 PMD: vring2 is disabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 0 to qp idx: 3 PMD: vring3 is disabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 1 to qp idx: 0 PMD: vring0 is enabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 1 to qp idx: 1 PMD: vring1 is enabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 0 to qp idx: 2 PMD: vring2 is disabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE VHOST_CONFIG: set queue enable: 0 to qp idx: 3 PMD: vring3 is disabled Port 0: Queue state event VHOST_CONFIG: read message VHOST_USER_SET_FEATURES VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE VHOST_CONFIG: guest memory region 0, size: 0x8
Re: [Qemu-devel] [PATCH 4/4] virtio-net: notify backend with number of queue pairs setup
On 01/16/2018 04:07 AM, Michael S. Tsirkin wrote: On Fri, Jan 12, 2018 at 03:56:58PM +0100, Maxime Coquelin wrote: Signed-off-by: Maxime Coquelin --- hw/net/virtio-net.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 38674b08aa..b8908c98ed 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -561,6 +561,7 @@ static int peer_detach(VirtIONet *n, int index) static void virtio_net_set_queues(VirtIONet *n) { +NetClientState *nc = qemu_get_queue(n->nic); int i; int r; @@ -568,6 +569,10 @@ static void virtio_net_set_queues(VirtIONet *n) return; } +if (get_vhost_net(nc->peer)) { +vhost_net_set_queue_num(nc->peer, n->curr_queues); +} + for (i = 0; i < n->max_queues; i++) { if (i < n->curr_queues) { r = peer_attach(n, i); Seems wrong to me. curr_queues isn't the max # of queues configured as the documentation says. It's the number of queues currently in use by driver. Ok. What about detecting the number of queues configured, by checking for example that decs_phys, avail_phys and used_phys are different? Thanks, Maxime -- 2.14.3
Re: [Qemu-devel] [PATCH 1/4] vhost-user: fix multiple queue specification
On 01/16/2018 04:00 AM, Michael S. Tsirkin wrote: On Fri, Jan 12, 2018 at 03:56:55PM +0100, Maxime Coquelin wrote: The number of queues supported by the slave is queried with message VHOST_USER_GET_QUEUE_NUM, not with message VHOST_USER_GET_PROTOCOL_FEATURES. Also, looking at master and slave implemntations, the payload returned by the slave is the number of queue pairs supported by the slave, not the number of queues. virtio doesn't have a concept of queue pairs. virtio net does have a concept of a tx/rx pair for purposes of steering. Ok, thanks for the clarification. I will have a look at how vhost-user SCSI implements it. Would this be a slave bug then? If I'm not mistaken, the bug is in QEMU: VHOST_USER_GET_QUEUE_NUM is stored in (struct vhost_dev).max_queues. vhost_net_get_max_queues() returns (struct vhost_dev).max_queues. And vhost_user_start() from net/vhost-user.c calls vhost_net_get_max_queues() to get the max number of tx/rx pairs. If we want to fix QEMU, I think we will need a new flag for compatibility with older/current backends that assume it represent a queue pair. I've applied the 1st chunk for now. Thanks. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index d49444e037..8a14191a1e 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -214,8 +214,8 @@ Multiple queue is treated as a protocol extension, hence the slave has to implement protocol features first. The multiple queues feature is supported only when the protocol feature VHOST_USER_PROTOCOL_F_MQ (bit 0) is set. -The max number of queues the slave supports can be queried with message -VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of +The max number of queue pairs the slave supports can be queried with message +VHOST_USER_GET_QUEUE_NUM. Master should stop when the number of requested queues is bigger than that. As all queues share one connection, the master uses a unique index for each @@ -537,7 +537,7 @@ Master message types Master payload: N/A Slave payload: u64 - Query how many queues the backend supports. This request should be + Query how many queue pairs the backend supports. This request should be sent only when VHOST_USER_PROTOCOL_F_MQ is set in queried protocol features by VHOST_USER_GET_PROTOCOL_FEATURES. -- 2.14.3
[Qemu-devel] [PATCH 2/4] vhost-user: specify and implement VHOST_USER_SET_QUEUE_NUM request
When the slave cannot add queues dynamically, it needs to know for how many queues to wait to be initialized. This patch introduce new vhost-user protocol feature & request for the master to send the number of queue pairs allocated by the driver. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 16 hw/virtio/vhost-user.c| 24 include/hw/virtio/vhost-backend.h | 3 +++ 3 files changed, 43 insertions(+) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 8a14191a1e..85c0e03a95 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -218,6 +218,9 @@ The max number of queue pairs the slave supports can be queried with message VHOST_USER_GET_QUEUE_NUM. Master should stop when the number of requested queues is bigger than that. +When VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM is negotiated, the master must send +the number of queue pairs initialized with message VHOST_USER_SET_QUEUE_NUM. + As all queues share one connection, the master uses a unique index for each queue in the sent message to identify a specified queue. One queue pair is enabled initially. More queues are enabled dynamically, by sending @@ -354,6 +357,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_MTU4 #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 +#define VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM 7 Master message types @@ -623,6 +627,18 @@ Master message types and expect this message once (per VQ) during device configuration (ie. before the master starts the VQ). + * VHOST_USER_SET_QUEUE_NUM + + Id: 24 + Equivalent ioctl: N/A + Master payload: u64 + + Set the number of queue pairs initialized. + Master sends such request to notify the slave the number of queue pairs + that have been initialized. + This request should only be sent if VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM + feature has been successfully negotiated. + Slave message types --- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 093675ed98..9e7728d2da 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -34,6 +34,7 @@ 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_SET_QUEUE_NUM = 7, VHOST_USER_PROTOCOL_F_MAX }; @@ -65,6 +66,7 @@ typedef enum VhostUserRequest { VHOST_USER_SET_SLAVE_REQ_FD = 21, VHOST_USER_IOTLB_MSG = 22, VHOST_USER_SET_VRING_ENDIAN = 23, +VHOST_USER_SET_QUEUE_NUM = 24, VHOST_USER_MAX } VhostUserRequest; @@ -922,6 +924,27 @@ 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_set_queue_num(struct vhost_dev *dev, uint64_t queues) +{ +VhostUserMsg msg = { +.request = VHOST_USER_SET_QUEUE_NUM, +.size = sizeof(msg.payload.u64), +.flags = VHOST_USER_VERSION, +.payload.u64 = queues, +}; + +if (!(dev->protocol_features & +(1ULL << VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM))) { +return 0; +} + +if (vhost_user_write(dev, &msg, NULL, 0) < 0) { +return -1; +} + +return 0; +} + const VhostOps user_ops = { .backend_type = VHOST_BACKEND_TYPE_USER, .vhost_backend_init = vhost_user_init, @@ -948,4 +971,5 @@ 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_set_queue_num = vhost_user_set_queue_num, }; diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index a7a5f22bc6..1dd3e4bbf3 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -84,6 +84,8 @@ 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_queue_num_op)(struct vhost_dev *dev, + uint64_t queues); typedef struct VhostOps { VhostBackendType backend_type; @@ -118,6 +120,7 @@ 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_set_queue_num_op vhost_set_queue_num; } VhostOps; extern const VhostOps user_ops; -- 2.14.3
[Qemu-devel] [PATCH 0/4] vhost-user: notify backend with number of queues setup
This series introduces a new vhost-user protocol request for QEMU to notify the backend with the number of queues setup by the guest driver. When the user backend cannot add queues dynamically (i.e. once the port is running), it waits for all queues to be initialized. Problem is that QEMU sends messages for all queues declared in command line, even the ones no setup by the guest driver. Without fix from Zheng Xiang [0] that doesn't start queues that haven't been setup by the guest, it ends up corrupting memory around GPA 0 as SET_VRING_ADDR is sent with uninitialized ring addresses. With the fix, the DPDK backend would be stuck forever, waiting for unused queues to be setup, which won't happen. Note that these problems are met neither with virtio-net Linux driver, nor DPDK's Virtio PMD, because these drivers always setup all queues provided by QEMU, even if they doesn't use all of them. However, the problem can be reproduced with Windows guest, when QEMU declares more queue pairs than vcpus. In this case, the Windows virtio-net driver only setup as much queue pairs as vcpus. [0]: https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg02484.html Maxime Coquelin (4): vhost-user: fix multiple queue specification vhost-user: specify and implement VHOST_USER_SET_QUEUE_NUM request vhost-net: add vhost_net_set_queue_num helper virtio-net: notify backend with number of queue pairs setup docs/interop/vhost-user.txt | 22 +++--- hw/net/vhost_net.c| 17 + hw/net/virtio-net.c | 5 + hw/virtio/vhost-user.c| 24 include/hw/virtio/vhost-backend.h | 3 +++ include/net/vhost_net.h | 1 + 6 files changed, 69 insertions(+), 3 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH 1/4] vhost-user: fix multiple queue specification
The number of queues supported by the slave is queried with message VHOST_USER_GET_QUEUE_NUM, not with message VHOST_USER_GET_PROTOCOL_FEATURES. Also, looking at master and slave implemntations, the payload returned by the slave is the number of queue pairs supported by the slave, not the number of queues. Signed-off-by: Maxime Coquelin --- docs/interop/vhost-user.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index d49444e037..8a14191a1e 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -214,8 +214,8 @@ Multiple queue is treated as a protocol extension, hence the slave has to implement protocol features first. The multiple queues feature is supported only when the protocol feature VHOST_USER_PROTOCOL_F_MQ (bit 0) is set. -The max number of queues the slave supports can be queried with message -VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of +The max number of queue pairs the slave supports can be queried with message +VHOST_USER_GET_QUEUE_NUM. Master should stop when the number of requested queues is bigger than that. As all queues share one connection, the master uses a unique index for each @@ -537,7 +537,7 @@ Master message types Master payload: N/A Slave payload: u64 - Query how many queues the backend supports. This request should be + Query how many queue pairs the backend supports. This request should be sent only when VHOST_USER_PROTOCOL_F_MQ is set in queried protocol features by VHOST_USER_GET_PROTOCOL_FEATURES. -- 2.14.3
[Qemu-devel] [PATCH 4/4] virtio-net: notify backend with number of queue pairs setup
Signed-off-by: Maxime Coquelin --- hw/net/virtio-net.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 38674b08aa..b8908c98ed 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -561,6 +561,7 @@ static int peer_detach(VirtIONet *n, int index) static void virtio_net_set_queues(VirtIONet *n) { +NetClientState *nc = qemu_get_queue(n->nic); int i; int r; @@ -568,6 +569,10 @@ static void virtio_net_set_queues(VirtIONet *n) return; } +if (get_vhost_net(nc->peer)) { +vhost_net_set_queue_num(nc->peer, n->curr_queues); +} + for (i = 0; i < n->max_queues; i++) { if (i < n->curr_queues) { r = peer_attach(n, i); -- 2.14.3
[Qemu-devel] [PATCH 3/4] vhost-net: add vhost_net_set_queue_num helper
This patch adds a new help to notify the backend with the number of queue pairs setup by the guest driver. Signed-off-by: Maxime Coquelin --- hw/net/vhost_net.c | 17 + include/net/vhost_net.h | 1 + 2 files changed, 18 insertions(+) diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index e037db63a3..d60e237a34 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -124,6 +124,18 @@ uint64_t vhost_net_get_max_queues(VHostNetState *net) return net->dev.max_queues; } +int vhost_net_set_queue_num(NetClientState *nc, uint64_t queues) +{ +VHostNetState *net = get_vhost_net(nc); +const VhostOps *vhost_ops = net->dev.vhost_ops; + +if (vhost_ops->vhost_set_queue_num) { +return vhost_ops->vhost_set_queue_num(&net->dev, queues); +} + +return 0; +} + uint64_t vhost_net_get_acked_features(VHostNetState *net) { return net->dev.acked_features; @@ -456,6 +468,11 @@ uint64_t vhost_net_get_max_queues(VHostNetState *net) return 1; } +int vhost_net_set_queue_num(NetClientState *nc, uint64_t queues) +{ +return 0; +} + struct vhost_net *vhost_net_init(VhostNetOptions *options) { error_report("vhost-net support is not compiled in"); diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h index afc1499eb9..39e639d014 100644 --- a/include/net/vhost_net.h +++ b/include/net/vhost_net.h @@ -15,6 +15,7 @@ typedef struct VhostNetOptions { } VhostNetOptions; uint64_t vhost_net_get_max_queues(VHostNetState *net); +int vhost_net_set_queue_num(NetClientState *nc, uint64_t queues); struct vhost_net *vhost_net_init(VhostNetOptions *options); int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, int total_queues); -- 2.14.3
Re: [Qemu-devel] [PATCH] vhost: fix corrupting GPA 0 when using uninitialized queues
On 01/12/2018 11:13 AM, Zheng Xiang wrote: When guest driver only setup part of queues declared in QEMU, it would corrupt guest's physical address 0 when using uninitialized queues in vhost_virtqueue_start. In AARCH64 virtual machines, the address of system memory starts at 0x4000 and the address of rom starts at 0. So, when using qemu with vhost-scsi, it will fail with below error: qemu-kvm: Error start vhost dev qemu-kvm: unable to start vhost-scsi: Cannot allocate memory This patch fix this issue by skipping calling vhost_virtqueue_start for uninitialized queues. Cc: Michael S. Tsirkin Signed-off-by: Zheng Xiang --- hw/virtio/vhost.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index e4290ce..ac79ffd 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1532,6 +1532,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) goto fail_mem; } for (i = 0; i < hdev->nvqs; ++i) { +if (virtio_queue_get_desc_addr(vdev, hdev->vq_index + i) == 0) +continue; r = vhost_virtqueue_start(hdev, vdev, hdev->vqs + i, Thanks, it fixes the silent corruption that happens with vhost-user net backend and Windows guests, when the number of queues pairs declared in Qemu is higher than the number of vcpus. Tested-by: Maxime Coquelin Maxime
Re: [Qemu-devel] [PATCH v3 0/5] vhost-user-test: Fixes & code clean-up
On 12/21/2017 11:49 PM, Michael S. Tsirkin wrote: On Thu, Dec 21, 2017 at 10:21:20PM +0100, Maxime Coquelin wrote: Sorry, I missed to fixup the feature mask in patch 4, this is the only change in this v3. The two changes in v2 are fixing the features mask before the function rework, and reword of commit message of patch "vhost-user-test: extract read-guest-mem test from main loop" (Thanks Marc-André). This series fixes two issues in vhost-user-test: 1. Setup virtqueues in all tests 2. Fix features mask for all but test_multiqueue() The clean-ups comprises making read-guest-mem test consistent with other tests by initializing the device in the qtest thread. Also, some code factorization is done with regard to device initialization so that all tests share the same init. Looks like mingw tests fail with this. No, the build issue with mingw is due to a regression in master. Laurent posted a patch to fix it: [PATCH] hw/i386/vmport: fix missing definitions with x86_64-w64-mingw32 Message-Id: <20171221211103.30311-1-laur...@vivier.eu> Maxime Maxime Coquelin (5): vhost-user-test: fix features mask vhost-user-test: extract read-guest-mem test from main loop vhost-user-test: setup virtqueues in all tests vhost-user-test: make features mask an init_virtio_dev() argument vhost-user-test: use init_virtio_dev in multiqueue test tests/vhost-user-test.c | 171 ++-- 1 file changed, 79 insertions(+), 92 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH v3 4/5] vhost-user-test: make features mask an init_virtio_dev() argument
The goal is to generalize the use of [un]init_virtio_dev() to all tests, which does not necessarily expose the same features set. Signed-off-by: Maxime Coquelin --- tests/vhost-user-test.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 969e3932cc..6a144e8d7e 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -164,7 +164,7 @@ typedef struct TestServer { static const char *tmpfs; static const char *root; -static void init_virtio_dev(TestServer *s) +static void init_virtio_dev(TestServer *s, uint32_t features_mask) { uint32_t features; int i; @@ -187,7 +187,7 @@ static void init_virtio_dev(TestServer *s) } features = qvirtio_get_features(&s->dev->vdev); -features = features & (1u << VIRTIO_NET_F_MAC); +features = features & features_mask; qvirtio_set_features(&s->dev->vdev, features); qvirtio_set_driver_ok(&s->dev->vdev); @@ -652,7 +652,7 @@ static void test_read_guest_mem(void) s = qtest_start(qemu_cmd); g_free(qemu_cmd); -init_virtio_dev(server); +init_virtio_dev(server, 1u << VIRTIO_NET_F_MAC); read_guest_mem(server); @@ -681,7 +681,7 @@ static void test_migrate(void) from = qtest_start(cmd); g_free(cmd); -init_virtio_dev(s); +init_virtio_dev(s, 1u << VIRTIO_NET_F_MAC); wait_for_fds(s); size = get_log_size(s); g_assert_cmpint(size, ==, (2 * 1024 * 1024) / (VHOST_LOG_PAGE * 8)); @@ -803,7 +803,7 @@ static void test_reconnect_subprocess(void) qtest_start(cmd); g_free(cmd); -init_virtio_dev(s); +init_virtio_dev(s, 1u << VIRTIO_NET_F_MAC); wait_for_fds(s); wait_for_rings_started(s, 2); @@ -841,7 +841,7 @@ static void test_connect_fail_subprocess(void) qtest_start(cmd); g_free(cmd); -init_virtio_dev(s); +init_virtio_dev(s, 1u << VIRTIO_NET_F_MAC); wait_for_fds(s); wait_for_rings_started(s, 2); @@ -871,7 +871,7 @@ static void test_flags_mismatch_subprocess(void) qtest_start(cmd); g_free(cmd); -init_virtio_dev(s); +init_virtio_dev(s, 1u << VIRTIO_NET_F_MAC); wait_for_fds(s); wait_for_rings_started(s, 2); -- 2.14.3
[Qemu-devel] [PATCH v3 3/5] vhost-user-test: setup virtqueues in all tests
Only the multiqueue test setups the virtqueues. This patch generalizes the setup of virtqueues for all tests. Signed-off-by: Maxime Coquelin --- tests/vhost-user-test.c | 53 +++-- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index df567248ae..969e3932cc 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -55,6 +55,7 @@ /*** FROM hw/virtio/vhost-user.c */ #define VHOST_MEMORY_MAX_NREGIONS8 +#define VHOST_MAX_VIRTQUEUES0x100 #define VHOST_USER_F_PROTOCOL_FEATURES 30 #define VHOST_USER_PROTOCOL_F_MQ 0 @@ -141,6 +142,8 @@ enum { typedef struct TestServer { QPCIBus *bus; +QVirtioPCIDevice *dev; +QVirtQueue *vq[VHOST_MAX_VIRTQUEUES]; gchar *socket_path; gchar *mig_path; gchar *chr_name; @@ -155,6 +158,7 @@ typedef struct TestServer { bool test_fail; int test_flags; int queues; +QGuestAllocator *alloc; } TestServer; static const char *tmpfs; @@ -162,26 +166,43 @@ static const char *root; static void init_virtio_dev(TestServer *s) { -QVirtioPCIDevice *dev; uint32_t features; +int i; s->bus = qpci_init_pc(NULL); g_assert_nonnull(s->bus); -dev = qvirtio_pci_device_find(s->bus, VIRTIO_ID_NET); -g_assert_nonnull(dev); +s->dev = qvirtio_pci_device_find(s->bus, VIRTIO_ID_NET); +g_assert_nonnull(s->dev); -qvirtio_pci_device_enable(dev); -qvirtio_reset(&dev->vdev); -qvirtio_set_acknowledge(&dev->vdev); -qvirtio_set_driver(&dev->vdev); +qvirtio_pci_device_enable(s->dev); +qvirtio_reset(&s->dev->vdev); +qvirtio_set_acknowledge(&s->dev->vdev); +qvirtio_set_driver(&s->dev->vdev); + +s->alloc = pc_alloc_init(); -features = qvirtio_get_features(&dev->vdev); +for (i = 0; i < s->queues * 2; i++) { +s->vq[i] = qvirtqueue_setup(&s->dev->vdev, s->alloc, i); +} + +features = qvirtio_get_features(&s->dev->vdev); features = features & (1u << VIRTIO_NET_F_MAC); -qvirtio_set_features(&dev->vdev, features); +qvirtio_set_features(&s->dev->vdev, features); -qvirtio_set_driver_ok(&dev->vdev); -qvirtio_pci_device_free(dev); +qvirtio_set_driver_ok(&s->dev->vdev); +} + +static void uninit_virtio_dev(TestServer *s) +{ +int i; + +for (i = 0; i < s->queues * 2; i++) { +qvirtqueue_cleanup(s->dev->vdev.bus, s->vq[i], s->alloc); +} +pc_alloc_uninit(s->alloc); + +qvirtio_pci_device_free(s->dev); } static void wait_for_fds(TestServer *s) @@ -635,6 +656,8 @@ static void test_read_guest_mem(void) read_guest_mem(server); +uninit_virtio_dev(server); + qtest_quit(s); test_server_free(server); } @@ -711,6 +734,8 @@ static void test_migrate(void) read_guest_mem(dest); +uninit_virtio_dev(s); + g_source_destroy(source); g_source_unref(source); @@ -789,6 +814,8 @@ static void test_reconnect_subprocess(void) wait_for_fds(s); wait_for_rings_started(s, 2); +uninit_virtio_dev(s); + qtest_end(); test_server_free(s); return; @@ -818,6 +845,8 @@ static void test_connect_fail_subprocess(void) wait_for_fds(s); wait_for_rings_started(s, 2); +uninit_virtio_dev(s); + qtest_end(); test_server_free(s); } @@ -846,6 +875,8 @@ static void test_flags_mismatch_subprocess(void) wait_for_fds(s); wait_for_rings_started(s, 2); +uninit_virtio_dev(s); + qtest_end(); test_server_free(s); } -- 2.14.3
[Qemu-devel] [PATCH v3 5/5] vhost-user-test: use init_virtio_dev in multiqueue test
Now that init_virtio_dev() has been generalized to all cases, use it in test_multiqueue() to avoid code duplication. Signed-off-by: Maxime Coquelin --- tests/vhost-user-test.c | 65 ++--- 1 file changed, 8 insertions(+), 57 deletions(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 6a144e8d7e..ec6ac9dc9e 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -892,79 +892,30 @@ static void test_flags_mismatch(void) #endif -static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot) -{ -QVirtioPCIDevice *dev; - -dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET); -g_assert(dev != NULL); -g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_NET); - -qvirtio_pci_device_enable(dev); -qvirtio_reset(&dev->vdev); -qvirtio_set_acknowledge(&dev->vdev); -qvirtio_set_driver(&dev->vdev); - -return dev; -} - -static void driver_init(QVirtioDevice *dev) -{ -uint32_t features; - -features = qvirtio_get_features(dev); -features = features & ~(QVIRTIO_F_BAD_FEATURE | -(1u << VIRTIO_RING_F_INDIRECT_DESC) | -(1u << VIRTIO_RING_F_EVENT_IDX)); -qvirtio_set_features(dev, features); - -qvirtio_set_driver_ok(dev); -} - -#define PCI_SLOT0x04 - static void test_multiqueue(void) { -const int queues = 2; TestServer *s = test_server_new("mq"); -QVirtioPCIDevice *dev; -QPCIBus *bus; -QVirtQueuePCI *vq[queues * 2]; -QGuestAllocator *alloc; char *cmd; -int i; - -s->queues = queues; +uint32_t features_mask = ~(QVIRTIO_F_BAD_FEATURE | +(1u << VIRTIO_RING_F_INDIRECT_DESC) | +(1u << VIRTIO_RING_F_EVENT_IDX)); +s->queues = 2; test_server_listen(s); cmd = g_strdup_printf(QEMU_CMD_MEM QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", 512, 512, root, s->chr_name, s->socket_path, "", s->chr_name, - queues, queues * 2 + 2); + s->queues, s->queues * 2 + 2); qtest_start(cmd); g_free(cmd); -bus = qpci_init_pc(NULL); -dev = virtio_net_pci_init(bus, PCI_SLOT); +init_virtio_dev(s, features_mask); -alloc = pc_alloc_init(); -for (i = 0; i < queues * 2; i++) { -vq[i] = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, alloc, i); -} +wait_for_rings_started(s, s->queues * 2); -driver_init(&dev->vdev); -wait_for_rings_started(s, queues * 2); +uninit_virtio_dev(s); -/* End test */ -for (i = 0; i < queues * 2; i++) { -qvirtqueue_cleanup(dev->vdev.bus, &vq[i]->vq, alloc); -} -pc_alloc_uninit(alloc); -qvirtio_pci_device_disable(dev); -g_free(dev->pdev); -g_free(dev); -qpci_free_pc(bus); qtest_end(); test_server_free(s); -- 2.14.3
[Qemu-devel] [PATCH v3 2/5] vhost-user-test: extract read-guest-mem test from main loop
This patch makes read-guest-test consistent with other tests, i.e. create the test server in the test function. Reviewed-by: Marc-André Lureau Signed-off-by: Maxime Coquelin --- tests/vhost-user-test.c | 41 +++-- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 43c6528644..df567248ae 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -617,6 +617,28 @@ GSourceFuncs test_migrate_source_funcs = { .check = test_migrate_source_check, }; +static void test_read_guest_mem(void) +{ +TestServer *server = NULL; +char *qemu_cmd = NULL; +QTestState *s = NULL; + +server = test_server_new("test"); +test_server_listen(server); + +qemu_cmd = GET_QEMU_CMD(server); + +s = qtest_start(qemu_cmd); +g_free(qemu_cmd); + +init_virtio_dev(server); + +read_guest_mem(server); + +qtest_quit(s); +test_server_free(server); +} + static void test_migrate(void) { TestServer *s = test_server_new("src"); @@ -919,10 +941,7 @@ static void test_multiqueue(void) int main(int argc, char **argv) { -QTestState *s = NULL; -TestServer *server = NULL; const char *hugefs; -char *qemu_cmd = NULL; int ret; char template[] = "/tmp/vhost-test-XX"; GMainLoop *loop; @@ -947,20 +966,11 @@ int main(int argc, char **argv) root = tmpfs; } -server = test_server_new("test"); -test_server_listen(server); - loop = g_main_loop_new(NULL, FALSE); /* run the main loop thread so the chardev may operate */ thread = g_thread_new(NULL, thread_function, loop); -qemu_cmd = GET_QEMU_CMD(server); - -s = qtest_start(qemu_cmd); -g_free(qemu_cmd); -init_virtio_dev(server); - -qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem); +qtest_add_func("/vhost-user/read-guest-mem", test_read_guest_mem); qtest_add_func("/vhost-user/migrate", test_migrate); qtest_add_func("/vhost-user/multiqueue", test_multiqueue); @@ -978,12 +988,7 @@ int main(int argc, char **argv) ret = g_test_run(); -if (s) { -qtest_quit(s); -} - /* cleanup */ -test_server_free(server); /* finish the helper thread and dispatch pending sources */ g_main_loop_quit(loop); -- 2.14.3
[Qemu-devel] [PATCH v3 0/5] vhost-user-test: Fixes & code clean-up
Sorry, I missed to fixup the feature mask in patch 4, this is the only change in this v3. The two changes in v2 are fixing the features mask before the function rework, and reword of commit message of patch "vhost-user-test: extract read-guest-mem test from main loop" (Thanks Marc-André). This series fixes two issues in vhost-user-test: 1. Setup virtqueues in all tests 2. Fix features mask for all but test_multiqueue() The clean-ups comprises making read-guest-mem test consistent with other tests by initializing the device in the qtest thread. Also, some code factorization is done with regard to device initialization so that all tests share the same init. Maxime Coquelin (5): vhost-user-test: fix features mask vhost-user-test: extract read-guest-mem test from main loop vhost-user-test: setup virtqueues in all tests vhost-user-test: make features mask an init_virtio_dev() argument vhost-user-test: use init_virtio_dev in multiqueue test tests/vhost-user-test.c | 171 ++-- 1 file changed, 79 insertions(+), 92 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH v3 1/5] vhost-user-test: fix features mask
VIRTIO_NET_F_MAC is a bit position, not a bit mask. Signed-off-by: Maxime Coquelin --- tests/vhost-user-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index e2c89ed376..43c6528644 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -177,7 +177,7 @@ static void init_virtio_dev(TestServer *s) qvirtio_set_driver(&dev->vdev); features = qvirtio_get_features(&dev->vdev); -features = features & VIRTIO_NET_F_MAC; +features = features & (1u << VIRTIO_NET_F_MAC); qvirtio_set_features(&dev->vdev, features); qvirtio_set_driver_ok(&dev->vdev); -- 2.14.3