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 statusto 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, fromdiscussions and the order of vhost-user protocol features, it appears tome 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 <hre...@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
