Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit

[Cornelia Huck]

On Tue, 28 Apr 2015 10:16:04 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Tue, Apr 28, 2015 at 10:04:15AM +0200, Cornelia Huck wrote:
> > On Tue, 28 Apr 2015 09:14:07 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Tue, Apr 28, 2015 at 02:13:20PM +0800, Jason Wang wrote:
> > > > 
> > > > 
> > > > On Tue, Apr 28, 2015 at 1:13 PM, Michael S. Tsirkin <address@hidden> 
> > > > wrote:
> > > > >On Tue, Apr 28, 2015 at 11:14:04AM +0800, Jason Wang wrote:
> > > > >>     On Mon, Apr 27, 2015 at 7:05 PM, Michael S. Tsirkin
> > > > >><address@hidden> wrote:
> > > > >> >On Thu, Apr 23, 2015 at 02:21:41PM +0800, Jason Wang wrote:
> > > > >> >> This patch introduces a bus specific queue limitation. It will be
> > > > >> >> useful for increasing the limit for one of the bus without
> > > > >>disturbing
> > > > >> >> other buses.
> > > > >> >> Cc: Michael S. Tsirkin <address@hidden>
> > > > >> >> Cc: Alexander Graf <address@hidden>
> > > > >> >> Cc: Richard Henderson <address@hidden>
> > > > >> >> Cc: Cornelia Huck <address@hidden>
> > > > >> >> Cc: Christian Borntraeger <address@hidden>
> > > > >> >> Cc: Paolo Bonzini <address@hidden>
> > > > >> >> Signed-off-by: Jason Wang <address@hidden>
> > > > >> >> Reviewed-by: Cornelia Huck <address@hidden>
> > > > >> >
> > > > >> >Is this still needed if you drop the attempt to
> > > > >> >keep the limit around for old machine types?
> > > > >> If we agree to drop, we probably need transport specific macro.
> > > > >
> > > > >You mean just rename VIRTIO_PCI_QUEUE_MAX to VIRTIO_QUEUE_MAX?
> > > > >Fine, why not.
> > > > 
> > > > I mean keeping VIRTIO_PCI_QUEUE_MAX for pci only and just increase pci
> > > > limit. And introduce e.g VIRTIO_PCI_QUEUE_CCW for ccw and keep it as 64.
> > > > Since to my understanding, it's not safe to increase the limit for all 
> > > > other
> > > > transports which was pointed out by Cornelia in V1:
> > > > http://permalink.gmane.org/gmane.comp.emulators.qemu/318245.
> > > 
> > > I think all you need is add a check to CCW_CMD_SET_IND:
> > > limit to 64 for legacy interrupts only.
> > 
> > It isn't that easy.
> > 
> > What is easy is to add a check to the guest driver that fails setup for
> > devices with more than 64 queues not using adapter interrupts.
> > 
> > On the host side, we're lacking information when interpreting
> > CCW_CMD_SET_IND (the command does not contain a queue count, and the
> > actual number of virtqueues is not readily available.)
> 
> Why isn't it available? All devices call virtio_add_queue
> as appropriate. Just fail legacy adaptors.

Because we don't know what the guest is going to use? It is free to
use per-subchannel indicators, even if it is operating in virtio-1 mode.

> 
> > We also can't
> > fence off when setting up the vqs, as this happens before we know which
> > kind of indicators the guest wants to use.
> > 
> > More importantly, we haven't even speced what we want to do in this
> > case. Do we want to reject SET_IND for devices with more than 64
> > queues? (Probably yes.)
> > 
> > All this involves more work, and I'd prefer to do Jason's changes
> > instead as this gives us some more time to figure this out properly.
> > 
> > And we haven't even considered s390-virtio yet, which I really want to
> > touch as little as possible :)
> 
> Well this patch does touch it anyway :)

But only small, self-evident changes.

> For s390 just check and fail at init if you like.

What about devices that may change their number of queues? I'd really
prefer large queue numbers to be fenced off in the the individual
devices, and for that they need to be able to grab a transport-specific
queue limit.