RE: [EXT] Re: [PATCH v8] virtio-net: Add support for the flexible driver notification structure.

[Vitaly Mireyno <vmireyno@marvell.com>]

>
>On 2020/8/31 äå5:16, Vitaly Mireyno wrote:
>> When the driver is required to send an available buffer notification to the device, it sends the
>virtqueue number to be notified.
>> With this new feature, the device can optionally provide a per-virtqueue value for the driver to use in
>driver notifications, instead of the virtqueue number.
>> Some devices may benefit from this flexibility by providing, for example, an internal virtqueue
>identifier, or an internal offset related to the virtqueue number.
>>
>> Changes from v6:
>>   * Changed the feature definition such that the value, that the
>> driver sets in the vqn field, is per-virtqueue
>>
>> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
>> ---
>>   content.tex | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/content.tex b/content.tex index 91735e3..09043e6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -824,6 +824,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio
>Transport
>>           le64 queue_desc;                /* read-write */
>>           le64 queue_driver;              /* read-write */
>>           le64 queue_device;              /* read-write */
>> +        le16 queue_notify_data;         /* read-only for driver */
>
>
>Do we need to mention this field only exists when VIRTIO_NET_F_NOTIF_CONFIG_DATA is
>negotiated?
>

Will add.

>
>> +        le16 padding;
>
>
>Any reason for intruding the padding here?
>

I thought of a dword alignment, but reading again the spec seems that it's not required. Will remove.

>
>>   };
>>   \end{lstlisting}
>>
>> @@ -890,6 +892,18 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio
>Transport
>>
>>   \item[\field{queue_device}]
>>           The driver writes the physical address of Device Area here.  See section \ref{sec:Basic Facilities
>of a Virtio Device / Virtqueues}.
>> +
>> +\item[\field{queue_notify_data}]
>> +        If VIRTIO_NET_F_NOTIF_CONFIG_DATA has been negotiated, the driver will use this value
>
>
>The feature should work for other types of devices, so there's probably
>no need for "NET" prefix.
>

Ok. Will move it to the device independent features section ("6 Reserved Feature Bits")

>
>> +        to put it in the 'virtqueue number' field in the available buffer notification structure.
>> +        See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And
>Device Operation / Available Buffer Notifications}.
>> +        \begin{note}
>> +        This field provides the device with flexibility to determine how virtqueues
>> +        will be referred to in available buffer notifications.
>> +        In a trivial case the device can set \field{queue_notify_data}=vqn. Some devices
>> +        may benefit from providing another value, for example an internal virtqueue
>> +        identifier, or an internal offset related to the virtqueue number.
>> +        \end{note}
>>   \end{description}
>>
>>   \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options /
>Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
>> @@ -940,7 +954,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio
>Transport
>>
>>   \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},
>\field{config_generation} or \field{queue_notify_off}.
>> +The driver MUST NOT write to \field{device_feature}, \field{num_queues},
>\field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
>>
>>   If VIRTIO_F_RING_PACKED has been negotiated,
>>   the driver MUST NOT write the value 0 to \field{queue_size}.
>> @@ -1519,6 +1533,10 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport
>Option
>>   See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification
>capability}
>>   for how to calculate the Queue Notify address.
>>
>> +\drivernormative{\paragraph}{Available Buffer Notifications}{Virtio Transport Options / Virtio Over
>PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}
>> +If VIRTIO_NET_F_NOTIF_CONFIG_DATA has been negotiated, the driver MUST set the 'virtqueue
>number'
>> +field of the available buffer notification structure to the \field{queue_notify_data} value.
>> +
>>   \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus /
>PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>>
>>   If a used buffer notification is necessary for a virtqueue, the device would typically act as follows:
>> @@ -2895,6 +2913,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device /
>Feature bits
>>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>>
>> +\item[VIRTIO_NET_F_NOTIF_CONFIG_DATA(57)] Driver uses the data provided by the
>> +    device as a virtqueue identifier in available buffer notifications.
>> +
>>   \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact \field{hdr_len}
>>       value. Device benefits from knowing the exact header length.
>>
>> @@ -4180,6 +4201,28 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   See \ref{sec:Basic
>>   Facilities of a Virtio Device / Virtqueues / Message Framing}.
>>
>> +\subsubsection{Driver Notifications}\label{sec:Device Types / Network Device / Device Operation /
>Driver Notifications}
>> +As mentioned in section \ref{sec:Virtqueues / Driver notifications}, when the
>> +driver is required to send an available buffer notification to the device, it
>> +sends the virtqueue number to be notified. The method of delivering notifications
>> +is transport specific.
>> +With the PCI transport, the device can optionally provide a per-virtqueue value
>> +for the driver to use in driver notifications, instead of the virtqueue number.
>> +Some devices may benefit from this flexibility by providing, for example,
>> +an internal virtqueue identifier, or an internal offset related to the virtqueue number.
>> +
>> +The VIRTIO_NET_F_NOTIF_CONFIG_DATA feature indicates the availability of such value.
>> +The definition of the data to be provided by the driver in driver notification and
>> +the method for delivering it is transport specific.
>> +
>> +For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio
>Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>> +
>> +\drivernormative{\paragraph}{Driver Notifications}{Device Types / Network Device / Device
>Operation / Driver Notifications}
>> +The driver SHOULD accept the VIRTIO_NET_F_NOTIF_CONFIG_DATA feature if it has
>> +been offered.
>> +
>> +If the VIRTIO_NET_F_NOTIF_CONFIG_DATA feature has been negotiated, the driver MUST provide
>the transport-specific value for the virtqueue number with driver notifications.
>> +
>
>
>Do we need to clarify that the NOTIFICATION_DATA and NOTIF_CONFIG_DATA
>is mutually exclusive?
>

I donât see them as mutually exclusive.
NOTIF_CONFIG_DATA means that instead of the vq number, the driver will use other per-vq value in driver notifications.
If NOTIFICATION_DATA has not been negotiated,  it will be just that value.
If NOTIFICATION_DATA has been negotiated, it will be vqn field in the notification struct.
(according to "4.1.5.2 Available Buffer Notifications")
Does this make sense?

>Thanks
>
>
>>   \section{Block Device}\label{sec:Device Types / Block Device}
>>
>>   The virtio block device is a simple virtual block device (ie.
>> --
>>