? 2023/2/21 13:27, Jason Wang ??:>
> ? 2023/2/8 17:42, 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.
>
>
> It would be better not introduce a bug first and fix it in the
> following patch.
>
>
>>
>> Signed-off-by: Eugenio P?rez <eperezma at redhat.com>
>> ---
>> ? hw/virtio/vhost-vdpa.c | 19 +++++++++++++++++++
>> ? hw/virtio/trace-events |? 1 +
>> ? 2 files changed, 20 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 2e79fbe4b2..cbbe92ffe8 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -1108,6 +1108,24 @@ 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) ||
>
>
> Any reason we need to use vhost_vdpa_first_dev() instead of replacing the
>
> if (started) {
> } else {
> ??? vhost_vdpa_reset_device(dev);
> ??? ....
> }
Ok, I think I kind of understand, so I think we need re-order the
patches, at least patch 4 should come before this patch?
Thanks
>
>
> We check
>
> if (dev->vq_index + dev->nvqs != dev->vq_index_end) in
> vhost_vdpa_dev_start() but vhost_vdpa_first_dev() inside
> vhost_vdpa_suspend(). This will result code that is hard to maintain.
>
> Thanks
>
>
>> +??????? !(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
{
>> +??????? return;
>> +??? }
>> +
>> +??? 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);
>> +??????? /* Not aborting since we're called from stop context */
>> +??? }
>> +}
>> +
>> ? static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> ? {
>> ????? struct vhost_vdpa *v = dev->opaque;
>> @@ -1122,6 +1140,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