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

[Claudio Fontana <cfontana@suse.de>]

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.


Indeed, thanks I completely overlooked it.

However we end up with multiple calls to k->set_status,
one from the virtio_set_status call, and one from the virtio_reset() call, from a single 0 write.

Also I wonder about the ordering. Starting from the guest driver writing 0,
we end up (for virtio-net), calling virtio_net_set_status() first,
which lands us in virtio_net_vhost_status(),
which in the case of an already started vhost lands us in vhost_net_stop().

If we do virtio_reset immediately on seeing the guest writing 0,
we instead land first in virtio_net_reset().

Which one is the right behavior? I am having more success with the second ordering currently.
I can describe the scenario in more detail if needed in this context.

All in all, I think that the meaning of virtio_set_status as a function is unclear;
if is it supposed to match the virtio standard meaning of be virtio status field, then I'd expect we should do the virtio reset right there,
like we process the other bits of the status field, and not do it again from virtio_pci...

I wonder also about all the callers of virtio_set_status, what the assumptions of each caller are...

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