Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK

[Eugenio Perez Martin]

On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Fri, Feb 09, 2024 at 02:29:59PM -0800, Steve Sistare wrote:
> > Calling suspend or resume requires VIRTIO_CONFIG_S_DRIVER_OK, for all
> > vdpa devices.
> >
> > Suggested-by: Eugenio Perez Martin <eperezma@xxxxxxxxxx>"
> > Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
>
> I don't think failing suspend or resume makes sense though -
> e.g. practically failing suspend will just prevent sleeping I think -
> why should guest not having driver loaded prevent
> system suspend?
>

In the QEMU case the vhost device has not started, so QEMU should
allow the system suspension.

I haven't tested the QEMU behavior on suspend (not poweroff) with the
guest driver loaded, but I think QEMU should indeed block the
suspension, as there is no way to recover the device after that
without the guest cooperation?

> there's also state such as features set which does need to be
> preserved.
>

That's true if the device does not support resuming. Well, in the
particular case of features, maybe we need to keep it, as userspace
could call VHOST_GET_FEATURES. But maybe we can clean some things,
right.

> I think the thing to do is to skip invoking suspend/resume callback, and in
> fact checking suspend/resume altogether.
>

I don't follow this. What should be done in this cases by QEMU?
1) The device does not support suspend
2) The device support suspend but not resume

In my opinion 1) should be forbidden, as we don't support to resume
the device properly, and 2) can be allowed by fetching all the state.

Thanks!

> > ---
> >  drivers/vhost/vdpa.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index bc4a51e4638b..ce1882acfc3b 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -598,6 +598,9 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> >       if (!ops->suspend)
> >               return -EOPNOTSUPP;
> >
> > +     if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
> > +             return -EINVAL;
> > +
> >       ret = ops->suspend(vdpa);
> >       if (!ret)
> >               v->suspended = true;
> > @@ -618,6 +621,9 @@ static long vhost_vdpa_resume(struct vhost_vdpa *v)
> >       if (!ops->resume)
> >               return -EOPNOTSUPP;
> >
> > +     if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
> > +             return -EINVAL;
> > +
> >       ret = ops->resume(vdpa);
> >       if (!ret)
> >               v->suspended = false;
> > --
> > 2.39.3
>
>