Re: [PULL v2 31/41] virtio-pci: implement queue_enabled method

[Laurent Vivier]

On 27/07/2020 16:00, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 03:51:41PM +0200, Laurent Vivier wrote:
>> On 04/07/2020 20:30, Michael S. Tsirkin wrote:
>>> From: Jason Wang <jasowang@redhat.com>
>>>
>>> With version 1, we can detect whether a queue is enabled via
>>> queue_enabled.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>>> Message-Id: <20200701145538.22333-5-lulu@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>  hw/virtio/virtio-pci.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>> index 7bc8c1c056..8554cf2a03 100644
>>> --- a/hw/virtio/virtio-pci.c
>>> +++ b/hw/virtio/virtio-pci.c
>>> @@ -1107,6 +1107,18 @@ static AddressSpace 
>>> *virtio_pci_get_dma_as(DeviceState *d)
>>>      return pci_get_address_space(dev);
>>>  }
>>>  
>>> +static bool virtio_pci_queue_enabled(DeviceState *d, int n)
>>> +{
>>> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>> +
>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>>> +        return proxy->vqs[vdev->queue_sel].enabled;
>>> +    }
>>> +
>>> +    return virtio_queue_enabled(vdev, n);
>>> +}
>>
>> With "disable-legacy=off,disable-modern=true",
>> this changes introduces an infinite loop: virtio_queue_enabled() calls
>> again virtio_pci_queue_enabled() that calls
>> againvirtio_pci_queue_enabled()...
>>
>> I think this should be changed like this:
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index ada1101d07..0a85c17e91 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1116,7 +1116,7 @@ static bool virtio_pci_queue_enabled(DeviceState
>> *d, int n)
>>          return proxy->vqs[vdev->queue_sel].enabled;
>>      }
>>
>> -    return virtio_queue_enabled(vdev, n);
>> +    return virtio_queue_get_desc_addr(vdev, n) != 0;
>>  }
>>
> 
> Thanks for the report and debugging the issue!

Ironically, the bug has been introduced because of my comment on the series:

https://patchew.org/QEMU/20200529140620.28759-1-lulu@redhat.com/20200529140620.28759-5-lulu@redhat.com/

Sorry for that.

> Maybe move
>        return virtio_queue_get_desc_addr(vdev, n) != 0;
> to a new API
>       virtio_pci_queue_enabled_legacy()
> 
> to avoid code duplication.
> 
> Could you cook up a patch pls? don't forget the Fixes: tag
> so people remember to backport it.

Yes. I'm going to do that.

Thanks,
Laurent