Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 10:18:12AM +0100, Stefan Hajnoczi wrote:
> On Tue, Nov 05, 2019 at 03:56:43PM -0500, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> > > @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy 
> > > *vpci_dev, Error **errp)
> > >      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> > >      DeviceState *proxy = DEVICE(vpci_dev);
> > >      char *bus_name;
> > > +    bool modern = virtio_pci_modern(vpci_dev);
> > > +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> > > +
> > > +    if (!modern && virtqueue_size > 128) {
> > > +        error_setg(errp,
> > > +                   "too big virtqueue size (%u, max: 128) "
> > > +                   "for non-modern virtio device", virtqueue_size);
> > > +        return;
> > > +    }
> > 
> > why? what is illegal about 256 for legacy?
> 
> I think it was mentioned that this limit is specific to SeaBIOS
> src/hw/virtio-pci.c:vp_find_vq():
> 
>   #define MAX_QUEUE_NUM      (128)
>   ...
>   if (num > MAX_QUEUE_NUM) {
>       dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>       goto fail;
>   }

OK I see. It's worth documenting this (with version of seabios
that has the issue).
And yes virtio_pci_modern will not do the right thing.
This checks whether device has the modern interface, but
an old seabios will not use the modern interface even
if it's there.

You want to start with small queues and then check after features have
been negotiated with firmware, and make them bigger.


But even then I am not so sure we should just block
bigger queues by default. kernel can use bigger queues fine,
and not all disks are necessarily used by seabios.


If you want a user friendly option, we can add a flag that tells
qemu to adjust the size to a known safe value.
Then we'd start small and make it bigger if guest is a modern one.




> I'm not sure there is anything we can do in QEMU.  Either you can let
> SeaBIOS fail, or if you want something more user-friendly, then the
> management tool can implement a check based on the SeaBIOS version and
> the -device virtio-blk-pci,queue-size=SIZE property value.
> 
> Stefan