Re: [PATCH 2/5] Introduce VIRTIO_F_ADMIN_VQ_INDIRECT_DESC/VIRTIO_F_ADMIN_VQ_IN_ORDER

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 1/25/2022 5:52 AM, Parav Pandit wrote:
> Hi Jason,
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Tuesday, January 25, 2022 8:59 AM
>>
>> 在 2022/1/19 下午12:48, Parav Pandit 写道:
>>>> From: Jason Wang <jasowang@redhat.com>
>>>> Sent: Wednesday, January 19, 2022 9:33 AM
>>>>
>>>>
>>>> It it means IMS, there's already a proposal[1] that introduce MSI
>>>> commands via the admin virtqueue. And we had similar requirement for
>>>> virtio-MMIO[2] and managed device or SF [3], so I would rather to
>>>> introduce IMS (need a better name though) as a basic facility instead
>>>> of tie it to any specific transport.
>>>>
>>> IMS of [1] is a interrupt configuration by the virtio driver for the device is it
>> driving, which needs a queue.
>>> So regardless of the device type as PCI PF/VF/SF/ADI, there is desire to have a
>> generic admin queue not attached to device type.
>>> And AQ in this proposal exactly serves this purpose.
>>>
>>> Device configuring its own IMS vector vs PCI PF configuring VF's MSI-X max
>> vector count are two different functionality.
>>> Both of these commands can ride on a generic queue.
>>> However the queue is not same, because PF owns its own admin queue
>>> (for vf msix config), VF or SF operates its own admin queue (for IMS
>>> config).
>>
>> So I think in the next version we need to clarify:
>>
>> 1) is there a single admin virtqueue shared by all the VFs and PF
>>
>> or
>>
>> 2) per VF/PF admin virtqueue, and how does the driver know how to find the
>> corresponding admin virtqueue
>>
> Admin queue is not per VF.
> Lets take concrete examples.
> 1. So for example, PCI PF can have one AQ.
> This AQ carries command to query/config MSI-X vector of VFs.
>
> 2. In second example, PCI PF is creating/destroying SFs. This is again done by using the AQ of the PCI PF.
>
> 3. A PCI VF has its own AQ to configure some of its own generic attribute, don't know which is that today.
> May be something that is extremely hard to do over features bit.
> Currently proposed v2 doesn't restrict admin queue to be within PCI PF or VF or that matter not limited to other transports.
>   
>>> So a good example is,
>>> 1. PCI PF configures 8 MSI-X or 16 IMS vectors for the VF using PF_AQ in HV.
>>> 2. PCI VF when using IMS configures, IMS data, vector, mask etc using VF_AQ
>> in GVM.
>>> Both the functions will have AQ feature bit set.
>>
>> Where did the VF_AQ sit? I guess it belongs to the VF. But if this is
>> true, don't we need some kind of address isolation like PASID?
>>
> Above one for IMS is not a good example. I replied the reasoning last week for it.
>   
>>> Fair enough, so we have more users of admin queue than just MSI-X config.
>>
>> Well, what I really meant is that we actually have more users of IMS.
>> That's is exactly what virito-mmio wants. In this case introducing admin
>> queue looks too heavyweight for that.
>>
> IMS config cannot be done over AQ as described in previous email in this thread.
>
>>>>> 2. AQ to follows IN_ORDER and INDIRECT_DESC negotiation like rest of
>>>>> the queues 3. Update commit log to describe why config space is not
>>>>> chosen (scale, on-die registers, uniform way to handle all aq cmds)
>>>> I fail to understand the scale/registeres issues. With the one of my previous
>>>> proposal (device selector), technically we don't even need any config space
>> or
>>>> BAR for VF or SF by multiplexing the registers for PF.
>>>>
>>> Scale issue is: when you want to create, query, manipulate hundreds of
>> objects, having shared MMIO register or configuration register, will be too
>> slow.
>>
>>
>> Ok, this need to be clarified in the commit log. And we need make sure
>> it's not an issue that is only happen for some specific vendor.
> It is present in the v2 commit log cover letter.
> Please let me know if you think it should be in the actual patch commit log.
>
>
>>> And additionally such register set doesn't scale to allow sharing large
>> number of bytes as DMA cannot be done.
>>
>>
>> That's true.
>>
>>
>>>   From physical device perspective, it doesn’t scale because device needs to
>> have those resources ready to answer on MMIO reads and for hundreds to
>> thousand of devices it just cannot do it.
>>> This is one of the reason for birth of IMS.
>>
>> IMS allows the table to be stored in the memory and cached by the device
>> to have the best scalability. But I had other questions:
>>
>> 1) if we have a single admin virtqueue, there will still be contention
>> in the driver side
>>
> AQ inherently allows out of order commands execution.
> It shouldn't face contention. For example 1K depth AQ should be serving hundreds of descriptors commands in parallel for SF creation, VF MSI-X config and more.
>
> Which area/commands etc you think can lead to the contention?
>   
>> 2) if we have per vf admin virtqueue, it still doesn't scale since it
>> occupies more hardware resources
>>
> That is too heavy, and doesn’t scale. Proposal is to not have per vf admin queue.
> Proposal is to have one admin queue in a virtio device.

