On Thu, Feb 23, 2023 at 3:38 AM Jason Wang <jasow...@redhat.com> wrote:
>
>
> 在 2023/2/22 22:25, Eugenio Perez Martin 写道:
> > On Wed, Feb 22, 2023 at 5:05 AM Jason Wang <jasow...@redhat.com> wrote:
> >>
> >> 在 2023/2/8 17:42, Eugenio Pérez 写道:
> >>> Next patches enable devices to be migrated even if vdpa netdev has not
> >>> been started with x-svq. However, not all devices are migratable, so we
> >>> need to block migration if we detect that.
> >>>
> >>> Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it
> >>> has not been started with x-svq.
> >>>
> >>> Signed-off-by: Eugenio Pérez <epere...@redhat.com>
> >>> ---
> >>> hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++
> >>> 1 file changed, 21 insertions(+)
> >>>
> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>> index 84a6b9690b..9d30cf9b3c 100644
> >>> --- a/hw/virtio/vhost-vdpa.c
> >>> +++ b/hw/virtio/vhost-vdpa.c
> >>> @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev,
> >>> void *opaque, Error **errp)
> >>> return 0;
> >>> }
> >>>
> >>> + /*
> >>> + * If dev->shadow_vqs_enabled at initialization that means the
> >>> device has
> >>> + * been started with x-svq=on, so don't block migration
> >>> + */
> >>> + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) {
> >>> + uint64_t backend_features;
> >>> +
> >>> + /* We don't have dev->backend_features yet */
> >>> + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES,
> >>> + &backend_features);
> >>> + if (unlikely(ret)) {
> >>> + error_setg_errno(errp, -ret, "Could not get backend
> >>> features");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
> >>> + error_setg(&dev->migration_blocker,
> >>> + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND
> >>> feature.");
> >>> + }
> >>
> >> I wonder why not let the device to decide? For networking device, we can
> >> live without suspend probably.
> >>
> > Right, but how can we know if this is a net device in init? I don't
> > think a switch (vhost_vdpa_get_device_id(dev)) is elegant.
>
>
> I meant the caller of vhost_vdpa_init() which is net_init_vhost_vdpa().
>
That's doable but I'm not sure if it is convenient.
Since we're always offering _F_LOG I thought of the lack of _F_SUSPEND
as the default migration blocker for other kinds of devices like blk.
If we move this code to net_init_vhost_vdpa, all other devices are in
charge of block migration by themselves.
I guess the right action is to use a variable similar to
vhost_vdpa->f_log_all. It defaults to false, and the device can choose
if it should export it or not. This way, the device does not migrate
by default, and the equivalent of net_init_vhost_vdpa could choose
whether to offer _F_LOG with SVQ or not.
OTOH I guess other kinds of devices already must place blockers beyond
_F_LOG, so maybe it makes sense to always offer _F_LOG even if
_F_SUSPEND is not offered? Stefano G., would that break vhost-vdpa-blk
support?
Thanks!
> Thanks
>
>
> >
> > If the parent device does not need to be suspended i'd go with
> > exposing a suspend ioctl but do nothing in the parent device. After
> > that, it could even choose to return an error for GET_VRING_BASE.
> >
> > If we want to implement it as a fallback in qemu, I'd go for
> > implementing it on top of this series. There are a few operations we
> > could move to a device-kind specific ops.
> >
> > Would it make sense to you?
> >
> > Thanks!
> >
> >
> >> Thanks
> >>
> >>
> >>> + }
> >>> +
> >>> /*
> >>> * Similar to VFIO, we end up pinning all guest memory and have to
> >>> * disable discarding of RAM.
>
