[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
I think I still have some comments. Posted them on list.
On Thu, Mar 05, 2020 at 05:16:59PM +0000, Vitaly Mireyno wrote:
> 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]