lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 13 Feb 2024 08:49:21 +0100
From: Eugenio Perez Martin <eperezma@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Steve Sistare <steven.sistare@...cle.com>, 
	virtualization <virtualization@...ts.linux-foundation.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, Jason Wang <jasowang@...hat.com>, 
	Si-Wei Liu <si-wei.liu@...cle.com>, Stefano Garzarella <sgarzare@...hat.com>
Subject: Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK

On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin <mst@...hat.com> 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@...hat.com>"
> > Signed-off-by: Steve Sistare <steven.sistare@...cle.com>
>
> 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
>
>


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