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

[Vitaly Mireyno <vmireyno@marvell.com>]

I can suggest changing the feature definition such that the value, that the driver sets in the vqn field, will be per-virtqueue (instead of per-device).
The device can provide a per-virtqueue value to the driver in the PCI Common configuration structure (virtio_pci_common_cfg).
This will probably make the feature more generic. It will also simplify the feature, as it will not depend on notify_off_multiplier anymore.

Would it make more sense?


>-----Original Message-----
>From: Cornelia Huck <cohuck@redhat.com>
>Sent: Thursday, 26 March, 2020 13:53
>To: Michael S. Tsirkin <mst@redhat.com>
>Cc: Vitaly Mireyno <vmireyno@marvell.com>; virtio-comment@lists.oasis-open.org; Jason Wang
><jasowang@redhat.com>; Ariel Elior <aelior@marvell.com>
>Subject: [EXT] Re: [PATCH v7] virtio-net: Add support for the flexible driver notification structure.
>
>External Email
>
>----------------------------------------------------------------------
>On Thu, 26 Mar 2020 07:26:16 -0400
>"Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Mar 26, 2020 at 10:43:20AM +0100, Cornelia Huck wrote:
>> > On Wed, 18 Mar 2020 18:29:19 +0000
>> > Vitaly Mireyno <vmireyno@marvell.com> wrote:
>
>> > > +
>> > > +\begin{note}
>> > > +If \field{notify_off_multiplier} > 0, the virtqueue number can
>> > > +potentially be derived by the device from the Queue Notify
>> > > +address, so \field{vqn} may be redundant.
>>
>> I feel this is confusing the reader more than it explains.
>> the field is always there. It's either the virtqueue number or
>> whatever we get from the device.
>
>"the virtqueue number can potentially be derived by the device from the Queue Notify address, so
>obtaining the virtqueue number from \field{vqn} is not needed"
>
>?
>
>>
>> > > +\end{note}
>> > > +
>(...)
>> > > @@ -4180,6 +4205,33 @@ \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 always 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 distinct per-virtqueue address for driver
>> > > +notifications (Queue Notify address). In this case, the virtqueue
>> > > +number can potentially be derived by the device from the Queue Notify address. For devices,
>that provide per-virtqueue Queue Notify addresses and are able to derive the virtqueue number from
>that address, the virtqueue number in the notification data is redundant.
>> >
>> > I would keep the remark that the queue number might be redundant
>> > with the PCI transport.
>>
>> I feel redundant is too strong a word. If anything sending data we
>> just read from device back to it is definitely redundant.
>> Spec says send it so we send it.
>> Maybe say it can be ignored by the device?
>
>Maybe "not needed"?
>
>(see above)
>
>>
>> > > +On the other hand, some devices can benefit from receiving
>> > > +additional data with driver notifications. An example could be a
>> > > +hardware device implementing multiple protocols (with virtio
>> > > +being one of them), so the extra notification data could serve as a notification type indication or
>a protocol indication.
>> >
>> > Hm... what about replacing the whole text above with
>> >
>> > "Driver notifications (\ref{sec:Virtqueues / Driver notifications})
>> > always contain at least the virtqueue number for which buffers are
>> > available. Some devices can benefit from receiving additional data
>> > with driver notifications. For example, a hardware device
>> > implementing multiple protocols (with virtio being one of them)
>> > could benefit from a notification type indication or a protocol indication."
>>
>>
>> My problem with both versions is that it talks about "additional
>> information" while in fact it's information that comes from the device
>> itself.
>
>"per-device information", maybe?