Move vhost-user SET_STATUS 0 after get vring base?

Move vhost-user SET_STATUS 0 after get vring base?

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

Hi,
Cindy's commit ca71db438bdc ("vhost: implement vhost_dev_start method")
added SET_STATUS calls to vhost_dev_start() and vhost_dev_stop() for all
vhost backends.

Eugenio's commit c3716f260bff ("vdpa: move vhost reset after get vring
base") deferred the SET_STATUS 0 call in vhost_dev_stop() until after
GET_VRING_BASE for vDPA only. In that commit Eugenio said, "A patch to
make vhost_user_dev_start more similar to vdpa is desirable, but it can
be added on top".

I agree and think it's a good idea to keep the vhost backends in sync
where possible.

vhost-user still has the old behavior where QEMU sends SET_STATUS 0
before GET_VRING_BASE. Most existing vhost-user backends don't implement
the SET_STATUS message, so I think no one has tripped over this yet.

Any thoughts on making vhost-user behave like vDPA here?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Move vhost-user SET_STATUS 0 after get vring base?

[Michael S. Tsirkin]

On Tue, Apr 18, 2023 at 11:18:11AM -0400, Stefan Hajnoczi wrote:
> Hi,
> Cindy's commit ca71db438bdc ("vhost: implement vhost_dev_start method")
> added SET_STATUS calls to vhost_dev_start() and vhost_dev_stop() for all
> vhost backends.
> 
> Eugenio's commit c3716f260bff ("vdpa: move vhost reset after get vring
> base") deferred the SET_STATUS 0 call in vhost_dev_stop() until after
> GET_VRING_BASE for vDPA only. In that commit Eugenio said, "A patch to
> make vhost_user_dev_start more similar to vdpa is desirable, but it can
> be added on top".
> 
> I agree and think it's a good idea to keep the vhost backends in sync
> where possible.
> 
> vhost-user still has the old behavior where QEMU sends SET_STATUS 0
> before GET_VRING_BASE. Most existing vhost-user backends don't implement
> the SET_STATUS message, so I think no one has tripped over this yet.
> 
> Any thoughts on making vhost-user behave like vDPA here?
> 
> Stefan

Wow. Well  SET_STATUS 0 resets the device so yes, I think doing that
before GET_VRING_BASE will lose a state. Donnu how it does not trip
up people, indeed the only idea is if people ignore SET_STATUS.


-- 
MST

Re: Move vhost-user SET_STATUS 0 after get vring base?

[Yajun Wu]

On 4/18/2023 11:34 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 18, 2023 at 11:18:11AM -0400, Stefan Hajnoczi wrote:
>> Hi,
>> Cindy's commit ca71db438bdc ("vhost: implement vhost_dev_start method")
>> added SET_STATUS calls to vhost_dev_start() and vhost_dev_stop() for all
>> vhost backends.
>>
>> Eugenio's commit c3716f260bff ("vdpa: move vhost reset after get vring
>> base") deferred the SET_STATUS 0 call in vhost_dev_stop() until after
>> GET_VRING_BASE for vDPA only. In that commit Eugenio said, "A patch to
>> make vhost_user_dev_start more similar to vdpa is desirable, but it can
>> be added on top".
>>
>> I agree and think it's a good idea to keep the vhost backends in sync
>> where possible.
>>
>> vhost-user still has the old behavior where QEMU sends SET_STATUS 0
>> before GET_VRING_BASE. Most existing vhost-user backends don't implement
>> the SET_STATUS message, so I think no one has tripped over this yet.
>>
>> Any thoughts on making vhost-user behave like vDPA here?
>>
>> Stefan
> Wow. Well  SET_STATUS 0 resets the device so yes, I think doing that
> before GET_VRING_BASE will lose a state. Donnu how it does not trip
> up people, indeed the only idea is if people ignore SET_STATUS.
>
>
> --
> MST

For DPDK vhost-user backend SET_STATUS 0 (reset) is ignored.

Yajun

Re: Move vhost-user SET_STATUS 0 after get vring base?

[Eugenio Perez Martin]

On Tue, Apr 18, 2023 at 5:18 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Hi,
> Cindy's commit ca71db438bdc ("vhost: implement vhost_dev_start method")
> added SET_STATUS calls to vhost_dev_start() and vhost_dev_stop() for all
> vhost backends.
>
> Eugenio's commit c3716f260bff ("vdpa: move vhost reset after get vring
> base") deferred the SET_STATUS 0 call in vhost_dev_stop() until after
> GET_VRING_BASE for vDPA only. In that commit Eugenio said, "A patch to
> make vhost_user_dev_start more similar to vdpa is desirable, but it can
> be added on top".
>
> I agree and think it's a good idea to keep the vhost backends in sync
> where possible.
>
> vhost-user still has the old behavior where QEMU sends SET_STATUS 0
> before GET_VRING_BASE. Most existing vhost-user backends don't implement
> the SET_STATUS message, so I think no one has tripped over this yet.
>

My bet is that those backends simply do not migrate so they don't hit
it. But maybe those backends return -1 for GET_VRING_BASE and use
split vq, so it can be fetched from guest's used idx?

> Any thoughts on making vhost-user behave like vDPA here?
>

I guess the first step should be to gather a list of backends that use
SET_STATUS and are interested in migration. But in my opinion the
current behavior can be considered a bug and it is unlikely that it is
implemented properly there.

* If they ignore the set_status, we can totally reorder the order and
it will be the same.
* If they always return an error for GET_VRING_BASE then they will
keep it returning, so no harm here either.
* If they use more complicated logic like "return -1 for
GET_VRING_BASE as long as the device is not DRIVER_OK". Improving the
situation in this case.

Thanks!