[PATCH v1] virtio-pci: store virtqueue size directly to a device

[PATCH v1] virtio-pci: store virtqueue size directly to a device

[Denis Plotnikov]

Currenly, the virtqueue size is saved to the proxy on pci writing and
is read from the device pci reading.
The virtqueue size is propagated later on form the proxy to the device
on virqueue enabling stage.

This could be a problem, if a guest, on the virtqueue configuration, sets
the size and then re-read it immediatly before the queue enabling
in order to check if the desiged size has been set.

This happens in seabios: (sebios snippet)

vp_find_vq()
{
    ...
    /* check if the queue is available */
    if (vp->use_modern) {
        num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
        if (num > MAX_QUEUE_NUM) {
            vp_write(&vp->common, virtio_pci_common_cfg, queue_size,
                     MAX_QUEUE_NUM);
            num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
        }
    } else {
        num = vp_read(&vp->legacy, virtio_pci_legacy, queue_num);
    }
    if (!num) {
        dprintf(1, "ERROR: queue size is 0\n");
        goto fail;
    }
    if (num > MAX_QUEUE_NUM) {
        dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
        goto fail;
    }
    ...
}

If the device queue num is greater then the max queue size supported by seabios,
seabios tries to reduce the queue size, then re-read it again, I suppose to
check if the setting actually happens, and then checks the virtqueue size again,
to deside whether it is satisfied with the vaule.
In this case, if device's virtqueue size is 512 and seabios max supported queue
size is 256, seabios tries to set 256 but than read 512 again and can't proceed
with that vaule, preventing the guest from successful booting.
The root case was investigated by Roman Kagan <rkagan@virtuozzo.com>

The patch fixes the problem, by propagating the queue size to the device right
away, so the written value could be read on the next step, if the value was
ok for the device.

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 hw/virtio/virtio-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b47a9c73..e5c759e19e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1256,6 +1256,8 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         break;
     case VIRTIO_PCI_COMMON_Q_SIZE:
         proxy->vqs[vdev->queue_sel].num = val;
