Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On Tue, Feb 13, 2024 at 8:49 AM Eugenio Perez Martin wrote: > > On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin 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 " > > > Signed-off-by: Steve Sistare > > > > 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. > Ok I missed the whole other thread, everything is clear now. Thanks! > 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 > > > >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin 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 " > > Signed-off-by: Steve Sistare > > 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 > >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On Mon, Feb 12, 2024 at 11:37:12AM -0500, Steven Sistare wrote: > On 2/12/2024 10:56 AM, Michael S. Tsirkin wrote: > > On Mon, Feb 12, 2024 at 09:56:31AM -0500, Steven Sistare wrote: > >> On 2/12/2024 3:19 AM, Michael S. Tsirkin 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 " > Signed-off-by: Steve Sistare > >>> > >>> 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? > >> > >> Got it, my fix is too heavy handed. > >> > >>> there's also state such as features set which does need to be > >>> preserved. > >>> > >>> I think the thing to do is to skip invoking suspend/resume callback > >> > >> OK. > >> > >>> and in > >>> fact checking suspend/resume altogether. > >> > >> Currently ops->suspend, vhost_vdpa_can_suspend(), and > >> VHOST_BACKEND_F_SUSPEND > >> are equivalent. Hence if !ops->suspend, then then the driver does not > >> support > >> it, and indeed may break if suspend is used, so system suspend must be > >> blocked, > >> AFAICT. Yielding: > > > > If DRIVER_OK is not set then there's nothing to be done for migration. > > So callback not needed. > > OK, I missed your point. Next attempt: > >vhost_vdpa_suspend() >if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) >return 0; > >if (!ops->suspend) >return -EOPNOTSUPP; right > - Steve > >> vhost_vdpa_suspend() > >> if (!ops->suspend) > >> return -EOPNOTSUPP; > >> > >> if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) > >> return 0; > >> > >> - Steve > >> > --- > 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 > >>> > >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On 2/12/2024 10:56 AM, Michael S. Tsirkin wrote: > On Mon, Feb 12, 2024 at 09:56:31AM -0500, Steven Sistare wrote: >> On 2/12/2024 3:19 AM, Michael S. Tsirkin 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 " Signed-off-by: Steve Sistare >>> >>> 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? >> >> Got it, my fix is too heavy handed. >> >>> there's also state such as features set which does need to be >>> preserved. >>> >>> I think the thing to do is to skip invoking suspend/resume callback >> >> OK. >> >>> and in >>> fact checking suspend/resume altogether. >> >> Currently ops->suspend, vhost_vdpa_can_suspend(), and VHOST_BACKEND_F_SUSPEND >> are equivalent. Hence if !ops->suspend, then then the driver does not >> support >> it, and indeed may break if suspend is used, so system suspend must be >> blocked, >> AFAICT. Yielding: > > If DRIVER_OK is not set then there's nothing to be done for migration. > So callback not needed. OK, I missed your point. Next attempt: vhost_vdpa_suspend() if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) return 0; if (!ops->suspend) return -EOPNOTSUPP; - Steve >> vhost_vdpa_suspend() >> if (!ops->suspend) >> return -EOPNOTSUPP; >> >> if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) >> return 0; >> >> - Steve >> --- 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 >>> >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On Mon, Feb 12, 2024 at 09:56:31AM -0500, Steven Sistare wrote: > On 2/12/2024 3:19 AM, Michael S. Tsirkin 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 " > >> Signed-off-by: Steve Sistare > > > > 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? > > Got it, my fix is too heavy handed. > > > there's also state such as features set which does need to be > > preserved. > > > > I think the thing to do is to skip invoking suspend/resume callback > > OK. > > > and in > > fact checking suspend/resume altogether. > > Currently ops->suspend, vhost_vdpa_can_suspend(), and VHOST_BACKEND_F_SUSPEND > are equivalent. Hence if !ops->suspend, then then the driver does not support > it, and indeed may break if suspend is used, so system suspend must be > blocked, > AFAICT. Yielding: If DRIVER_OK is not set then there's nothing to be done for migration. So callback not needed. > vhost_vdpa_suspend() > if (!ops->suspend) > return -EOPNOTSUPP; > > if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) > return 0; > > - Steve > > >> --- > >> 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 > >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
On 2/12/2024 3:19 AM, Michael S. Tsirkin 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 " >> Signed-off-by: Steve Sistare > > 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? Got it, my fix is too heavy handed. > there's also state such as features set which does need to be > preserved. > > I think the thing to do is to skip invoking suspend/resume callback OK. > and in > fact checking suspend/resume altogether. Currently ops->suspend, vhost_vdpa_can_suspend(), and VHOST_BACKEND_F_SUSPEND are equivalent. Hence if !ops->suspend, then then the driver does not support it, and indeed may break if suspend is used, so system suspend must be blocked, AFAICT. Yielding: vhost_vdpa_suspend() if (!ops->suspend) return -EOPNOTSUPP; if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK)) return 0; - Steve >> --- >> 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 >
Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
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 " > Signed-off-by: Steve Sistare 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? there's also state such as features set which does need to be preserved. I think the thing to do is to skip invoking suspend/resume callback, and in fact checking suspend/resume altogether. > --- > 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