1. Patchew
  2. QEMU
  3. View series

[PATCH] virtio-pci: fix queue_enable write

Jason Wang posted 1 patch 3 years, 11 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan failed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200529030728.7687-1-jasowang@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
hw/virtio/virtio-pci.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
[PATCH] virtio-pci: fix queue_enable write
Posted by Jason Wang 3 years, 11 months ago 
Spec said: The driver uses this to selectively prevent the device from
executing requests from this virtqueue. 1 - enabled; 0 - disabled.

Though write 0 to queue_enable is forbidden by the sepc, we should not
assume that the value is 1.

Fix this by ignoring the write value other than 1.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index d028c17c24..b3558eeaee 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1273,16 +1273,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         virtio_queue_set_vector(vdev, vdev->queue_sel, val);
         break;
     case VIRTIO_PCI_COMMON_Q_ENABLE:
-        virtio_queue_set_num(vdev, vdev->queue_sel,
-                             proxy->vqs[vdev->queue_sel].num);
-        virtio_queue_set_rings(vdev, vdev->queue_sel,
+        if (val == 1) {
+            virtio_queue_set_num(vdev, vdev->queue_sel,
+                                 proxy->vqs[vdev->queue_sel].num);
+            virtio_queue_set_rings(vdev, vdev->queue_sel,
                        ((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
                        proxy->vqs[vdev->queue_sel].desc[0],
                        ((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
                        proxy->vqs[vdev->queue_sel].avail[0],
                        ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
                        proxy->vqs[vdev->queue_sel].used[0]);
-        proxy->vqs[vdev->queue_sel].enabled = 1;
+            proxy->vqs[vdev->queue_sel].enabled = 1;
+        }
         break;
     case VIRTIO_PCI_COMMON_Q_DESCLO:
         proxy->vqs[vdev->queue_sel].desc[0] = val;
-- 
2.20.1
Re: [PATCH] virtio-pci: fix queue_enable write
Posted by Stefan Hajnoczi 3 years, 11 months ago 
On Fri, May 29, 2020 at 11:07:28AM +0800, Jason Wang wrote:
> Spec said: The driver uses this to selectively prevent the device from
> executing requests from this virtqueue. 1 - enabled; 0 - disabled.
> 
> Though write 0 to queue_enable is forbidden by the sepc, we should not
> assume that the value is 1.
> 
> Fix this by ignoring the write value other than 1.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [PATCH] virtio-pci: fix queue_enable write
Posted by Michael S. Tsirkin 3 years, 10 months ago 
On Fri, May 29, 2020 at 11:07:28AM +0800, Jason Wang wrote:
> Spec said: The driver uses this to selectively prevent the device from
> executing requests from this virtqueue. 1 - enabled; 0 - disabled.
> 
> Though write 0 to queue_enable is forbidden by the sepc, we should not

spec?

> assume that the value is 1.
> 
> Fix this by ignoring the write value other than 1.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>


Do we want to call virtio_error here so we can figure out something's wrong?



> ---
>  hw/virtio/virtio-pci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index d028c17c24..b3558eeaee 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1273,16 +1273,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
>          break;
>      case VIRTIO_PCI_COMMON_Q_ENABLE:
> -        virtio_queue_set_num(vdev, vdev->queue_sel,
> -                             proxy->vqs[vdev->queue_sel].num);
> -        virtio_queue_set_rings(vdev, vdev->queue_sel,
> +        if (val == 1) {
> +            virtio_queue_set_num(vdev, vdev->queue_sel,
> +                                 proxy->vqs[vdev->queue_sel].num);
> +            virtio_queue_set_rings(vdev, vdev->queue_sel,
>                         ((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
>                         proxy->vqs[vdev->queue_sel].desc[0],
>                         ((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
>                         proxy->vqs[vdev->queue_sel].avail[0],
>                         ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>                         proxy->vqs[vdev->queue_sel].used[0]);
> -        proxy->vqs[vdev->queue_sel].enabled = 1;
> +            proxy->vqs[vdev->queue_sel].enabled = 1;
> +        }
>          break;
>      case VIRTIO_PCI_COMMON_Q_DESCLO:
>          proxy->vqs[vdev->queue_sel].desc[0] = val;
> -- 
> 2.20.1
Re: [PATCH] virtio-pci: fix queue_enable write
Posted by Jason Wang 3 years, 10 months ago 
On 2020/6/9 下午11:43, Michael S. Tsirkin wrote:
> On Fri, May 29, 2020 at 11:07:28AM +0800, Jason Wang wrote:
>> Spec said: The driver uses this to selectively prevent the device from
>> executing requests from this virtqueue. 1 - enabled; 0 - disabled.
>>
>> Though write 0 to queue_enable is forbidden by the sepc, we should not
> spec?


Chapter 4.1.4.3.2 said:

"

The driver MUST NOT write a 0 to queue_enable.

"


>
>> assume that the value is 1.
>>
>> Fix this by ignoring the write value other than 1.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Do we want to call virtio_error here so we can figure out something's wrong?


That looks better. Will do.

Thanks


>
>
>
>> ---
>>   hw/virtio/virtio-pci.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index d028c17c24..b3558eeaee 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1273,16 +1273,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>>           virtio_queue_set_vector(vdev, vdev->queue_sel, val);
>>           break;
>>       case VIRTIO_PCI_COMMON_Q_ENABLE:
>> -        virtio_queue_set_num(vdev, vdev->queue_sel,
>> -                             proxy->vqs[vdev->queue_sel].num);
>> -        virtio_queue_set_rings(vdev, vdev->queue_sel,
>> +        if (val == 1) {
>> +            virtio_queue_set_num(vdev, vdev->queue_sel,
>> +                                 proxy->vqs[vdev->queue_sel].num);
>> +            virtio_queue_set_rings(vdev, vdev->queue_sel,
>>                          ((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
>>                          proxy->vqs[vdev->queue_sel].desc[0],
>>                          ((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
>>                          proxy->vqs[vdev->queue_sel].avail[0],
>>                          ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>>                          proxy->vqs[vdev->queue_sel].used[0]);
>> -        proxy->vqs[vdev->queue_sel].enabled = 1;
>> +            proxy->vqs[vdev->queue_sel].enabled = 1;
>> +        }
>>           break;
>>       case VIRTIO_PCI_COMMON_Q_DESCLO:
>>           proxy->vqs[vdev->queue_sel].desc[0] = val;
>> -- 
>> 2.20.1
>
Re: [PATCH] virtio-pci: fix queue_enable write
Posted by Michael S. Tsirkin 3 years, 10 months ago 
On Wed, Jun 10, 2020 at 10:03:28AM +0800, Jason Wang wrote:
> 
> On 2020/6/9 下午11:43, Michael S. Tsirkin wrote:
> > On Fri, May 29, 2020 at 11:07:28AM +0800, Jason Wang wrote:
> > > Spec said: The driver uses this to selectively prevent the device from
> > > executing requests from this virtqueue. 1 - enabled; 0 - disabled.
> > > 
> > > Though write 0 to queue_enable is forbidden by the sepc, we should not
> > spec?
> 
> 
> Chapter 4.1.4.3.2 said:
> 
> "
> 
> The driver MUST NOT write a 0 to queue_enable.
> 
> "
> 


I mean you misspelled spec as sepc :)

> > 
> > > assume that the value is 1.
> > > 
> > > Fix this by ignoring the write value other than 1.
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > 
> > Do we want to call virtio_error here so we can figure out something's wrong?
> 
> 
> That looks better. Will do.
> 
> Thanks
> 
> 
> > 
> > 
> > 
> > > ---
> > >   hw/virtio/virtio-pci.c | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index d028c17c24..b3558eeaee 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -1273,16 +1273,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > >           virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > >           break;
> > >       case VIRTIO_PCI_COMMON_Q_ENABLE:
> > > -        virtio_queue_set_num(vdev, vdev->queue_sel,
> > > -                             proxy->vqs[vdev->queue_sel].num);
> > > -        virtio_queue_set_rings(vdev, vdev->queue_sel,
> > > +        if (val == 1) {
> > > +            virtio_queue_set_num(vdev, vdev->queue_sel,
> > > +                                 proxy->vqs[vdev->queue_sel].num);
> > > +            virtio_queue_set_rings(vdev, vdev->queue_sel,
> > >                          ((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
> > >                          proxy->vqs[vdev->queue_sel].desc[0],
> > >                          ((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
> > >                          proxy->vqs[vdev->queue_sel].avail[0],
> > >                          ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
> > >                          proxy->vqs[vdev->queue_sel].used[0]);
> > > -        proxy->vqs[vdev->queue_sel].enabled = 1;
> > > +            proxy->vqs[vdev->queue_sel].enabled = 1;
> > > +        }
> > >           break;
> > >       case VIRTIO_PCI_COMMON_Q_DESCLO:
> > >           proxy->vqs[vdev->queue_sel].desc[0] = val;
> > > -- 
> > > 2.20.1
> >
Re: [PATCH] virtio-pci: fix queue_enable write
Posted by Jason Wang 3 years, 10 months ago 
On 2020/6/10 下午12:16, Michael S. Tsirkin wrote:
> On Wed, Jun 10, 2020 at 10:03:28AM +0800, Jason Wang wrote:
>> On 2020/6/9 下午11:43, Michael S. Tsirkin wrote:
>>> On Fri, May 29, 2020 at 11:07:28AM +0800, Jason Wang wrote:
>>>> Spec said: The driver uses this to selectively prevent the device from
>>>> executing requests from this virtqueue. 1 - enabled; 0 - disabled.
>>>>
>>>> Though write 0 to queue_enable is forbidden by the sepc, we should not
>>> spec?
>>
>> Chapter 4.1.4.3.2 said:
>>
>> "
>>
>> The driver MUST NOT write a 0 to queue_enable.
>>
>> "
>>
>
> I mean you misspelled spec as sepc :)


Oh, right.

Will fix.

Thanks


>
>>>> assume that the value is 1.
>>>>
>>>> Fix this by ignoring the write value other than 1.
>>>>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Do we want to call virtio_error here so we can figure out something's wrong?
>>
>> That looks better. Will do.
>>
>> Thanks
>>
>>
>>>
>>>
>>>> ---
>>>>    hw/virtio/virtio-pci.c | 10 ++++++----
>>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>> index d028c17c24..b3558eeaee 100644
>>>> --- a/hw/virtio/virtio-pci.c
>>>> +++ b/hw/virtio/virtio-pci.c
>>>> @@ -1273,16 +1273,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>>>>            virtio_queue_set_vector(vdev, vdev->queue_sel, val);
>>>>            break;
>>>>        case VIRTIO_PCI_COMMON_Q_ENABLE:
>>>> -        virtio_queue_set_num(vdev, vdev->queue_sel,
>>>> -                             proxy->vqs[vdev->queue_sel].num);
>>>> -        virtio_queue_set_rings(vdev, vdev->queue_sel,
>>>> +        if (val == 1) {
>>>> +            virtio_queue_set_num(vdev, vdev->queue_sel,
>>>> +                                 proxy->vqs[vdev->queue_sel].num);
>>>> +            virtio_queue_set_rings(vdev, vdev->queue_sel,
>>>>                           ((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
>>>>                           proxy->vqs[vdev->queue_sel].desc[0],
>>>>                           ((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
>>>>                           proxy->vqs[vdev->queue_sel].avail[0],
>>>>                           ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>>>>                           proxy->vqs[vdev->queue_sel].used[0]);
>>>> -        proxy->vqs[vdev->queue_sel].enabled = 1;
>>>> +            proxy->vqs[vdev->queue_sel].enabled = 1;
>>>> +        }
>>>>            break;
>>>>        case VIRTIO_PCI_COMMON_Q_DESCLO:
>>>>            proxy->vqs[vdev->queue_sel].desc[0] = val;
>>>> -- 
>>>> 2.20.1

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.