diff mbox series

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

Message ID 20200704182750.1088103-32-mst@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,v2,01/41] tests: disassemble-aml.sh: generate AML in readable format | expand

Commit Message

Michael S. Tsirkin July 4, 2020, 6:30 p.m. UTC 
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(+)

Comments

Laurent Vivier July 27, 2020, 1:51 p.m. UTC | #1 
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;
 }


> +
>  static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
>                                     struct virtio_pci_cap *cap)
>  {
> @@ -2064,6 +2076,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
>      k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
>      k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
>      k->get_dma_as = virtio_pci_get_dma_as;
> +    k->queue_enabled = virtio_pci_queue_enabled;
>  }
>  
>  static const TypeInfo virtio_pci_bus_info = {
> 

Thanks,
Laurent
Michael S. Tsirkin July 27, 2020, 2 p.m. UTC | #2 
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!

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.




> > +
> >  static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
> >                                     struct virtio_pci_cap *cap)
> >  {
> > @@ -2064,6 +2076,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> >      k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
> >      k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
> >      k->get_dma_as = virtio_pci_get_dma_as;
> > +    k->queue_enabled = virtio_pci_queue_enabled;
> >  }
> >  
> >  static const TypeInfo virtio_pci_bus_info = {
> > 
> 
> Thanks,
> Laurent
Laurent Vivier July 27, 2020, 2:06 p.m. UTC | #3 
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
diff mbox series

Patch

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);
+}
+
 static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
                                    struct virtio_pci_cap *cap)
 {
@@ -2064,6 +2076,7 @@  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
     k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
     k->get_dma_as = virtio_pci_get_dma_as;
+    k->queue_enabled = virtio_pci_queue_enabled;
 }
 
 static const TypeInfo virtio_pci_bus_info = {