RE: [PATCH 1/5] Add virtio Admin Virtqueue specification

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, January 18, 2022 3:01 AM

> On Mon, Jan 17, 2022 at 11:56:09AM +0200, Max Gurtovoy wrote:
> > > > +Admin virtqueue is used to send administrative commands to
> > > > +manipulate various features of the device which would not easily
> > > > +map into the configuration space.
> > > IMHO this is too vague to be useful. E.g. I don't really see why
> > > would not commands specified in the next patch map to config space.
> >
> > Well I took this sentence from the current spec :)
> 
> Well in current spec it applies to things like MAC address filtering, which does
> not easily map into config space because number of MACs varies.

It doesn't well map to the config space for very primary reason that that it is read+write access that driver should be able to in async manner.
Yes, we will improve this part of the commit log to described that doing via AQ enables driver to not get blocked by previous outstanding command.

> 
> 
> >
> > >
> > >
> > > We had an off-list meeting where I proposed addressing one device
> > > from another or grouping multiple devices as a more specific scope.
> > > That would be one way to address this.
> >
> > Are you suggestion a creation of a virtio subsystem or a virtio group
> > definition ?
> >
> > Devices will be part of this subsystem: one primary/manager device and
> > many secondary/managed devices ?
> >
> > Each subsystem will have a unique UUID and each device will have a
> > unique vdev_id within this subsystem.
> >
> > If this is the direction, I can prepare something..
> 
> I was merely saying that what is special about admin queue is that it allows
> controlling one device from another within some group.
> Or maybe that it allows grouping multiple devices.
> *Not* that these are things that do not map to config space.
> 
> Let me give you another example, imagine that you want to handle pagefaults
> from device.  Clearly a generic thing that does not map to config space.  It
> could be a good candidate for the admin queue, however it would require that
> lots of buffers are pre-added to the queue. So it looks like it will beed another
> distinct fault queue.  
Right page fault queue is async queue located in hv and/or guest more like net device rq.
AQ is serving request-response queue.
Page fault queue likely needed multiple to have any reasonable bw, per cpu is one option.

> Further it is possible that you want to handle faults
> within guest, by the driver. In that case you do not want it in the admin queue
> since that is controlled by hypervisor, you want it in a separate queue
> controlled by driver.
> 
Yes. so it is better to not merge page fault queue with admin queue.

> 
> I don't recall discussion about UUID so I can't really say what I think about that.
> Do we need a UUID? I'm not sure I understand why.
> It can't hurt to abstract things a bit so it's not all tied to PFs/VFs since we know
> we'll want subfunctions down the road, too, if that is what you mean.
>
I still didn't find any reason in the discussion to find out why grouping device is needed.
Current AQ proposal implicitly indicates that VFs of a PF are managed by its parent PF.
And for some reason this work by one of the VF, this role assignment can be certainly a new command on AQ as group command or some other command.
 
> 
> 
> > >
> > > Following this idea, all commands would then gain fields for
> > > addressing one device from another.
> > >
> > > Not everything maps well to a queue. E.g. it would be great to have
> > > list of available commands in memory.
> >
> > I'm not sure I agree. Why can't it map to a queue ?
> 
> You can map it to a queue, yes. But something static and read only such as list
> of commands maps well to config space. And it's not controlling one device
> from another, so does not really seem to belong in the admin queue.
> 
Aq serves the writing device config too in patch-5 in this patchset.

> >
> > > Figuring out max vectors also looks like a good example for memory
> > > and not through a command.
> >
> > Any explanation why is it looks good ? or better ?
> 
> why is memory easier to operate than a VQ?
> It's much simpler and so less error prone.  you can have multiple actors read
> such a field at the same time without races, so e.g.  there could be a sysfs
> attribute that reads from device on each access, and not special error handling
> is needed.
>
Writing fields is inherent part of the aq without getting blocked on previous writes.
I see you acked that AQ is fine in cover letter patch as below, so we are sync on the motivation now.
Yes, will update the commit log as you suggested.

 " if the answer is "commands A,B,C do not fit in config space, we placed commands D,E in a VQ for consistency"
then that is an ok answer, but it's something to be mentioned in the commit log"