Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset

[Hanna Czenczek <hreitz@redhat.com>]

On 19.07.23 16:11, Hanna Czenczek wrote:
> On 18.07.23 17:10, Stefan Hajnoczi wrote:
>> On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
>>> The only user of vhost_user_reset_status() is vhost_dev_stop(), which
>>> only uses it as a fall-back to stop the back-end if it does not support
>>> SUSPEND.  However, vhost-user's implementation is a no-op unless the
>>> back-end supports SET_STATUS.
>>>
>>> vhost-vdpa's implementation instead just calls
>>> vhost_vdpa_reset_device(), implying that it's OK to fully reset the
>>> device if SET_STATUS is not supported.
>>>
>>> To be fair, vhost_vdpa_reset_device() does nothing but to set the 
>>> status
>>> to zero.  However, that may well be because vhost-vdpa has no method
>>> besides this to reset a device.  In contrast, vhost-user has
>>> RESET_DEVICE and a RESET_OWNER, which can be used instead.
>>>
>>> While it is not entirely clear from documentation or git logs, from
>>> discussions and the order of vhost-user protocol features, it 
>>> appears to
>>> me as if RESET_OWNER originally had no real meaning for vhost-user, and
>>> was thus used to signal a device reset to the back-end.  Then,
>>> RESET_DEVICE was introduced, to have a well-defined dedicated reset
>>> command.  Finally, vhost-user received full STATUS support, including
>>> SET_STATUS, so setting the device status to 0 is now the preferred way
>>> of resetting a device.  Still, RESET_DEVICE and RESET_OWNER should
>>> remain valid as fall-backs.
>>>
>>> Therefore, have vhost_user_reset_status() fall back to
>>> vhost_user_reset_device() if the back-end has no STATUS support.
>>>
>>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>>> ---
>>>   hw/virtio/vhost-user.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index 4507de5a92..53a881ec2a 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct 
>>> vhost_dev *dev)
>>>       if (virtio_has_feature(dev->protocol_features,
>>>                              VHOST_USER_PROTOCOL_F_STATUS)) {
>>>           vhost_user_set_status(dev, 0);
>>> +    } else {
>>> +        vhost_user_reset_device(dev);
>>>       }
>>>   }
>> Did you check whether DPDK treats setting the status to 0 as equivalent
>> to RESET_DEVICE?
>
> If it doesn’t, what’s even the point of using reset_status?

Sorry, I’m being unclear, and I think this may be important because it 
ties into the question from patch 1, what qemu is even trying to do by 
running SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the 
impression that SET_STATUS(0) and RESET_DEVICE should be equivalent:

vhost-vdpa.c runs SET_STATUS(0) in a function called 
vhost_vdpa_reset_device().  This is one thing that gave me the 
impression that this is about an actual full reset.

Another is the whole discussion that we’ve had.  vhost_dev_stop() does 
not call a `vhost_reset_device()` function, it calls 
`vhost_reset_status()`.  Still, we were always talking about resetting 
the device.

It doesn’t make sense to me that vDPA would provide no function to fully 
reset a device, while vhost-user does.  Being able to reset a device 
sounds vital to me.  This also gave me the impression that SET_STATUS(0) 
on vDPA at least is functionally equivalent to a full device reset.

Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on 
vhost-user.  That would be a real shame, so I assumed this would not be 
the case; that SET_STATUS(0) does the same thing on both protocols.

The virtio specification says “Writing 0 into this field resets the 
device.” about the device_status field.

This also makes sense, because the device_status field is basically used 
to tell the device that a driver has taken control.  If reset, this 
indicates the driver has given up control, and to me this is a point 
where a device should fully reset itself.

So all in all, I can’t see the rationale why any implementation that 
supports SET_STATUS would decide to treat SET_STATUS(0) not as 
equivalent or a superset of RESET_DEVICE.  I may be wrong, and this 
might explain a whole deal about what kind of background operations we 
hope to stop with SET_STATUS(0).

Hanna