diff mbox series

[V1] vdpa: suspend and resume require DRIVER_OK

Message ID 1707517799-137286-1-git-send-email-steven.sistare@oracle.com
State New
Headers
Series [V1] vdpa: suspend and resume require DRIVER_OK |

Commit Message

Steven Sistare Feb. 9, 2024, 10:29 p.m. UTC 
  Calling suspend or resume requires VIRTIO_CONFIG_S_DRIVER_OK, for all
vdpa devices.

Suggested-by: Eugenio Perez Martin <eperezma@redhat.com>"
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vhost/vdpa.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Michael S. Tsirkin Feb. 12, 2024, 8:19 a.m. UTC | #1 
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@redhat.com>"
> Signed-off-by: Steve Sistare <steven.sistare@oracle.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?

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
Steven Sistare Feb. 12, 2024, 2:56 p.m. UTC | #2 
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 <eperezma@redhat.com>"
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.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?

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
>
Michael S. Tsirkin Feb. 12, 2024, 3:56 p.m. UTC | #3 
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 <eperezma@redhat.com>"
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.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?
> 
> 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
> >
Steven Sistare Feb. 12, 2024, 4:37 p.m. UTC | #4 
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 <eperezma@redhat.com>"
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.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?
>>
>> 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
>>>
>
Michael S. Tsirkin Feb. 13, 2024, 12:06 a.m. UTC | #5 
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 <eperezma@redhat.com>"
> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.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?
> >>
> >> 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
> >>>
> >
Eugenio Perez Martin Feb. 13, 2024, 7:49 a.m. UTC | #6 
On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin <mst@redhat.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@redhat.com>"
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.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
>
>
Eugenio Perez Martin Feb. 13, 2024, 8:08 a.m. UTC | #7 
On Tue, Feb 13, 2024 at 8:49 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Feb 12, 2024 at 9:20 AM Michael S. Tsirkin <mst@redhat.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@redhat.com>"
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.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.
>

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
> >
> >
diff mbox series

Patch

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;