Re: [PATCH] admin: Enable driver to send admin commands before DRIVER_OK

[Zhu Lingshan <lingshan.zhu@amd.com>]

On 9/11/2024 11:37 AM, Parav Pandit wrote:
>
>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Wednesday, September 11, 2024 2:06 AM
>>
>> On Tue, Sep 10, 2024 at 07:01:08PM +0000, Parav Pandit wrote:
>>>
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Wednesday, September 11, 2024 12:02 AM
>>>>
>>>> On Tue, Sep 10, 2024 at 08:07:23PM +0300, Parav Pandit wrote:
>>>>> Currently the driver can operate administration commands using
>>>>> administration virtqueues. Administration virtqueues must be
>>>>> enabled before it reached DRIVER_OK stage similar to all other
>>>>> queues. This is a limiting factor.
>>>>>
>>>>> Many of the device functionalties needs to be discovered and
>>>>> configured early enough before the driver reaches the DRIVER_OK
>> stage.
>>>>> Some examples are:
>>>>>
>>>>> a. driver wants to dynamically create the virtqueues of virtio-net
>>>>>    device with more parameters, for example header data split,
>>>>>    multiple physical addresses.
>>>>>    Here, the driver needs to tell PCI device early enough that it no
>>>>>    longer uses queue_* registers for non admin queues.
>>>>>
>>>>> b. driver wants to discover these features and related attributes
>>>>>    early enough so it has chance to decide to proceed via admin
>>>>>    cmd interface or proceed to DRIVER_OK and follow the current flow.
>>>>>
>>>>> To overcome these limitations, introduce a feature bit that
>>>>> enables the driver to send capabilities related admin commands
>>>>> before DRIVER_OK via the available interface such as administration
>> virtqueue.
>>>>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>>>>
>>>> We were there in 1.0 and it's messy. And next thing you do, you will
> Also if you can explain what exactly is the mess, if there is one?
As far as I can see, the spec says:
Set the DRIVER_OK status bit. At this point the device is “live”.

And no vqs are active before DRIVER_OK. How can the device assume that
the driver is ready before DRIVER_OK? Why admin vq is special?
The common config space is not enough to init a device?
If you see a gap, how about fix it in the common cfg?

Thanks

