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 <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.
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