| Message ID | 20230224155438.112797-8-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Dynamically switch to vhost shadow virtqueues at vdpa net migration | expand |
在 2023/2/24 23:54, Eugenio Pérez 写道: > The function vhost.c:vhost_dev_stop fetches the vring base so the vq > state can be migrated to other devices. However, this is unreliable in > vdpa, since we didn't signal the device to suspend the queues, making > the value fetched useless. > > Suspend the device if possible before fetching first and subsequent > vring bases. > > Moreover, vdpa totally reset and wipes the device at the last device > before fetch its vrings base, making that operation useless in the last > device. This will be fixed in later patches of this series. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> I suggest to squash this into patch 5 (or even squash patch 6 into this) since it's not good to introduce a bug in 5 and fix in 7. > --- > v4: > * Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features > * Fall back on reset & fetch used idx from guest's memory A hint to squash patch 6. Thanks > --- > hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++++++++ > hw/virtio/trace-events | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 228677895a..f542960a64 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev) > > ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); > trace_vhost_vdpa_reset_device(dev, status); > + v->suspended = false; > return ret; > } > > @@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) > } > } > > +static void vhost_vdpa_suspend(struct vhost_dev *dev) > +{ > + struct vhost_vdpa *v = dev->opaque; > + int r; > + > + if (!vhost_vdpa_first_dev(dev)) { > + return; > + } > + > + if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > + trace_vhost_vdpa_suspend(dev); > + r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND); > + if (unlikely(r)) { > + error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno); > + } else { > + v->suspended = true; > + return; > + } > + } > + > + vhost_vdpa_reset_device(dev); > +} > + > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > { > struct vhost_vdpa *v = dev->opaque; > @@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > } > vhost_vdpa_set_vring_ready(dev); > } else { > + vhost_vdpa_suspend(dev); > vhost_vdpa_svqs_stop(dev); > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > } > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index a87c5f39a2..8f8d05cf9b 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p" > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" > vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 > vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 > +vhost_vdpa_suspend(void *dev) "dev: %p" > vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d" > vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: %llu refcnt: %d fd: %d log: %p" > vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
On 2/24/2023 7:54 AM, Eugenio Pérez wrote: > The function vhost.c:vhost_dev_stop fetches the vring base so the vq > state can be migrated to other devices. However, this is unreliable in > vdpa, since we didn't signal the device to suspend the queues, making > the value fetched useless. > > Suspend the device if possible before fetching first and subsequent > vring bases. > > Moreover, vdpa totally reset and wipes the device at the last device > before fetch its vrings base, making that operation useless in the last > device. This will be fixed in later patches of this series. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > v4: > * Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features > * Fall back on reset & fetch used idx from guest's memory > --- > hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++++++++ > hw/virtio/trace-events | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 228677895a..f542960a64 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev) > > ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); > trace_vhost_vdpa_reset_device(dev, status); > + v->suspended = false; > return ret; > } > > @@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) > } > } > > +static void vhost_vdpa_suspend(struct vhost_dev *dev) > +{ > + struct vhost_vdpa *v = dev->opaque; > + int r; > + > + if (!vhost_vdpa_first_dev(dev)) { > + return; > + } > + > + if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { Polarity reversed. This ends up device getting reset always even if the backend offers _F_SUSPEND. -Siwei > + trace_vhost_vdpa_suspend(dev); > + r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND); > + if (unlikely(r)) { > + error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno); > + } else { > + v->suspended = true; > + return; > + } > + } > + > + vhost_vdpa_reset_device(dev); > +} > + > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > { > struct vhost_vdpa *v = dev->opaque; > @@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > } > vhost_vdpa_set_vring_ready(dev); > } else { > + vhost_vdpa_suspend(dev); > vhost_vdpa_svqs_stop(dev); > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > } > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index a87c5f39a2..8f8d05cf9b 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p" > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" > vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 > vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 > +vhost_vdpa_suspend(void *dev) "dev: %p" > vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d" > vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: %llu refcnt: %d fd: %d log: %p" > vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
On Wed, Mar 1, 2023 at 2:30 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > > > On 2/24/2023 7:54 AM, Eugenio Pérez wrote: > > The function vhost.c:vhost_dev_stop fetches the vring base so the vq > > state can be migrated to other devices. However, this is unreliable in > > vdpa, since we didn't signal the device to suspend the queues, making > > the value fetched useless. > > > > Suspend the device if possible before fetching first and subsequent > > vring bases. > > > > Moreover, vdpa totally reset and wipes the device at the last device > > before fetch its vrings base, making that operation useless in the last > > device. This will be fixed in later patches of this series. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > v4: > > * Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features > > * Fall back on reset & fetch used idx from guest's memory > > --- > > hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++++++++ > > hw/virtio/trace-events | 1 + > > 2 files changed, 26 insertions(+) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 228677895a..f542960a64 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev) > > > > ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); > > trace_vhost_vdpa_reset_device(dev, status); > > + v->suspended = false; > > return ret; > > } > > > > @@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) > > } > > } > > > > +static void vhost_vdpa_suspend(struct vhost_dev *dev) > > +{ > > + struct vhost_vdpa *v = dev->opaque; > > + int r; > > + > > + if (!vhost_vdpa_first_dev(dev)) { > > + return; > > + } > > + > > + if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > Polarity reversed. This ends up device getting reset always even if the > backend offers _F_SUSPEND. > Good catch, I'll fix it in v5. Thanks! > -Siwei > > > + trace_vhost_vdpa_suspend(dev); > > + r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND); > > + if (unlikely(r)) { > > + error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno); > > + } else { > > + v->suspended = true; > > + return; > > + } > > + } > > + > > + vhost_vdpa_reset_device(dev); > > +} > > + > > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > > { > > struct vhost_vdpa *v = dev->opaque; > > @@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > > } > > vhost_vdpa_set_vring_ready(dev); > > } else { > > + vhost_vdpa_suspend(dev); > > vhost_vdpa_svqs_stop(dev); > > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > > } > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > > index a87c5f39a2..8f8d05cf9b 100644 > > --- a/hw/virtio/trace-events > > +++ b/hw/virtio/trace-events > > @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p" > > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" > > vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 > > vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 > > +vhost_vdpa_suspend(void *dev) "dev: %p" > > vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d" > > vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: %llu refcnt: %d fd: %d log: %p" > > vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64 >
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 228677895a..f542960a64 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -712,6 +712,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev) ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); trace_vhost_vdpa_reset_device(dev, status); + v->suspended = false; return ret; } @@ -1109,6 +1110,29 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) } } +static void vhost_vdpa_suspend(struct vhost_dev *dev) +{ + struct vhost_vdpa *v = dev->opaque; + int r; + + if (!vhost_vdpa_first_dev(dev)) { + return; + } + + if (!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { + trace_vhost_vdpa_suspend(dev); + r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND); + if (unlikely(r)) { + error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno); + } else { + v->suspended = true; + return; + } + } + + vhost_vdpa_reset_device(dev); +} + static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) { struct vhost_vdpa *v = dev->opaque; @@ -1123,6 +1147,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) } vhost_vdpa_set_vring_ready(dev); } else { + vhost_vdpa_suspend(dev); vhost_vdpa_svqs_stop(dev); vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); } diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index a87c5f39a2..8f8d05cf9b 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p" vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 +vhost_vdpa_suspend(void *dev) "dev: %p" vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d" vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: %llu refcnt: %d fd: %d log: %p" vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
The function vhost.c:vhost_dev_stop fetches the vring base so the vq state can be migrated to other devices. However, this is unreliable in vdpa, since we didn't signal the device to suspend the queues, making the value fetched useless. Suspend the device if possible before fetching first and subsequent vring bases. Moreover, vdpa totally reset and wipes the device at the last device before fetch its vrings base, making that operation useless in the last device. This will be fixed in later patches of this series. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- v4: * Look for _F_SUSPEND at vhost_dev->backend_cap, not backend_features * Fall back on reset & fetch used idx from guest's memory --- hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++++++++ hw/virtio/trace-events | 1 + 2 files changed, 26 insertions(+)