[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v3] virtio-net: Add support for the flexible driver notification structure
If we agree on the latest comment, I'd like to request a TC vote.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/73
>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Sent: Tuesday, 3 March, 2020 16:48
>To: Jason Wang <jasowang@redhat.com>
>Cc: Vitaly Mireyno <vmireyno@marvell.com>; virtio-comment@lists.oasis-open.org; Ariel Elior
><aelior@marvell.com>
>Subject: [EXT] Re: [virtio-comment] [PATCH v3] virtio-net: Add support for the flexible driver
>notification structure
>
>External Email
>
>----------------------------------------------------------------------
>On Thu, Feb 27, 2020 at 06:20:43PM +0800, Jason Wang wrote:
>>
>> On 2020/2/27 äå6:11, Michael S. Tsirkin wrote:
>> > On Thu, Feb 27, 2020 at 06:05:30PM +0800, Jason Wang wrote:
>> > > On 2020/2/27 äå1:10, Michael S. Tsirkin wrote:
>> > > > On Wed, Feb 26, 2020 at 02:54:36PM +0000, Vitaly Mireyno wrote:
>> > > > > > -----Original Message-----
>> > > > > > From: Jason Wang <jasowang@redhat.com>
>> > > > > > Sent: Wednesday, 26 February, 2020 5:13
>> > > > > > 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: [EXT] Re: [virtio-comment] [PATCH v3] virtio-net:
>> > > > > > Add support for the flexible driver notification structure
>> > > > > >
>> > > > > > ------------------------------------------------------------
>> > > > > > ----------
>> > > > > >
>> > > > > > On 2020/2/26 äå12:30, 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 v2 - Defined a new feature flag instead of a PCI-specific flag.
>> > > > > > >
>> > > > > > > Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
>> > > > > > > ---
>> > > > > > > content.tex | 34 ++++++++++++++++++++++++++++++++++
>> > > > > > > 1 file changed, 34 insertions(+)
>> > > > > > >
>> > > > > > > diff --git a/content.tex b/content.tex index
>> > > > > > > b91a132..5223d5c 100644
>> > > > > > > --- a/content.tex
>> > > > > > > +++ b/content.tex
>> > > > > > > @@ -965,6 +965,8 @@ \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 */
>> > > > > > > + le16 padding; /* Pad to a dword */
>> > > > > > > };
>> > > > > > > \end{lstlisting}
>> > > > > > >
>> > > > > > > @@ -984,6 +986,21 @@ \subsubsection{Notification structure
>> > > > > > > layout}\label{sec:Virtio Transport
>> > > > > > Options
>> > > > > > > the same Queue Notify address for all queues.
>> > > > > > > \end{note}
>> > > > > > >
>> > > > > > > +\field{notify_data} is the data that the driver will set
>> > > > > > > +in the \field{vqn} field in the available buffer
>> > > > > > > +notification, if VIRTIO_NET_F_NOTIF_EXTRA_DATA has been negotiated.
>> > > > > > > +
>> > > > > > > +\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.
>> > > > > > > +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,10 @@ \subsubsection{Notification
>> > > > > > > structure layout}\label{sec:Virtio Transport
>> > > > > > Options
>> > > > > > > cap.length >= queue_notify_off * notify_off_multiplier + 4
>> > > > > > > \end{lstlisting}
>> > > > > > >
>> > > > > > > +If VIRTIO_NET_F_NOTIF_EXTRA_DATA has been negotiated, the
>> > > > > > > +device MUST set \field{notify_data} to a valid value, and
>> > > > > > > +SHOULD set \field{notify_off_multiplier} > 0.
>> > > > > > > +
>> > > > > > > \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
>> > > > > > > +1540,14 @@ \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} The driver SHOULD accept
>> > > > > > > +the
>> > > > > > VIRTIO_NET_F_NOTIF_EXTRA_DATA feature if it has been offered.
>> > > > > >
>> > > > > >
>> > > > > > I think we need use "MUST" here?
>> > > > > >
>> > > > > Wouldnât it break the existing drivers then?
>> > > > I agree, we can't declare existing drivers non-conformant by fiat.
>> > > > Certainly not for a boutique feature like this.
>> > >
>> > > I may miss something but I think device won't work correctly if
>> > > this feature is not negotiated?
>> > >
>> > > Thanks
>> > That would depend on the device. Device can always fail FEATURES_OK
>> > if it doesn't support existing drivers.
>> > I really need to address
>> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_oasi
>> > s-2Dtcs_virtio-2Dspec_issues_71&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=
>> > lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=LDvPTNJX7kgpZPkoFNWGKG
>> > NTXAM0O4Q3fzG7WFpKpBs&s=11aXD7si-Zae-HAENc1Af9X1mFBhJ27f_8CkXMBDcVQ&
>> > e=
>> > so people know what to expect ...
>>
>>
>> That's my understanding as well.
>>
>> E.g for IOMMU_PLATFORM, device can still work if it the feature is not
>> negotiated so we use "SHOULD" in the spec.
>>
>> But for this feature, there's no way except for failing the
>> negotiation, so "MUST" seems correct.
>>
>> Thanks
>
>That would depend on the device. Some devices could work without the extra data in the kick, just
>with worse performance.
>So I feel this is device's job to enforce this bit, not driver's.
>
>
>>
>> >
>> > > >
>> > > > > > Other looks good to me.
>> > > > > >
>> > > > > > Thanks
>> > > > > >
>> > > > > >
>> > > > > > > +
>> > > > > > > +If VIRTIO_NET_F_NOTIF_EXTRA_DATA has been negotiated, 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:
>> > > > > > > @@ -2895,6 +2924,10 @@ \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_EXTRA_DATA(57)] Driver provides an extra data with
>> > > > > > > + available buffer notifications, to aid in notification processing by the
>> > > > > > > + device.
>> > > > > > > +
>> > > > > > > \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact \field{hdr_len}
>> > > > > > > value. Device benefits from knowing the exact header length.
>> > > > > > >
>> > > > > > > @@ -2934,6 +2967,7 @@ \subsubsection{Feature bit
>> > > > > > > requirements}\label{sec:Device Types /
>> > > > > > Network Device
>> > > > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
>> > > > > > > \item[VIRTIO_NET_F_RSC_EXT] Requires
>> > > > > > > VIRTIO_NET_F_HOST_TSO4 or
>> > > > > > VIRTIO_NET_F_HOST_TSO6.
>> > > > > > > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
>> > > > > > > +\item[VIRTIO_NET_F_NOTIF_EXTRA_DATA] Requires VIRTIO_F_NOTIFICATION_DATA.
>> > > > > > > \end{description}
>> > > > > > >
>> > > > > > > \subsubsection{Legacy Interface: Feature
>> > > > > > > bits}\label{sec:Device Types / Network Device / Feature
>> > > > > > > bits / Legacy Interface: Feature bits}
>> > > > > > > --
>> > > > > > >
>> > > > > > > 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=nKjWec2b6R0mOy
>> > > > > > > Paz7xtfQ&r=l
>> > > > > > > DHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=94BRvyqIARqVJ
>> > > > > > > -yydsIqG25_z
>> > > > > > > AUHjeENww-NxeMs_s8&s=fspKS4uA_CyM-S4HrR6GdJbvttjkX5iCygZvc
>> > > > > > > ybe_qs&e=
>> > > > > > > Feedback License:
>> > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>> > > > > > > org_who_ipr_feedback-5Flicense.pdf&d=DwIFaQ&c=nKjWec2b6R0m
>> > > > > > > OyPaz7xtfQ&r
>> > > > > > > =lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=94BRvyqIARq
>> > > > > > > VJ-yydsIqG25
>> > > > > > > _zAUHjeENww-NxeMs_s8&s=sskV_Q0nV6odmu9tb3_QO4sJgxQQfA7SDNk
>> > > > > > > DbhCPgZ0&e=
>> > > > > > > List Guidelines:
>> > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>> > > > > > > org_policies-2Dguidelines_mailing-2Dlists&d=DwIFaQ&c=nKjWe
>> > > > > > > c2b6R0mOyPaz
>> > > > > > > 7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=94BR
>> > > > > > > vyqIARqVJ-yy
>> > > > > > > dsIqG25_zAUHjeENww-NxeMs_s8&s=0FDuoPUSirLgEwBf7ATkyVMaalFe
>> > > > > > > B6N0-vhpOnkB
>> > > > > > > vWY&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=94BRvyqIARqVJ-yydsIqG25
>> > > > > > > _zAUHjeENww-
>> > > > > > > NxeMs_s8&s=raC15yZMnr9y9Iv5_woorP7Bq2OQiO-uybPhgqzNtlE&e=
>> > > > > > > Join OASIS:
>> > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.
>> > > > > > > org_join_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3
>> > > > > > > lqqsArgFRdce
>> > > > > > > vq01tbLQAw4A_NO7xgI&m=94BRvyqIARqVJ-yydsIqG25_zAUHjeENww-N
>> > > > > > > xeMs_s8&s=Gw VYWNwNZr7MFb5jJ7acLULJni1uMlwLgWBgiTSrp74&e=
>> > > > > > >
>> > > > 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
>> > > > -2Dopen.org_archives_virtio-2Dcomment_&d=DwIFaQ&c=nKjWec2b6R0mOy
>> > > > Paz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=LDvPTNJ
>> > > > X7kgpZPkoFNWGKGNTXAM0O4Q3fzG7WFpKpBs&s=pLW8-vLMi_pYMcFoNY71JqtNG
>> > > > GEInPoJ3OecdJGzJPA&e= Feedback License:
>> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2
>> > > > Dopen.org_who_ipr_feedback-5Flicense.pdf&d=DwIFaQ&c=nKjWec2b6R0m
>> > > > OyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=LDvPT
>> > > > NJX7kgpZPkoFNWGKGNTXAM0O4Q3fzG7WFpKpBs&s=Q8tWMaY4fmsA80CHo8Ke6R_
>> > > > PLNPpYczxZ5PLzWMvBIw&e= List Guidelines:
>> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2
>> > > > Dopen.org_policies-2Dguidelines_mailing-2Dlists&d=DwIFaQ&c=nKjWe
>> > > > c2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&
>> > > > m=LDvPTNJX7kgpZPkoFNWGKGNTXAM0O4Q3fzG7WFpKpBs&s=gHtMpYgytUdsH8Ip
>> > > > bYJ8vbBrMtkaNEWG0DAbQoKhMgo&e=
>> > > > Committee:
>> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2
>> > > > Dopen.org_committees_virtio_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r
>> > > > =lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=LDvPTNJX7kgpZPkoF
>> > > > NWGKGNTXAM0O4Q3fzG7WFpKpBs&s=WyGi--MvQmA96NSjT5qWVV_xqf5ciKNbAS-
>> > > > WNOgn8aU&e= Join OASIS:
>> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2
>> > > > Dopen.org_join_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3
>> > > > lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=LDvPTNJX7kgpZPkoFNWGKGNTXAM0O4
>> > > > Q3fzG7WFpKpBs&s=4dD8Zd9yZWf0NN_ZJWlXvchshGMUr31-If12fuQyIXY&e=
>> > > >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]