RE: [virtio-comment] RE: [PATCH v5 2/7] Introduce admin command set

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, May 17, 2022 7:48 AM
> 
> On Mon, May 16, 2022 at 09:08:34PM +0000, Parav Pandit wrote:
> > Hi Michael,
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Sunday, May 15, 2022 11:24 AM
> >
> > [..]
> >
> > > > +\subsection{VIRTIO ADMIN DEVICE CAPS ACCEPT
> > > command}\label{sec:Basic
> > > > +Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN
> > > > +DEVICE CAPS ACCEPT command}
> > > > +
> > > > +The VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT command is used by the
> > > driver to acknowledge those admin capabilities it understands and
> > > wishes to use.
> > >
> > >
> > > ok so we have a protocol here, kind of like feature negotiation.
> > > Please write its description.
> > > e.g. is it ok to change accepted caps? when? can device change its
> > > caps etc etc etc.
> > >
> > > Avoiding this kind of spec work is exactly why me and jason keep
> > > telling you to consider just using features instead. Add a 64 bit
> > > admin features field to the PCI transport and be done with it. CCW
> > > and MMIO already have feature selector so it's trivial to add feature bits.
> > >
> > As we begin to scale with the device, adding more and more registers like
> this demands more on-device real estate to comply to the PCI standards.
> >
> > And therefore, things are queried/accessed rare or occasionally, are better
> accessed via a queue interface.
> >
> > One can argue that admin VQ is proposed only for the mgmt. functions so
> having this cfg register for PF is enough.
> >
> > However, AQ may find some usage in the VF/SF themselves down the
> road.
> > Hence, keeping the cap exchange transport this way is more optimal.
> >
> > Max has called out this AQ rationale in 4 or 5 points in the cover letter.
> 
> Hmm. It's kind of a generic claim though. 
I am sorry for my way late response.

Not sure what you mean by generic claim.
If you mean generic for any PCI device, than yes, at scale such large register scale doesn't work well.

> We never put devices on a diet
> trying to conserve registers. 
> There is cost associated with this dance and that
> is driver boot time.
How much more time you anticipate by adding an additional queue which slows down the boot time?
We don't see downtime increasing beyond in < 2 msec range.

And AQ is not used anyway until the actual AQ work is done.
With Q_RESET in place, driver won't even create the AQ during boot time anyway.

> 
> I also don't really understand how you can claim you need to save memory
> like this and at the same time blindly add a more or less "just in case" misc
> config in the config space.
> So, not pretty.
Adding to config space was really your suggestion. :)

There are multiple points described in cover letter that I prefer not to repeat here. It was not only memory.

Also, capabilities and fields around that are expected to grow more than q_index.
So not a right comparison.

> 
> And as I said, you will need much more spec work to reach the level to which
> features are specified - and note we are not yet happy with how features are
Can you be specific of the work that you are expecting in this v5 version?

> specified either! So it's a moving target.
> 
> Maybe put this in features for now, and leave the whole capability thing for
> another day?
> 
> --
> MST

It appears a narrow view to do temporary work on putting in features bits that doesn't scale.