+        virtio_queue_set_num(vdev, vdev->queue_sel,
+                             proxy->vqs[vdev->queue_sel].num);
         break;
     case VIRTIO_PCI_COMMON_Q_MSIX:
         msix_vector_unuse(&proxy->pci_dev,
-- 
2.17.0

Re: [PATCH v1] virtio-pci: store virtqueue size directly to a device

[Michael S. Tsirkin]

On Mon, Dec 23, 2019 at 02:37:58PM +0300, Denis Plotnikov wrote:
> Currenly, the virtqueue size is saved to the proxy on pci writing and
> is read from the device pci reading.
> The virtqueue size is propagated later on form the proxy to the device
> on virqueue enabling stage.
> 
> This could be a problem, if a guest, on the virtqueue configuration, sets
> the size and then re-read it immediatly before the queue enabling
> in order to check if the desiged size has been set.
> 
> This happens in seabios: (sebios snippet)
> 
> vp_find_vq()
> {
>     ...
>     /* check if the queue is available */
>     if (vp->use_modern) {
>         num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
>         if (num > MAX_QUEUE_NUM) {
>             vp_write(&vp->common, virtio_pci_common_cfg, queue_size,
>                      MAX_QUEUE_NUM);
>             num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
>         }
>     } else {
>         num = vp_read(&vp->legacy, virtio_pci_legacy, queue_num);
>     }
>     if (!num) {
>         dprintf(1, "ERROR: queue size is 0\n");
>         goto fail;
>     }
>     if (num > MAX_QUEUE_NUM) {
>         dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>         goto fail;
>     }
>     ...
> }
> 
> If the device queue num is greater then the max queue size supported by seabios,
> seabios tries to reduce the queue size, then re-read it again, I suppose to
> check if the setting actually happens, and then checks the virtqueue size again,
> to deside whether it is satisfied with the vaule.
> In this case, if device's virtqueue size is 512 and seabios max supported queue
> size is 256, seabios tries to set 256 but than read 512 again and can't proceed
> with that vaule, preventing the guest from successful booting.
> The root case was investigated by Roman Kagan <rkagan@virtuozzo.com>
> 
> The patch fixes the problem, by propagating the queue size to the device right
> away, so the written value could be read on the next step, if the value was
> ok for the device.
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

Thanks, I already have this queued as:

commit 8aabbbd9d04f95d5581d2275362996ecb5516dd9
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Fri Dec 13 09:22:48 2019 -0500

    virtio: update queue size on guest write
    
    Some guests read back queue size after writing it.
    Update the size immediatly upon write otherwise
    they get confused.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I would appreciate checking other transports, they likely
need the same fix.


> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index c6b47a9c73..e5c759e19e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1256,6 +1256,8 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>          break;
>      case VIRTIO_PCI_COMMON_Q_SIZE:
>          proxy->vqs[vdev->queue_sel].num = val;
> +        virtio_queue_set_num(vdev, vdev->queue_sel,
> +                             proxy->vqs[vdev->queue_sel].num);
>          break;
>      case VIRTIO_PCI_COMMON_Q_MSIX:
>          msix_vector_unuse(&proxy->pci_dev,
> -- 
> 2.17.0

Re: [PATCH v1] virtio-pci: store virtqueue size directly to a device

[Denis Plotnikov]

On 23.12.2019 17:31, Michael S. Tsirkin wrote:
> On Mon, Dec 23, 2019 at 02:37:58PM +0300, Denis Plotnikov wrote:
>> Currenly, the virtqueue size is saved to the proxy on pci writing and
>> is read from the device pci reading.
>> The virtqueue size is propagated later on form the proxy to the device
>> on virqueue enabling stage.
>>
>> This could be a problem, if a guest, on the virtqueue configuration, sets
>> the size and then re-read it immediatly before the queue enabling
>> in order to check if the desiged size has been set.
>>
>> This happens in seabios: (sebios snippet)
>>
>> vp_find_vq()
>> {
>>      ...
>>      /* check if the queue is available */
>>      if (vp->use_modern) {
>>          num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
>>          if (num > MAX_QUEUE_NUM) {
>>              vp_write(&vp->common, virtio_pci_common_cfg, queue_size,
>>                       MAX_QUEUE_NUM);
>>              num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size);
>>          }
>>      } else {
>>          num = vp_read(&vp->legacy, virtio_pci_legacy, queue_num);
>>      }
>>      if (!num) {
>>          dprintf(1, "ERROR: queue size is 0\n");
>>          goto fail;
>>      }
>>      if (num > MAX_QUEUE_NUM) {
>>          dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>>          goto fail;
>>      }
>>      ...
>> }
>>
>> If the device queue num is greater then the max queue size supported by seabios,
>> seabios tries to reduce the queue size, then re-read it again, I suppose to
>> check if the setting actually happens, and then checks the virtqueue size again,
>> to deside whether it is satisfied with the vaule.
>> In this case, if device's virtqueue size is 512 and seabios max supported queue
>> size is 256, seabios tries to set 256 but than read 512 again and can't proceed
>> with that vaule, preventing the guest from successful booting.
>> The root case was investigated by Roman Kagan <rkagan@virtuozzo.com>
>>
>> The patch fixes the problem, by propagating the queue size to the device right
>> away, so the written value could be read on the next step, if the value was
>> ok for the device.
>>
>> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Thanks, I already have this queued as:
>
> commit 8aabbbd9d04f95d5581d2275362996ecb5516dd9
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Fri Dec 13 09:22:48 2019 -0500
>
>      virtio: update queue size on guest write
>      
>      Some guests read back queue size after writing it.
>      Update the size immediatly upon write otherwise
>      they get confused.
>      
>      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> I would appreciate checking other transports, they likely
> need the same fix.
ok, I'll send the patch shortly
>
>
>> ---
>>   hw/virtio/virtio-pci.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index c6b47a9c73..e5c759e19e 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1256,6 +1256,8 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>>           break;
>>       case VIRTIO_PCI_COMMON_Q_SIZE:
>>           proxy->vqs[vdev->queue_sel].num = val;
>> +        virtio_queue_set_num(vdev, vdev->queue_sel,
>> +                             proxy->vqs[vdev->queue_sel].num);
>>           break;
>>       case VIRTIO_PCI_COMMON_Q_MSIX:
>>           msix_vector_unuse(&proxy->pci_dev,
>> -- 
>> 2.17.0