Re: [PATCH v2 03/13] vdpa: add vhost_vdpa_suspend

[Jason Wang <jasowang@redhat.com>]

在 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@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