Re: [virtio-comment] [PATCH v2] virtio-net: Add support for the flexible driver notification

* Re: [virtio-comment] [PATCH v2] virtio-net: Add support for the flexible driver notification
@ 2020-02-25 10:25 Vitaly Mireyno
  0 siblings, 0 replies; 5+ messages in thread
From: Vitaly Mireyno @ 2020-02-25 10:25 UTC (permalink / raw)
  To: Jason Wang, virtio-comment; +Cc: Michael S. Tsirkin, Ariel Elior

>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Sent: Tuesday, 25 February, 2020 11:57
>To: Vitaly Mireyno <vmireyno@marvell.com>; virtio-comment@lists.oasis-open.org
>Cc: Michael S. Tsirkin <mst@redhat.com>; Ariel Elior <aelior@marvell.com>
>Subject: Re: [virtio-comment] [PATCH v2] virtio-net: Add support for the flexible driver
>notification
>
>----------------------------------------------------------------------
>
>On 2020/2/25 下午5:50, Vitaly Mireyno wrote:
>>> -----Original Message-----
>>> From: Jason Wang <jasowang@redhat.com>
>>> Sent: Tuesday, 25 February, 2020 5:56
>>> To: Vitaly Mireyno <vmireyno@marvell.com>;
>>> virtio-comment@lists.oasis-open.org
>>> Cc: Michael S. Tsirkin <mst@redhat.com>; Ariel Elior
>>> <aelior@marvell.com>
>>> Subject: Re: [virtio-comment] [PATCH v2] virtio-net: Add support for
>>> the flexible driver notification
>>>
>>> ---------------------------------------------------------------------
>>> -
>>>
>>> On 2020/2/25 上午1:32, Vitaly Mireyno wrote:
>>>> Currently, the driver notification (available buffer notification) has a fixed structure.
>>>> If VIRTIO_F_NOTIFICATION_DATA has been negotiated, it includes: vqn, next_off and next_wrap.
>>>> If notify_off_multiplier > 0, the VQ number can be derived by the
>>>> device from the Queue Notify
>>> address, so vqn may be redundant.
>>>> Some devices benefit from receiving an additional data with driver
>>>> notifications. This data can
>>> optionally replace the vqn field in the driver notification structure.
>>>> In its simplest form, it would be sufficient for this data to be a per-device constant value.
>>>>
>>>> Changes from v1 - added more details about motivation and usage examples.
>>>>
>>>> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
>>>> ---
>>>>    content.tex | 28 ++++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex index b91a132..5abff30 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -965,6 +965,9 @@ \subsubsection{Notification structure
>>>> layout}\label{sec:Virtio Transport
>>> Options
>>>>    struct virtio_pci_notify_cap {
>>>>            struct virtio_pci_cap cap;
>>>>            le32 notify_off_multiplier; /* Multiplier for
>>>> queue_notify_off. */
>>>> +        le16 notify_data; /* Data to be placed in the vqn field */
>>>> +        u8 notify_data_valid;
>>>> +        u8 padding; /* Pad to a dword */
>>>
>>> I wonder whether we need a new feature flag then we can make it use for transport other than PCI.
>>>
>>> E.g using MMIO transport for vDPA seems attractive anyhow.
>>>
>> I didn't define a feature flag because driver notifications delivery method is transport-specific, and
>looks like not all transports support per-virtqueue notification address. According to the spec, MMIO
>supports a single notification address for all virtqueues. Or am I missing something?
>
>
>No, it's not implemented right now. But it's not hard to extend MMIO to support what PCI did right
>now.
>

OK. I can try defining a new feature flag, instead of the 'notify_data_valid' flag.

>
>>
>>>>    };
>>>>    \end{lstlisting}
>>>>
>>>> @@ -984,6 +987,20 @@ \subsubsection{Notification structure layout}\label{sec:Virtio Transport
>>> Options
>>>>    the same Queue Notify address for all queues.
>>>>    \end{note}
>>>>
>>>> +If \field{notify_data_valid} != 0, the driver will set the
>>>> +\field{vqn} field in the available buffer notification structure to the \field{notify_data} value.
>>>> +
>>>> +\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. Some devices benefit from receiving
>>>> +the additional data with driver notifications. An example could be a
>>>> +hardware device implementing multiple protocols (with virtio being
>>>> +one of them), so extra notification data could serve as a notification type indication or a protocol
>>> indication.
>>>
>>>
>>> A silly question. If virtqueue number can be derived by the device, device should already know itself
>a
>>> virtio device. Then the type indication seems redundant?
>>>
>> It's the other way around - the device can derive the virtqueue number *because* it knows that this
>is a virtio notification type.
>
>
>Ok, I get here. But it looks to me the device should know the protocols
>before its datapath is started. Or does the device support changing
>protocol dynamically (or what happen when driver write the vqn as a
>indication of other protocol?)
>

The device supports multiple datapath protocols, even simultaneously, and the only way for device to know the protocol, which driver wants to address, is by setting a specific value in the notification. There is no per-datapath-queue protocol configuration.
If a driver will set an illegal value in the notification data,  it may result in an unexpected HW behavior, but the possible damage is local and is limited to the guests own VQ.

>Thanks
>
>
>>
>>> Thanks
>>>
>>>
>>>> +Another example could be using shared hardware memory space for
>>>> +driver notifications for multiple virtio devices in a trusted environment.
>>>> +\end{note}
>>>> +
>>>>    \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI
>Bus
>>> / PCI Device Layout / Notification capability}
>>>>    The device MUST present at least one notification capability.
>>>>
>>>> @@ -1020,6 +1037,12 @@ \subsubsection{Notification structure layout}\label{sec:Virtio Transport
>>> Options
>>>>    cap.length >= queue_notify_off * notify_off_multiplier + 4
>>>>    \end{lstlisting}
>>>>
>>>> +The device MAY set \field{notify_data_valid} to any non-zero value,
>>>> +to request the driver to set the \field{vqn} to the \field{notify_data} value.
>>>> +If the device sets \field{notify_data_valid} to a non-zero value, it
>>>> +MUST set \field{notify_data} to a valid value.
>>>> +For a normal operation, the device MUST set \field{notify_data_valid} to zero.
>>>> +
>>>>    \subsubsection{ISR status capability}\label{sec:Virtio Transport
>>>> Options / Virtio Over PCI Bus / PCI Device Layout / ISR status
>>>> capability}
>>>>
>>>>    The VIRTIO_PCI_CAP_ISR_CFG capability @@ -1519,6 +1542,11 @@
>>>> \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_F_NOTIFICATION_DATA has been negotiated, and if \field{notify_data_valid} != 0, the
>>> driver MUST set the \field{vqn} field of the available buffer notification structure to the
>>> \field{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:
>>>> --
>>>>
>>>> This publicly archived list offers a means to provide input to the
>>>> OASIS Virtual I/O Device (VIRTIO) TC.
>>>>
>>>> In order to verify user consent to the Feedback License terms and to
>>>> minimize spam in the list archive, subscription is required before
>>>> posting.
>>>>
>>>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>>>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>>>> List help: virtio-comment-help@lists.oasis-open.org
>>>> List archive:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.oasis-2Dope
>>>> n.org_archives_virtio-2Dcomment_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=l
>>>> DHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=nDdtxAy8Qznyvn6eq1lEP3O85
>>>> a_hVYSZJf3U0pmhgaU&s=7amNB2nX03xQEMxgpPK7BmhHosQBVr6HJpuEUnhn1E4&e=
>>>> Feedback License:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>>>> org_who_ipr_feedback-5Flicense.pdf&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r
>>>> =lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=nDdtxAy8Qznyvn6eq1lEP3O
>>>> 85a_hVYSZJf3U0pmhgaU&s=PRasEhPUaotk0G2EgWYkXmZ908KA_72aHaZeflXvUW8&e=
>>>> List Guidelines:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>>>> org_policies-2Dguidelines_mailing-2Dlists&d=DwIFaQ&c=nKjWec2b6R0mOyPaz
>>>> 7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=nDdtxAy8Qznyvn6e
>>>> q1lEP3O85a_hVYSZJf3U0pmhgaU&s=aeDJlcDhDKq8zdJDrQbWJxMe7M5I46bMgY65-LM2
>>>> TJ0&e=
>>>> Committee:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>>>> org_committees_virtio_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ
>>>> 3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=nDdtxAy8Qznyvn6eq1lEP3O85a_hVYSZJf3
>>>> U0pmhgaU&s=mUqbxBUIdH8VtDtV-PKgzs5F9ROtm8ONLCb-ydDqtUo&e=
>>>> Join OASIS:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>>>> org_join_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdce
>>>> vq01tbLQAw4A_NO7xgI&m=nDdtxAy8Qznyvn6eq1lEP3O85a_hVYSZJf3U0pmhgaU&s=fs
>>>> H_p1KFJVckuBq7Nem9HTLqDp7HTPxwTgHrTs4DJJE&e=
>>>>

^ permalink raw reply	[flat|nested] 5+ messages in thread