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

["Michael S. Tsirkin" <mst@redhat.com>]

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
> >>>
> >