Re: [virtio] [PATCH v10 04/10] admin: introduce virtio admin virtqueues

[Jiri Pirko <jiri@nvidia.com>]

Mon, Mar 06, 2023 at 07:40:38PM CET, mst@redhat.com wrote:
>On Mon, Mar 06, 2023 at 01:41:30PM +0100, Jiri Pirko wrote:
>> Thu, Mar 02, 2023 at 02:05:06PM CET, mst@redhat.com wrote:
>> >The admin virtqueues will be the first interface to issue admin commands.
>> >
>> >Currently virtio specification defines control virtqueue to manipulate
>> >features and configuration of the device it operates on. However,
>> >control virtqueue commands are device type specific, which makes it very
>> >difficult to extend for device agnostic commands.
>> >
>> >To support this requirement in a more generic way, this patch introduces
>> >a new admin virtqueue interface.
>> >
>> >We also support more than one admin virtqueue, for QoS and
>> >scalability requirements.
>> >
>> >Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >---
>> > admin.tex   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > content.tex |  7 +++--
>> > 2 files changed, 79 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/admin.tex b/admin.tex
>> >index 7e28b77..3ffac2e 100644
>> >--- a/admin.tex
>> >+++ b/admin.tex
>> >@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>> > \field{command_specific_data} and \field{command_specific_result}
>> > depends on these structures and is described separately or is
>> > implicit in the structure description.
>> >+
>> >+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
>> >+
>> >+An administration virtqueue of an owner device is used to submit
>> >+group administration commands. An owner device can have more
>> 
>> I admit I'm confused. You introduce a concept of admin virtqueue, which
>> sounds quite generic to me, usable in future by much more things than
>> "device groups", correct?
>> 
>> If yes, here you say "group administration commands" which contradics
>> that idea. On multiple places the text this patchset introduces
>> this very muych tights to groups. Like in struct virtio_admin_cmd
>> which contains fields very specific to groups.
>> 
>> If no, isn't it a mistake as the admin queue should be here to
>> handle more than just group commands?
>
>For now, no.

Hmm, if not for now, the future exension would not be so simple, I fear.


>
>Passing commands to devices themselves is already covered in spec
>reasonably well though not in a generic way.

You mean using the control queue, correct?

From one of the patch description of this patchset I understand that you
cannot use control queue for this because control queue is
device-specific, yet group control is device-agnostic.

My undestanding therefore was, that the admin queue you are introducing
serves as a generic carrier for device-agnostic commands, in parallel
for having control queue serving as a generic carrier of device-specific
commands. If this is not the case, I think it would be nice to describe
the exact monivation and scope of admin queue.


>
>What we lack is passing commands about one device to another device.
>E.g. control VFs through PFs.

Could you provide examples of such commands please?


>This is what groups do.
>But if we see more uses we can always add them.
>
>
>I'd rather avoid being too generic though.

In that case, why not to avoid using generic terms and stay
"group-centric"? What I mean is:
"Administration Virtqueues" -> "Group Administration Virtqueues"
"struct virtio_admin_cmd" -> "struct virtio_group_admin_cmd"

Etc. Helps to avoid confusion.


>
>
>
>
>> 
>> >+than one administration virtqueue.


[...]