Re: virtio: why no full reset on virtio_set_status 0 ?

[Claudio Fontana <cfontana@suse.de>]

On 7/28/22 09:43, Claudio Fontana wrote:
> On 7/28/22 03:27, Jason Wang wrote:
>> On Wed, Jul 27, 2022 at 11:32 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Wed, Jul 27, 2022 at 12:51:31PM +0200, Claudio Fontana wrote:
>>>> Hi Michael and all,
>>>>
>>>> I have started researching a qemu / ovs / dpdk bug:
>>>>
>>>> https://inbox.dpdk.org/dev/322122fb-619d-96f6-5c3e-9eabdbf3819a@redhat.com/T/
>>>>
>>>> that seems to be affecting multiple parties in the telco space,
>>>>
>>>> and during this process I noticed that qemu/hw/virtio/virtio.c does not do a full virtio reset
>>>> in virtio_set_status, when receiving a status value of 0.
>>>>
>>>> It seems it has always been this way, so I am clearly missing / forgetting something basic,
>>>>
>>>> I checked the virtio spec at https://docs.oasis-open.org/
>>>>
>>>> and from:
>>>>
>>>> "
>>>> 4.1.4.3 Common configuration structure layout
>>>>
>>>> device_status
>>>> The driver writes the device status here (see 2.1). Writing 0 into this field resets the device.
>>>>
>>>> "
>>>>
>>>> and
>>>>
>>>> "
>>>> 2.4.1 Device Requirements: Device Reset
>>>> A device MUST reinitialize device status to 0 after receiving a reset.
>>>> "
>>>>
>>>> I would conclude that in virtio.c::virtio_set_status we should unconditionally do a full virtio_reset.
>>>>
>>>> Instead, we have just the check:
>>>>
>>>>     if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) !=
>>>>         (val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>>         virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
>>>>     }
>>>>
>>>> which just sets the started field,
>>>>
>>>> and then we have the call to the virtio device class set_status (virtio_net...),
>>>> but the VirtioDevice is not fully reset, as per the virtio_reset() call we are missing:
>>>>
>>>> "
>>>>     vdev->start_on_kick = false;
>>>>     vdev->started = false;
>>>>     vdev->broken = false;
>>>>     vdev->guest_features = 0;
>>>>     vdev->queue_sel = 0;
>>>>     vdev->status = 0;
>>>>     vdev->disabled = false;
>>>>     qatomic_set(&vdev->isr, 0);
>>>>     vdev->config_vector = VIRTIO_NO_VECTOR;
>>>>     virtio_notify_vector(vdev, vdev->config_vector);
>>>>
>>>>     for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>>>>         ... initialize vdev->vq[i] ...
>>>>     }
>>>> "
>>>>
>>>> Doing a full reset seems to fix the problem for me, so I can send tentative patches if necessary,
>>>> but what am I missing here?
>>>>
>>>> Thanks,
>>>>
>>>> Claudio
>>>>
>>>> --
>>>> Claudio Fontana
>>>> Engineering Manager Virtualization, SUSE Labs Core
>>>>
>>>> SUSE Software Solutions Italy Srl
>>>
>>>
>>> So for example for pci:
>>>
>>>     case VIRTIO_PCI_STATUS:
>>>
>>>
>>>         ....
>>>
>>>         if (vdev->status == 0) {
>>>             virtio_pci_reset(DEVICE(proxy));
>>>         }
>>>
>>> which I suspect is a bug because:
>>>
>>> static void virtio_pci_reset(DeviceState *qdev)
>>> {
>>>     VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>>>     VirtioBusState *bus = VIRTIO_BUS(&proxy->bus);
>>>     PCIDevice *dev = PCI_DEVICE(qdev);
>>>     int i;
>>>
>>>     virtio_bus_reset(bus);
>>
>> Note that we do virtio_reset() here.
> 
> 
> Yes, thank you, I completely overlooked it, I noticed this in Michael's response as well.
> 
> However we end up with multiple calls to k->set_status, one from the virtio_set_status call,
> and one from the virtio_bus_reset(), which is probably something we don't want.
> 
> All in all it is not clear what the meaning of virtio_set_status is supposed to be I think,
> and I wonder what the assumptions are among all the callers.
> If it is supposed to be an implementation of the virtio standard field as described, I think we should do the reset right then and there,
> but maybe the true meaning of the function is another one I couldn't understand, since _some_ of the cases are processes there.
> 
> And there is a question about ordering:
> 
> in virtio_pci we end up calling virtio_set_status(0), which gets us k->set_status(vdev, 0), which lands in virtio_net_set_status(0) and virtio_net_vhost_status,
> which causes a vhost_net_stop().
> 
> Should we instead land in virtio_net_reset() first, by doing a virtio reset earlier when detecting a 0 value from the driver?
> 
> in the scenario I am looking at (with vhost-user, ovs/dpdk, and a guest testpmd application),
> the guest application goes away without any chance to signal (kill -9), then gets immediately restarted and does a write of 0 to status, while qemu and ovs still hold the state for the device.
> 
> As QEMU lands in vhost_net_stop(), it seems to cause a chain of events that crash ovs which is trying to read an rx burst from the queue,
> while QEMU is left hanging waiting forever for a response to VHOST_USER_GET_VRING_BASE issued as a result of vhost_net_stop.
> 
> Just saying, I am having more success with the second ordering, but I am still studying, don't have the full picture yet.


Currently I'm doing (on top of Michael's patch) the following which seems to be working
(but of course this does not even being to look at the other transports, architectures etc),
just an idea to share:

