Re: [PATCH v3 1/4] Add virtio Admin virtqueue

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Feb 08 2022, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:

> On 2/8/2022 8:45 AM, Michael S. Tsirkin wrote:
>> On Tue, Feb 08, 2022 at 02:41:44AM +0200, Max Gurtovoy wrote:
>>> On 2/7/2022 6:18 PM, Michael S. Tsirkin wrote:
>>>> On Mon, Feb 07, 2022 at 04:58:19PM +0200, Max Gurtovoy wrote:
>>>>> On 2/7/2022 12:39 PM, Michael S. Tsirkin wrote:
>>>>>> On Thu, Feb 03, 2022 at 09:57:13AM +0200, Max Gurtovoy wrote:
>>>>>>> +\begin{lstlisting}
>>>>>>> +struct virtio_admin_cmd {
>>>>>>> +        /* Device-readable part */
>>>>>>> +        u16 command;
>>>>>>> +        u8 command_specific_data[];
>>>>>>> +
>>>>>>> +        /* Device-writable part */
>>>>>>> +        u8 status;
>>>>>>> +        u8 command_specific_error;
>>>>>>> +        u8 command_specific_result[];
>>>>>>> +};
>>>>>>> +\end{lstlisting}
>>>>>> ok this abstraction is an improvement, thanks!
>>>>>>
>>>>>> What I'd like to see is moving a bit more format to this generic structure.
>>>>>>
>>>>>>    From what I could gather, some commands affect a group as a whole, and
>>>>>> some commands just a single member of the group. We could have a
>>>>>> "destination" field for that, and a special "all of the group"
>>>>>> destination for commands affecting the whole group.
>>>>>>
>>>>>>
>>>>>> Next, trying to think about scalable iov extensions. So we
>>>>>> will have groups of VFs and then SFs as the next level.
>>>>>> How does one differentiate between the two?
>>>>>> Maybe reserve a field for "destination type"?
>>>>> For now we have only a PCI group that composed of VFs and the PF.
>>>>>
>>>>> What you suggest, IMO is a definition of a generic virtio group/subsystem
>>>>> that I've mentioned in the discussion of V1.
>>>>>
>>>>> Once we have virtio group - it should have a group id and them the admin
>>>>> command can have a field calld group_id for commands that are targeted to
>>>>> the whole group.
>>>>>
>>>>> Some commands are referring to a specific device in the group so only a
>>>>> vdev_id is needed.
>>>>>
>>>>> Some commands are even targeted to the same device to query some info (we
>>>>> have examples in this series for that), so in this case there is no need for
>>>>> vdev_id nor group_id.
>>>>>
>>>>> So I'm sure sure we can improve common virtio_admin_cmd structure to have
>>>>> these attributes since they are not mandatory because of the reasons I've
>>>>> mentioned.
>>>> I'm not sure I understand 100%, but try to address in the next
>>>> revision and we'll discuss.
>>> I meant to say that I'm *not* sure we can improve the common structure...
>>>
>>> It was a typo.
>>>
>>> And I don't understand why this info can't be in the command_specific_data
>>> because of all the reasons I mentioned above.
>> It can, but as declared admin commands are there to handle
>> groups of VFs, so let's standardize how they refer to groups.
>
> "Admin virtqueue is one of the management interface that used to send administrative
> commands to manipulate various features of the *device* and/or to manipulate
> various features, if possible, of *another device* within the same group"
>
> It's not only for groups.

What I don't understand: Why can't we simply add target information to
the common definition? If a certain command doesn't need that target
information, it is simply ignored.

The benefit of reserving a field for target information and specifying
how groups and devices are supposed to be addressed is that not every
command will have to roll their own.