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

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

On 9/11/2024 12:38 PM, Jason Wang wrote:
> On Wed, Sep 11, 2024 at 12:12 PM Parav Pandit <parav@nvidia.com> wrote:
>>
>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>> Sent: Wednesday, September 11, 2024 9:30 AM
>>> To: Parav Pandit <parav@nvidia.com>; Michael S. Tsirkin <mst@redhat.com>
>>> Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; Shahaf Shuler
>>> <shahafs@nvidia.com>
>>> Subject: Re: [PATCH] admin: Enable driver to send admin commands before
>>> DRIVER_OK
>>>
>>>
>>>
>>> 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”.
>>>
>> This patch upgrades this detail. Please read through.
>>
>>> And no vqs are active before DRIVER_OK. How can the device assume that the
>>> driver is ready before DRIVER_OK?
>> Device does not assume.
>> When the feature described in this patch is negotiated, device knows that AQ will be live before DRIVER_OK.
>>
>>> Why admin vq is special?
>> Because it is used to do fundamental device and driver capabilities discovery.
> Then you need another transport or a control device.
So Maybe it is a good chance for me to continue the "transport vq" work?
>
> Thanks
>
>> Please read the commit log that describes the motivation.
>>
>>> The common config space is not enough to init a device?
>> No.
>>> If you see a gap, how about fix it in the common cfg?
>> Admin command is the interface do perform administration work.
>>
>>> 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