>
>>>> want it before features ok, and we will keep piling up hacks.
>>> I don't see any motivation to do before feature bit.
>> I do. 
> Can you please share the motivation, why one wants to enable the AQ before features_ok?
> And how would it configure all the basic things to operate a AQ before, for example, PACKED, VERSION, PLATFORM?
>
> If the motivation to negotiate device specific features via admin command, then that is still possible with current proposal.
>
>> And I keep saying, we need to be proactive, address more than one
>> problem with each feature and try to predict what will be needed,
>> otherwise it will be a mess of a million conflicting features.
>>
> Sure. make sense.
>
>>> Never before features ok, feature negotiation is two ways communication
>> that indicates device, that queue notification is coming.
>>>> I think that if we want
>>>> to do admin commands before VQs are active, we should just add a
>> capability
>>>> with registers for that. People wanted that anyway.
>>>>
>>> We discussed before that pci-sig discourages piling up vendor specific caps.
>>> So better to avoid that.
>> Well, that's a separate issue. Actually pci endpoint guys already want
>> a way to access device without messing up with capabilities.
>> So we'll likely create a way to put capabilities in a BAR.
>>
> So lets take the base line design requirement and see what more options do we have.
>
> a. do not extend pci capability
> b. utilize the current q config registers
> c. have bidirectional negotiation if driver will use the AQ early enough or not.
>
> With that what do you propose?
> A new register in the BAR instead of feature bits?
> How does the driver tell the device after reset and before features ok?
>
>>> An alternative would be to bump up the pci revision_id, which would align
>> to pci-sig and its known early enough.
>>> Though it does not take away the feature bit because device preparation is
>> needed to setup early admin commands.
>>> And feature bit can be a good hint to inform that.
>>> Only revision_id (from device to driver) is not good enough as there can be
>> large sw stack which may never use it and no point in enabling this in the
>> device without driver using it.
>>> So feature bit proposed here is self-contained, and its early enough.
>>
>> I don't really get what this revision_id discussion is about.
>>
> I will pause explaining the issue with rev_id for a moment and see what is the efficient way than proposed?
>  
>>>>
>>>>
>>>>
>>>>> ---
>>>>>  admin.tex         | 16 ++++++++++++++++
>>>>>  content.tex       | 13 ++++++++++++-
>>>>>  transport-pci.tex |  6 ++++++
>>>>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/admin.tex b/admin.tex
>>>>> index 39fc072..95054ed 100644
>>>>> --- a/admin.tex
>>>>> +++ b/admin.tex
>>>>> @@ -334,6 +334,11 @@ \subsection{Group administration
>>>>> commands}\label{sec:Basic Facilities of a Virti  supporting multiple
>>>>> group types, the list of supported commands  might differ between
>>>> different group types.
>>>>> +When the driver has negotiated the feature
>>>>> +VIRTIO_F_EARLY_CAP_ADMIN_CMD, the driver can use administration
>>>>> +commands VIRTIO_ADMIN_CMD_LIST_QUERY,
>>>> VIRTIO_ADMIN_CMD_LIST_USE and
>>>>> +commands related to device and driver capabilities listed in
>> \ref[sec:Basic
>>>> Facilities of a Virtio Device / Device groups / Group administration
>> commands
>>>> / Device and driver capabilities]{device and driver capabilities} for the self-
>>>> group type before the driver indicates DRIVER_OK status to the device.
>>>>> +
>>>>>  \input{admin-cmds-legacy-interface.tex}
>>>>>  \input{admin-cmds-capabilities.tex}
>>>>>  \input{admin-cmds-resource-objects.tex}
>>>>> @@ -608,6 +613,14 @@ \section{Administration
>>>>> Virtqueues}\label{sec:Basic Facilities of a Virtio Devic  or
>>>>> VIRTIO_ADMIN_STATUS_ENOMEM, then the command MUST NOT
>> have
>>>> any side effects, making it safe to retry.
>>>>> +If VIRTIO_F_EARLY_CAP_ADMIN_CMD feature is negotiated, the device
>>>>> +MUST process administration commands related to device and driver
>>>>> +capabilities before the driver indicates DRIVER_OK to the device.
>>>>> +
>>>>> +If VIRTIO_F_ADMIN_VQ and VIRTIO_F_EARLY_CAP_ADMIN_CMD
>> features
>>>> are
>>>>> +negotiated, the device MUST be able to generate notifications related
>>>>> +to the administration virtqueue before the driver indicates DRIVER_OK
>> to
>>>> the device.
>>>>> +
>>>>>  \drivernormative{\subsection}{Group administration commands}{Basic
>>>>> Facilities of a Virtio Device / Administration virtqueues}
>>>>>
>>>>>  The driver MAY supply device-readable or device-writeable parts @@
>>>>> -641,3 +654,6 @@ \section{Administration Virtqueues}\label{sec:Basic
>>>>> Facilities of a Virtio Devic
>>>>>
>>>>>  The driver SHOULD retry a command that completed with
>> \field{status}
>>>>> VIRTIO_ADMIN_STATUS_EAGAIN.
>>>>> +
>>>>> +When the VIRTIO_F_EARLY_ADMIN_CMD feature is negotiated, the
>> driver
>>>>> +MAY send commands only to the first administration queue defined by
>>>> the specific transport.
>>>>> diff --git a/content.tex b/content.tex index c32c218..12f9224 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -540,6 +540,8 @@ \section{Device Initialization}\label{sec:General
>>>> Initialization And Device Oper
>>>>>     device, optional per-bus setup, reading and possibly writing the
>>>>>     device's virtio configuration space, and population of virtqueues.
>>>>>
>>>>> +\item\label{itm:General Initialization And Device Operation / Device
>>>> Initialization / Capabilities Access} Optionally get device capabilities and
>> set
>>>> driver capabilities if VIRTIO_F_EARLY_CAP_ADMIN_CMD is negotiated.
>>>>> +
>>>>>  \item\label{itm:General Initialization And Device Operation / Device
>>>> Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point
>> the
>>>> device is
>>>>>     ``live''.
>>>>>  \end{enumerate}
>>>>> @@ -550,7 +552,9 @@ \section{Device Initialization}\label{sec:General
>>>>> Initialization And Device Oper  driver MUST NOT continue initialization
>> in
>>>> that case.
>>>>>  The driver MUST NOT send any buffer available notifications to -the
>>>>> device before setting DRIVER_OK.
>>>>> +the device before setting DRIVER_OK; however, the driver MAY send
>>>>> +buffer available notifications before setting DRIVER_OK if
>>>>> +VIRTIO_F_EARLY_CAP_ADMIN_CMD is negotiated.
>>>>>
>>>>>  \subsection{Legacy Interface: Device
>>>>> Initialization}\label{sec:General Initialization And Device Operation
>>>>> / Device Initialization / Legacy Interface: Device Initialization}  Legacy
>>>> devices did not support the FEATURES_OK status bit, and thus did @@ -
>> 879,6
>>>> +883,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature
>>>> Bits}
>>>>>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
>>>> for
>>>>>  	handling features reserved for future use.
>>>>>
>>>>> +  \item[VIRTIO_F_EARLY_CAP_ADMIN_CMD(42)] This feature indicates
>>>> that the device
>>>>> +   exposes an administration command interface which is accessible to
>> the
>>>> driver
>>>>> +   before the driver indicates DRIVER_OK device status. When this
>> feature
>>>> is
>>>>> +   negotiated, once the the administration commands interface is
>>>> configured, it can be
>>>>> +   used by the driver to issue administration commands related to
>> device
>>>> and driver
>>>>> +   capabilities.
>>>>> +
>>>>>  \end{description}
>>>>>
>>>>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
>>>>> Bits} diff --git a/transport-pci.tex b/transport-pci.tex index
>>>>> 95b08b8..a612b34 100644
>>>>> --- a/transport-pci.tex
>>>>> +++ b/transport-pci.tex
>>>>> @@ -495,6 +495,12 @@ \subsubsection{Common configuration
>> structure
>>>>> layout}\label{sec:Virtio Transport  to ensure that indices of valid
>>>>> admin queues fit into  a 16 bit range beyond all other virtqueues.
>>>>>
>>>>> +If VIRTIO_F_ADMIN_VQ and VIRTIO_F_EARLY_ADMIN_CMD has been
>>>>> +negotiated, the device MUST support administration commands
>> through
>>>>> +the administration virtqueue identified by the
>>>>> +\field{admin_queue_index} after the administration virtqueue is
>>>>> +enabled and before the driver sets DRIVER_OK to the
>>>> \field{device_status}.
>>>>> +
>>>>>  \drivernormative{\paragraph}{Common configuration structure
>>>>> layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device
>>>>> Layout / Common configuration structure layout}
>>>>>
>>>>>  The driver MUST NOT write to \field{device_feature},
>>>>> \field{num_queues},
>>>>> --
>>>>> 2.34.1
>