Right ? where did we mention something that can imply otherwise ?


>   
>>>> I do see one advantage is that the admin virtqueue is transport
>> independent
>>>> (or it could be used as a transport).
>>>>
>>> I am yet to read the transport part from [1].
>>
>> Yes, the main goal is to be compatible with SIOV.
>>
> Admin queue is a command interface transport where higher layer services can be buit.
> This includes SR-IOV config, SIOV config.
> And v2 enables SIOV commands implementation whenever they are ready.
>
>>>>> 4. Improve documentation around msix config to link to sriov section of
>> virtio
>>>> spec
>>>>> 5. Describe error that if VF is bound to the device, admin commands
>>>> targeting VF can fail, describe this error code
>>>>> Did I miss anything?
>>>>>
>>>>> Yet to receive your feedback on group, if/why is it needed and, why/if it
>> must
>>>> be in this proposal, what pieces prevents it do as follow-on.
>>>>> Cornelia, Jason,
>>>>> Can you please review current proposal as well before we revise v2?
>>>> If I understand correctly, most of the features (except for the admin
>>>> virtqueue in_order stuffs) are not specific to the admin virtqueue. As
>>>> discussed in the previous versions, I still think it's better:
>>>>
>>>> 1) adding sections in the basic device facility or data structure for
>>>> provisioning and MSI
>>>> 2) introduce admin virtqueue on top as an device interface for those
>>>> features
>>>>
>>> I didn't follow your suggestion. Can you please explain?
>>> Specifically "data structure for provisioning and MSI"..
>>
>> I meant:
>>
>> There's a chapter "Basic Facilities of a Virtio Device", we can
>> introduce the concepts there like:
>>
>> 1) Managed device and Management device (terminology proposed by
>> Michael), and can use PF and VF as a example
>>
>> 2) Managed device provisioning (the data structure to specify the
>> attributes of a managed device (VF))
>>
>> 3) MSI
>>
> Above is good idea. Will revisit v2, if it is not arranged this way.

Let me make sure I understand, you would like to see a new chapter under 
"Basic Facilities of a Virtio Device" that is

called "Device management" and this chapter will explain in few words 
the concept and it will point to another chapter under "Basic Facilities 
of a Virtio Device"

that was introduced here "Admin Virtqueues" ?

So you do agree that managing a managed (create/destroy/setup/etc...) 
will be done using the AQ of the managing device ?

>   
>> And then we can introduced admin virtqueue in either
>>
>> 1) transport part
>>
>> or
>>
>> 2) PCI transport
>>
> It is not specific to PCI transport, and currently it is not a transport either.
> So admin queue will keep as general entity for admin work.
>   
>> In the admin virtqueue, there will be commands to provision and
>> configure MSI.
>>
> Please review v2 if it is not arranged this way.
>
>>>> The leaves the chance for future extensions to allow those features to
>>>> be used by transport specific interface which will benefit for
>>>>
>>> AQ allows communication (command, response) between driver and device
>> in transport independent way.
>>> Sometimes it query/set transport specific fields like MSI-X vectors of VF.
>>> Sometimes device configure its on IMS interrupt.
>>> Something else in future.
>>> So it is really a generic request-response queue.
>>
>> I agree, but I think we can't mandate new features to a specific transport.
>>
> Certainly. Admin queue is transport independent.
> PCI MSI-X configuration is PCI transport specific command, so structures are defined it accordingly.
> It is similar to struct virtio_pci_cap, struct virtio_pci_common_cfg etc.
>
> Any other transport will have transport specific interrupt configuration. So it will be defined accordingly whenever that occurs.
> For example, IMS for VF or IMS for SF.