| Message ID | 20210209153757.1653598-7-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA shadow virtqueue - notifications forwarding | expand |
On Tue, Feb 09, 2021 at 04:37:56PM +0100, Eugenio Pérez wrote: > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > index ac963bf23d..884818b109 100644 > --- a/include/hw/virtio/vhost.h > +++ b/include/hw/virtio/vhost.h > @@ -55,6 +55,8 @@ struct vhost_iommu { > QLIST_ENTRY(vhost_iommu) iommu_next; > }; > > +typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; There is already another declaration in hw/virtio/vhost-shadow-virtqueue.h. Should vhost.h include vhost-shadow-virtqueue.h? This is becoming confusing: 1. typedef in vhost-shadow-virtqueue.h 2. typedef in vhost.h 3. typedef in vhost-shadow-virtqueue.c 3 typedefs is a bit much :). I suggest: 1. typedef in vhost-shadow-virtqueue.h 2. #include "vhost-shadow-virtqueue.h" in vhost.h 3. struct VhostShadowVirtqueue (no typedef redefinition) in vhost-shadow-virtqueue.c That should make the code easier to understand, navigate with tools, and if a change is made (e.g. renaming the struct) then it won't be necessary to change things in 3 places. > + > typedef struct VhostDevConfigOps { > /* Vhost device config space changed callback > */ > @@ -83,7 +85,9 @@ struct vhost_dev { > uint64_t backend_cap; > bool started; > bool log_enabled; > + bool sw_lm_enabled; Rename to shadow_vqs_enabled? > uint64_t log_size; > + VhostShadowVirtqueue **shadow_vqs; > Error *migration_blocker; > const VhostOps *vhost_ops; > void *opaque; > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index b5d2645ae0..01f282d434 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -8,9 +8,12 @@ > */ > > #include "hw/virtio/vhost-shadow-virtqueue.h" > +#include "hw/virtio/vhost.h" > + > +#include "standard-headers/linux/vhost_types.h" > > #include "qemu/error-report.h" > -#include "qemu/event_notifier.h" > +#include "qemu/main-loop.h" > > /* Shadow virtqueue to relay notifications */ > typedef struct VhostShadowVirtqueue { > @@ -18,8 +21,95 @@ typedef struct VhostShadowVirtqueue { > EventNotifier kick_notifier; > /* Shadow call notifier, sent to vhost */ > EventNotifier call_notifier; > + > + /* Borrowed virtqueue's guest to host notifier. */ > + EventNotifier host_notifier; The purpose of these EventNotifier fields is not completely clear to me. Here is how I interpret the comments: 1. The vhost device is set up to use kick_notifier/call_notifier when the shadow vq is enabled. 2. host_notifier is the guest-visible vq's host notifier. This is set up when the shadow vq is enabled. But I'm not confident this is correct. Maybe you could expand the comment to make it clear what is happening? > + > + /* Virtio queue shadowing */ > + VirtQueue *vq; > } VhostShadowVirtqueue; > > +/* Forward guest notifications */ > +static void vhost_handle_guest_kick(EventNotifier *n) > +{ > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > + host_notifier); > + > + if (event_notifier_test_and_clear(n)) { > + event_notifier_set(&svq->kick_notifier); > + } > +} This function looks incomplete. You can make review easier by indicating the state of the code: /* TODO pop requests from vq and put them onto vhost vq */ I'm not sure why it's useful to include this incomplete function in the patch. Maybe the host notifier is already intercepted by the guest-visible vq is still mapped directly to the vhost vq so this works? An explanation in comments or the commit description would be helpful. > + > +/* > + * Start shadow virtqueue operation. > + * @dev vhost device > + * @hidx vhost virtqueue index > + * @svq Shadow Virtqueue > + * > + * Run in RCU context > + */ > +bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > + unsigned idx, > + VhostShadowVirtqueue *svq) > +{ > + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); > + struct vhost_vring_file kick_file = { > + .index = idx, > + .fd = event_notifier_get_fd(&svq->kick_notifier), > + }; > + int r; > + > + /* Check that notifications are still going directly to vhost dev */ > + assert(virtio_queue_host_notifier_status(svq->vq)); > + > + event_notifier_init_fd(&svq->host_notifier, > + event_notifier_get_fd(vq_host_notifier)); > + event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); If I understand correctly svq->host_notifier only exists as an easy way to use container_of() in vhost_handle_guest_kick? svq->host_notifier does not actually own the fd and therefore event_notifier_cleanup() must never be called on it? Please document this. > + > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > + if (unlikely(r != 0)) { > + error_report("Couldn't set kick fd: %s", strerror(errno)); > + goto err_set_vring_kick; > + } > + > + /* Check for pending notifications from the guest */ > + vhost_handle_guest_kick(&svq->host_notifier); > + > + return true; host_notifier is still registered with the vhost device so now the kernel vhost thread and QEMU are both monitoring the ioeventfd at the same time? Did I miss a vhost_set_vring_call() somewhere? > + > +err_set_vring_kick: > + event_notifier_set_handler(&svq->host_notifier, NULL); > + > + return false; > +} > + > +/* > + * Stop shadow virtqueue operation. > + * @dev vhost device > + * @idx vhost queue index > + * @svq Shadow Virtqueue > + * > + * Run in RCU context > + */ > +void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, > + unsigned idx, > + VhostShadowVirtqueue *svq) > +{ > + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); > + struct vhost_vring_file kick_file = { > + .index = idx, > + .fd = event_notifier_get_fd(vq_host_notifier), > + }; > + int r; > + > + /* Restore vhost kick */ > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > + /* Cannot do a lot of things */ > + assert(r == 0); > + > + event_notifier_set_handler(&svq->host_notifier, NULL); It may be necessary to call event_notifier_set(vq_host_notifier) before vhost_set_vring_kick() so that the vhost kernel thread looks at the vring immediately. That covers the case where svq->kick_notifier was just set but not yet handled by the vhost kernel thread. I'm not 100% sure this race condition can occur, but couldn't find anything that prevents it. > +err: > + for (; idx >= 0; --idx) { > + vhost_shadow_vq_free(dev->shadow_vqs[idx]); > + } > + g_free(dev->shadow_vqs[idx]); Should this be g_free(dev->shadow_vqs)?
On Wed, Feb 17, 2021 at 5:56 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Tue, Feb 09, 2021 at 04:37:56PM +0100, Eugenio Pérez wrote: > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > > index ac963bf23d..884818b109 100644 > > --- a/include/hw/virtio/vhost.h > > +++ b/include/hw/virtio/vhost.h > > @@ -55,6 +55,8 @@ struct vhost_iommu { > > QLIST_ENTRY(vhost_iommu) iommu_next; > > }; > > > > +typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; > > There is already another declaration in > hw/virtio/vhost-shadow-virtqueue.h. Should vhost.h include > vhost-shadow-virtqueue.h? > > This is becoming confusing: > 1. typedef in vhost-shadow-virtqueue.h > 2. typedef in vhost.h > 3. typedef in vhost-shadow-virtqueue.c > > 3 typedefs is a bit much :). I suggest: > 1. typedef in vhost-shadow-virtqueue.h > 2. #include "vhost-shadow-virtqueue.h" in vhost.h > 3. struct VhostShadowVirtqueue (no typedef redefinition) in vhost-shadow-virtqueue.c > > That should make the code easier to understand, navigate with tools, and > if a change is made (e.g. renaming the struct) then it won't be > necessary to change things in 3 places. > Totally agree. > > + > > typedef struct VhostDevConfigOps { > > /* Vhost device config space changed callback > > */ > > @@ -83,7 +85,9 @@ struct vhost_dev { > > uint64_t backend_cap; > > bool started; > > bool log_enabled; > > + bool sw_lm_enabled; > > Rename to shadow_vqs_enabled? > Ok, will change. > > uint64_t log_size; > > + VhostShadowVirtqueue **shadow_vqs; > > Error *migration_blocker; > > const VhostOps *vhost_ops; > > void *opaque; > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index b5d2645ae0..01f282d434 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -8,9 +8,12 @@ > > */ > > > > #include "hw/virtio/vhost-shadow-virtqueue.h" > > +#include "hw/virtio/vhost.h" > > + > > +#include "standard-headers/linux/vhost_types.h" > > > > #include "qemu/error-report.h" > > -#include "qemu/event_notifier.h" > > +#include "qemu/main-loop.h" > > > > /* Shadow virtqueue to relay notifications */ > > typedef struct VhostShadowVirtqueue { > > @@ -18,8 +21,95 @@ typedef struct VhostShadowVirtqueue { > > EventNotifier kick_notifier; > > /* Shadow call notifier, sent to vhost */ > > EventNotifier call_notifier; > > + > > + /* Borrowed virtqueue's guest to host notifier. */ > > + EventNotifier host_notifier; > > The purpose of these EventNotifier fields is not completely clear to me. > Here is how I interpret the comments: > > 1. The vhost device is set up to use kick_notifier/call_notifier when > the shadow vq is enabled. > Right. These are the ones the vhost backend sees, so I will add this to the doc. > 2. host_notifier is the guest-visible vq's host notifier. This is set up > when the shadow vq is enabled. > If by guest-visible you mean that the guest can notify the device though it, yes. Names are used as in qemu virtqueue device code. As you point later in the code, is borrowed (and not just referenced through a pointer) to reduce qemu VirtIODevice dependency. > But I'm not confident this is correct. Maybe you could expand the > comment to make it clear what is happening? > > > + > > + /* Virtio queue shadowing */ > > + VirtQueue *vq; > > } VhostShadowVirtqueue; > > > > +/* Forward guest notifications */ > > +static void vhost_handle_guest_kick(EventNotifier *n) > > +{ > > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > + host_notifier); > > + > > + if (event_notifier_test_and_clear(n)) { > > + event_notifier_set(&svq->kick_notifier); > > + } > > +} > > This function looks incomplete. You can make review easier by indicating > the state of the code: > > /* TODO pop requests from vq and put them onto vhost vq */ > Only guest -> device notifications are forwarded at this point, buffers aren't. The vhost backend/device still needs to access the latter the regular way, but guest -> device notifications now make an extra jump. > I'm not sure why it's useful to include this incomplete function in the > patch. It has little use at this moment except logging that notifications are being forwarded through qemu. Once notifications are being forwarded properly, we can develop buffer treatment on top of this. In my debugging it helps me to bisect the problem if the network loses connectivity when I enable shadow virtqueue: If it works in this commit, but not in 7/7 ("vhost: Route host->guest notification through shadow virtqueue"), I know that is the latter commit fault. If it works on 7/7, but it doesn't work on commits that exchange/translates buffers through qemu [1], I know host->guest notifications are being forwarded OK and the problem is elsewhere. > Maybe the host notifier is already intercepted by the > guest-visible vq is still mapped directly to the vhost vq so this works? > An explanation in comments or the commit description would be helpful. > I'm not sure what you mean in the first part, but descriptors and buffers are still mapped in vhost device, yes. > > + > > +/* > > + * Start shadow virtqueue operation. > > + * @dev vhost device > > + * @hidx vhost virtqueue index > > + * @svq Shadow Virtqueue > > + * > > + * Run in RCU context > > + */ > > +bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > + unsigned idx, > > + VhostShadowVirtqueue *svq) > > +{ > > + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); > > + struct vhost_vring_file kick_file = { > > + .index = idx, > > + .fd = event_notifier_get_fd(&svq->kick_notifier), > > + }; > > + int r; > > + > > + /* Check that notifications are still going directly to vhost dev */ > > + assert(virtio_queue_host_notifier_status(svq->vq)); > > + > > + event_notifier_init_fd(&svq->host_notifier, > > + event_notifier_get_fd(vq_host_notifier)); > > + event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); > > If I understand correctly svq->host_notifier only exists as an easy way > to use container_of() in vhost_handle_guest_kick? > > svq->host_notifier does not actually own the fd and therefore > event_notifier_cleanup() must never be called on it? > > Please document this. > Right, will document better. > > + > > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > > + if (unlikely(r != 0)) { > > + error_report("Couldn't set kick fd: %s", strerror(errno)); > > + goto err_set_vring_kick; > > + } > > + > > + /* Check for pending notifications from the guest */ > > + vhost_handle_guest_kick(&svq->host_notifier); > > + > > + return true; > (> host_notifier is still registered with the vhost device so now the > kernel vhost thread and QEMU are both monitoring the ioeventfd at the > same time? The vhost device is not monitoring host_notifier, since we replaced it with previous vhost_set_vring_kick(dev, &kick_file). > Did I miss a vhost_set_vring_call() somewhere? > It is in the next commit. This one just intercepts guest->device notifications. > > + > > +err_set_vring_kick: > > + event_notifier_set_handler(&svq->host_notifier, NULL); > > + > > + return false; > > +} > > + > > +/* > > + * Stop shadow virtqueue operation. > > + * @dev vhost device > > + * @idx vhost queue index > > + * @svq Shadow Virtqueue > > + * > > + * Run in RCU context > > + */ > > +void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, > > + unsigned idx, > > + VhostShadowVirtqueue *svq) > > +{ > > + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); > > + struct vhost_vring_file kick_file = { > > + .index = idx, > > + .fd = event_notifier_get_fd(vq_host_notifier), > > + }; > > + int r; > > + > > + /* Restore vhost kick */ > > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > > + /* Cannot do a lot of things */ > > + assert(r == 0); > > + > > + event_notifier_set_handler(&svq->host_notifier, NULL); > > It may be necessary to call event_notifier_set(vq_host_notifier) before > vhost_set_vring_kick() so that the vhost kernel thread looks at the > vring immediately. That covers the case where svq->kick_notifier was > just set but not yet handled by the vhost kernel thread. > > I'm not 100% sure this race condition can occur, but couldn't find > anything that prevents it. As I can see, we should not lose any notifications as long as vhost kernel thread checks the fd at set vring kick. They are always sent by the guest, and qemu never kicks it. We would have the same problem in vhost_virtqueue_start and qemu doesn't kick in it. However, I'm not sure if we can say that all devices will check it... Should it be mandated by vDPA? > > > +err: > > + for (; idx >= 0; --idx) { > > + vhost_shadow_vq_free(dev->shadow_vqs[idx]); > > + } > > + g_free(dev->shadow_vqs[idx]); > > Should this be g_free(dev->shadow_vqs)? Right, a big miss again. Thank you very much for the useful and in- depth review! [1] Not in this series, but already posted in previous RFC and obviously the final solution will contain them :).
diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h index 6cc18d6acb..c45035c4b7 100644 --- a/hw/virtio/vhost-shadow-virtqueue.h +++ b/hw/virtio/vhost-shadow-virtqueue.h @@ -17,6 +17,13 @@ typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; +bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, + unsigned idx, + VhostShadowVirtqueue *svq); +void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, + unsigned idx, + VhostShadowVirtqueue *svq); + VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx); void vhost_shadow_vq_free(VhostShadowVirtqueue *vq); diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index ac963bf23d..884818b109 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -55,6 +55,8 @@ struct vhost_iommu { QLIST_ENTRY(vhost_iommu) iommu_next; }; +typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; + typedef struct VhostDevConfigOps { /* Vhost device config space changed callback */ @@ -83,7 +85,9 @@ struct vhost_dev { uint64_t backend_cap; bool started; bool log_enabled; + bool sw_lm_enabled; uint64_t log_size; + VhostShadowVirtqueue **shadow_vqs; Error *migration_blocker; const VhostOps *vhost_ops; void *opaque; diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index b5d2645ae0..01f282d434 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -8,9 +8,12 @@ */ #include "hw/virtio/vhost-shadow-virtqueue.h" +#include "hw/virtio/vhost.h" + +#include "standard-headers/linux/vhost_types.h" #include "qemu/error-report.h" -#include "qemu/event_notifier.h" +#include "qemu/main-loop.h" /* Shadow virtqueue to relay notifications */ typedef struct VhostShadowVirtqueue { @@ -18,8 +21,95 @@ typedef struct VhostShadowVirtqueue { EventNotifier kick_notifier; /* Shadow call notifier, sent to vhost */ EventNotifier call_notifier; + + /* Borrowed virtqueue's guest to host notifier. */ + EventNotifier host_notifier; + + /* Virtio queue shadowing */ + VirtQueue *vq; } VhostShadowVirtqueue; +/* Forward guest notifications */ +static void vhost_handle_guest_kick(EventNotifier *n) +{ + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, + host_notifier); + + if (event_notifier_test_and_clear(n)) { + event_notifier_set(&svq->kick_notifier); + } +} + +/* + * Start shadow virtqueue operation. + * @dev vhost device + * @hidx vhost virtqueue index + * @svq Shadow Virtqueue + * + * Run in RCU context + */ +bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, + unsigned idx, + VhostShadowVirtqueue *svq) +{ + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); + struct vhost_vring_file kick_file = { + .index = idx, + .fd = event_notifier_get_fd(&svq->kick_notifier), + }; + int r; + + /* Check that notifications are still going directly to vhost dev */ + assert(virtio_queue_host_notifier_status(svq->vq)); + + event_notifier_init_fd(&svq->host_notifier, + event_notifier_get_fd(vq_host_notifier)); + event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); + + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); + if (unlikely(r != 0)) { + error_report("Couldn't set kick fd: %s", strerror(errno)); + goto err_set_vring_kick; + } + + /* Check for pending notifications from the guest */ + vhost_handle_guest_kick(&svq->host_notifier); + + return true; + +err_set_vring_kick: + event_notifier_set_handler(&svq->host_notifier, NULL); + + return false; +} + +/* + * Stop shadow virtqueue operation. + * @dev vhost device + * @idx vhost queue index + * @svq Shadow Virtqueue + * + * Run in RCU context + */ +void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, + unsigned idx, + VhostShadowVirtqueue *svq) +{ + EventNotifier *vq_host_notifier = virtio_queue_get_host_notifier(svq->vq); + struct vhost_vring_file kick_file = { + .index = idx, + .fd = event_notifier_get_fd(vq_host_notifier), + }; + int r; + + /* Restore vhost kick */ + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); + /* Cannot do a lot of things */ + assert(r == 0); + + event_notifier_set_handler(&svq->host_notifier, NULL); +} + /* * Creates vhost shadow virtqueue, and instruct vhost device to use the shadow * methods and file descriptors. @@ -27,8 +117,11 @@ typedef struct VhostShadowVirtqueue { VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx) { g_autofree VhostShadowVirtqueue *svq = g_new0(VhostShadowVirtqueue, 1); + int vq_idx = dev->vq_index + idx; int r; + svq->vq = virtio_get_queue(dev->vdev, vq_idx); + r = event_notifier_init(&svq->kick_notifier, 0); if (r != 0) { error_report("Couldn't create kick event notifier: %s", @@ -43,7 +136,7 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx) goto err_init_call_notifier; } - return svq; + return g_steal_pointer(&svq); err_init_call_notifier: event_notifier_cleanup(&svq->kick_notifier); diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 8fbf14385e..9d4728e545 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -25,6 +25,7 @@ #include "exec/address-spaces.h" #include "hw/virtio/virtio-bus.h" #include "hw/virtio/virtio-access.h" +#include "hw/virtio/vhost-shadow-virtqueue.h" #include "migration/blocker.h" #include "migration/qemu-file-types.h" #include "sysemu/dma.h" @@ -937,6 +938,84 @@ static void vhost_log_global_stop(MemoryListener *listener) } } +static int vhost_sw_live_migration_stop(struct vhost_dev *dev) +{ + int idx; + + WITH_RCU_READ_LOCK_GUARD() { + dev->sw_lm_enabled = false; + + for (idx = 0; idx < dev->nvqs; ++idx) { + vhost_shadow_vq_stop_rcu(dev, idx, dev->shadow_vqs[idx]); + } + } + + for (idx = 0; idx < dev->nvqs; ++idx) { + vhost_shadow_vq_free(dev->shadow_vqs[idx]); + } + + g_free(dev->shadow_vqs); + dev->shadow_vqs = NULL; + return 0; +} + +static int vhost_sw_live_migration_start(struct vhost_dev *dev) +{ + int idx; + + dev->shadow_vqs = g_new0(VhostShadowVirtqueue *, dev->nvqs); + for (idx = 0; idx < dev->nvqs; ++idx) { + dev->shadow_vqs[idx] = vhost_shadow_vq_new(dev, idx); + if (unlikely(dev->shadow_vqs[idx] == NULL)) { + goto err; + } + } + + WITH_RCU_READ_LOCK_GUARD() { + for (idx = 0; idx < dev->nvqs; ++idx) { + bool ok = vhost_shadow_vq_start_rcu(dev, idx, + dev->shadow_vqs[idx]); + + if (!ok) { + int stop_idx = idx; + + while (--stop_idx >= 0) { + vhost_shadow_vq_stop_rcu(dev, idx, + dev->shadow_vqs[stop_idx]); + } + + goto err; + } + } + } + + dev->sw_lm_enabled = true; + return 0; + +err: + for (; idx >= 0; --idx) { + vhost_shadow_vq_free(dev->shadow_vqs[idx]); + } + g_free(dev->shadow_vqs[idx]); + + return -1; +} + +static int vhost_sw_live_migration_enable(struct vhost_dev *dev, + bool enable_lm) +{ + int r; + + if (enable_lm == dev->sw_lm_enabled) { + return 0; + } + + r = enable_lm ? vhost_sw_live_migration_start(dev) + : vhost_sw_live_migration_stop(dev); + + return r; +} + static void vhost_log_start(MemoryListener *listener, MemoryRegionSection *section, int old, int new) @@ -1381,6 +1460,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, hdev->log = NULL; hdev->log_size = 0; hdev->log_enabled = false; + hdev->sw_lm_enabled = false; hdev->started = false; memory_listener_register(&hdev->memory_listener, &address_space_memory); QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); @@ -1486,6 +1566,10 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); int i, r; + if (hdev->sw_lm_enabled) { + vhost_sw_live_migration_enable(hdev, false); + } + for (i = 0; i < hdev->nvqs; ++i) { r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, false); @@ -1808,6 +1892,11 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev) hdev->vhost_ops->vhost_dev_start(hdev, false); } for (i = 0; i < hdev->nvqs; ++i) { + if (hdev->sw_lm_enabled) { + vhost_shadow_vq_stop_rcu(hdev, i, hdev->shadow_vqs[i]); + vhost_shadow_vq_free(hdev->shadow_vqs[i]); + } + vhost_virtqueue_stop(hdev, vdev, hdev->vqs + i, @@ -1821,6 +1910,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev) memory_listener_unregister(&hdev->iommu_listener); } vhost_log_put(hdev, true); + g_free(hdev->shadow_vqs); + hdev->sw_lm_enabled = false; hdev->started = false; hdev->vdev = NULL; } @@ -1837,5 +1928,60 @@ int vhost_net_set_backend(struct vhost_dev *hdev, void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error **errp) { - error_setg(errp, "Shadow virtqueue still not implemented"); + struct vhost_dev *hdev, *hdev_err; + VirtIODevice *vdev; + const char *err_cause = NULL; + int r; + ErrorClass err_class = ERROR_CLASS_GENERIC_ERROR; + + QLIST_FOREACH(hdev, &vhost_devices, entry) { + if (hdev->vdev && 0 == strcmp(hdev->vdev->name, name)) { + vdev = hdev->vdev; + break; + } + } + + if (!hdev) { + err_class = ERROR_CLASS_DEVICE_NOT_FOUND; + err_cause = "Device not found"; + goto not_found_err; + } + + for ( ; hdev; hdev = QLIST_NEXT(hdev, entry)) { + if (vdev != hdev->vdev) { + continue; + } + + if (!hdev->started) { + err_cause = "Device is not started"; + goto err; + } + + r = vhost_sw_live_migration_enable(hdev, enable); + if (unlikely(r)) { + err_cause = "Error enabling (see monitor)"; + goto err; + } + } + + return; + +err: + QLIST_FOREACH(hdev_err, &vhost_devices, entry) { + if (hdev_err == hdev) { + break; + } + + if (vdev != hdev->vdev) { + continue; + } + + vhost_sw_live_migration_enable(hdev, !enable); + } + +not_found_err: + if (err_cause) { + error_set(errp, err_class, + "Can't enable shadow vq on %s: %s", name, err_cause); + } }
Shadow virtqueue notifications forwarding is disabled when vhost_dev stops. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-shadow-virtqueue.h | 7 ++ include/hw/virtio/vhost.h | 4 + hw/virtio/vhost-shadow-virtqueue.c | 97 ++++++++++++++++++- hw/virtio/vhost.c | 148 ++++++++++++++++++++++++++++- 4 files changed, 253 insertions(+), 3 deletions(-)