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 <address@hidden>
> 
> 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 <address@hidden>
> Suggested-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Denis Plotnikov <address@hidden>

Thanks, I already have this queued as:

commit 8aabbbd9d04f95d5581d2275362996ecb5516dd9
Author: Michael S. Tsirkin <address@hidden>
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 <address@hidden>

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