---
 hw/virtio/virtio-pci.c | 7 ++++---
 hw/virtio/virtio.c     | 7 ++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 3189ec014d..3cbfa3ce3a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -312,6 +312,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case VIRTIO_PCI_QUEUE_PFN:
         pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
         if (pa == 0) {
+            virtio_bus_reset(&proxy->bus);
             virtio_pci_reset(DEVICE(proxy));
         }
         else
@@ -1941,11 +1942,8 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
 static void virtio_pci_reset(DeviceState *qdev)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
-    VirtioBusState *bus = VIRTIO_BUS(&proxy->bus);
-    PCIDevice *dev = PCI_DEVICE(qdev);
     int i;
 
-    virtio_bus_reset(bus);
     msix_unuse_all_vectors(&proxy->pci_dev);
 
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
@@ -1960,7 +1958,10 @@ static void virtio_pci_reset(DeviceState *qdev)
 static void virtio_pci_bus_reset(DeviceState *qdev)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
+    VirtioBusState *bus = VIRTIO_BUS(&proxy->bus);
 
+    virtio_bus_reset(bus);
     virtio_pci_reset(qdev);
 
     if (pci_is_express(dev)) {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..da58ca6f86 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1977,6 +1977,12 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     trace_virtio_set_status(vdev, val);
 
+    if (val == 0) {
+        VirtioBusState *bus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
+        virtio_bus_reset(bus);
+        return 0;
+    }
+
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) &&
             val & VIRTIO_CONFIG_S_FEATURES_OK) {
@@ -2025,7 +2031,6 @@ void virtio_reset(void *opaque)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     int i;
 
-    virtio_set_status(vdev, 0);
     if (current_cpu) {
         /* Guest initiated reset */
         vdev->device_endian = virtio_current_cpu_endian();
-- 
2.26.2






> 
> Thanks,
> 
> Claudio
> 
>>
>>>     msix_unuse_all_vectors(&proxy->pci_dev);
>>>
>>>     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>>>         proxy->vqs[i].enabled = 0;
>>>         proxy->vqs[i].num = 0;
>>>         proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
>>>         proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
>>>         proxy->vqs[i].used[0] = proxy->vqs[i].used[1] = 0;
>>>     }
>>>
>>>
>>> so far so good
>>>
>>>     if (pci_is_express(dev)) {
>>>         pcie_cap_deverr_reset(dev);
>>>         pcie_cap_lnkctl_reset(dev);
>>>
>>>         pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
>>>     }
>>>
>>> this part is wrong I think, it got here by mistake since the same
>>> function is used for bus level reset.
>>>
>>> Jason, Marcel, any input?
>>
>> Yes, I think we don't need PCI stuff here. We do virtio reset not pci.
>>
>> Thanks
>>
>>>
>>> --
>>> MST
>>>
>>
>>
> 
>