* [PATCH] virtio-net: Ignore num_buffers when unused @ 2024-12-26 5:37 Akihiko Odaki 2024-12-26 7:29 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Akihiko Odaki @ 2024-12-26 5:37 UTC (permalink / raw) To: virtio-comment Cc: Michael S. Tsirkin, Jason Wang, Parav Pandit, devel, Akihiko Odaki If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU sets num_buffers to zero while the specification tells to set it to one. Let drivers ignore the field to make them compatible with both vhost_net/QEMU and devices already compliant with the specification. Also, remove statements requiring devices to set the field to one to make vhost_net and QEMU compliant. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- device-types/net/description.tex | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/device-types/net/description.tex b/device-types/net/description.tex index 76585b0..b82fe8f 100644 --- a/device-types/net/description.tex +++ b/device-types/net/description.tex @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} -The device MUST set \field{num_buffers} to the number of descriptors used to -hold the incoming packet. +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set +\field{num_buffers} to the number of descriptors used to hold the incoming +packet. The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF was not negotiated. -\begin{note} -{This means that \field{num_buffers} will always be 1 -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} -\end{note} \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Processing incoming packets involves: \begin{enumerate} -\item \field{num_buffers} indicates how many descriptors - this packet is spread over (including this one): this will - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates + how many descriptors this packet is spread over (including this one). This allows receipt of large packets without having to allocate large buffers: a packet that does not fit in a single buffer can flow over to the next buffer, and so on. In this case, there will be @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network The other buffers will not begin with a struct virtio_net_hdr. \item If - \field{num_buffers} is one, then the entire packet will be - contained within this buffer, immediately following the struct - virtio_net_hdr. + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then + the entire packet will be contained within this buffer, immediately following + the struct virtio_net_hdr. \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be set: if so, device has validated the packet checksum. @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set -\field{num_buffers} to 1. - If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set \field{num_buffers} to indicate the number of buffers the packet (including the header) is spread over. @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network This is due to various bugs in implementations. \end{note} +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore +\field{num_buffers}. + If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT rely on the packet checksum being correct. --- base-commit: b495841a8e80d12c1130f8868f4128866291142d change-id: 20241226-reserved-d5960f173223 Best regards, -- Akihiko Odaki <akihiko.odaki@daynix.com> ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 5:37 [PATCH] virtio-net: Ignore num_buffers when unused Akihiko Odaki @ 2024-12-26 7:29 ` Michael S. Tsirkin 2024-12-26 7:32 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2024-12-26 7:29 UTC (permalink / raw) To: Akihiko Odaki; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU > sets num_buffers to zero while the specification tells to set it to one. > > Let drivers ignore the field to make them compatible with both > vhost_net/QEMU and devices already compliant with the specification. > Also, remove statements requiring devices to set the field to one to > make vhost_net and QEMU compliant. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > device-types/net/description.tex | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex > index 76585b0..b82fe8f 100644 > --- a/device-types/net/description.tex > +++ b/device-types/net/description.tex > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi > > \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} > > -The device MUST set \field{num_buffers} to the number of descriptors used to > -hold the incoming packet. > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set > +\field{num_buffers} to the number of descriptors used to hold the incoming > +packet. > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > was not negotiated. > -\begin{note} > -{This means that \field{num_buffers} will always be 1 > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} > -\end{note} > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} > \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > Processing incoming packets involves: > > \begin{enumerate} > -\item \field{num_buffers} indicates how many descriptors > - this packet is spread over (including this one): this will > - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates > + how many descriptors this packet is spread over (including this one). > This allows receipt of large packets without having to allocate large > buffers: a packet that does not fit in a single buffer can flow > over to the next buffer, and so on. In this case, there will be > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > The other buffers will not begin with a struct virtio_net_hdr. > > \item If > - \field{num_buffers} is one, then the entire packet will be > - contained within this buffer, immediately following the struct > - virtio_net_hdr. > + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then > + the entire packet will be contained within this buffer, immediately following > + the struct virtio_net_hdr. > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the > VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be > set: if so, device has validated the packet checksum. > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} > \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > -\field{num_buffers} to 1. > - > If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set > \field{num_buffers} to indicate the number of buffers > the packet (including the header) is spread over. > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > This is due to various bugs in implementations. > \end{note} > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore > +\field{num_buffers}. > + > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor > VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT > rely on the packet checksum being correct. There could be drivers relying on it. We can add a SHOULD not and ask drivers not to look at it, and we should fix devices. > --- > base-commit: b495841a8e80d12c1130f8868f4128866291142d > change-id: 20241226-reserved-d5960f173223 > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 7:29 ` Michael S. Tsirkin @ 2024-12-26 7:32 ` Michael S. Tsirkin 2024-12-26 7:35 ` Akihiko Odaki 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2024-12-26 7:32 UTC (permalink / raw) To: Akihiko Odaki; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: > On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: > > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU > > sets num_buffers to zero while the specification tells to set it to one. > > > > Let drivers ignore the field to make them compatible with both > > vhost_net/QEMU and devices already compliant with the specification. > > Also, remove statements requiring devices to set the field to one to > > make vhost_net and QEMU compliant. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > device-types/net/description.tex | 26 +++++++++++--------------- > > 1 file changed, 11 insertions(+), 15 deletions(-) > > > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex > > index 76585b0..b82fe8f 100644 > > --- a/device-types/net/description.tex > > +++ b/device-types/net/description.tex > > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi > > > > \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} > > > > -The device MUST set \field{num_buffers} to the number of descriptors used to > > -hold the incoming packet. > > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set > > +\field{num_buffers} to the number of descriptors used to hold the incoming > > +packet. > > > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > > was not negotiated. > > -\begin{note} > > -{This means that \field{num_buffers} will always be 1 > > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} > > -\end{note} > > > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > Processing incoming packets involves: > > > > \begin{enumerate} > > -\item \field{num_buffers} indicates how many descriptors > > - this packet is spread over (including this one): this will > > - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. > > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates > > + how many descriptors this packet is spread over (including this one). > > This allows receipt of large packets without having to allocate large > > buffers: a packet that does not fit in a single buffer can flow > > over to the next buffer, and so on. In this case, there will be > > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > The other buffers will not begin with a struct virtio_net_hdr. > > > > \item If > > - \field{num_buffers} is one, then the entire packet will be > > - contained within this buffer, immediately following the struct > > - virtio_net_hdr. > > + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then > > + the entire packet will be contained within this buffer, immediately following > > + the struct virtio_net_hdr. > > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the > > VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be > > set: if so, device has validated the packet checksum. > > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > > -\field{num_buffers} to 1. > > - > > If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set > > \field{num_buffers} to indicate the number of buffers > > the packet (including the header) is spread over. > > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > This is due to various bugs in implementations. > > \end{note} > > > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore > > +\field{num_buffers}. > > + > > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor > > VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT > > rely on the packet checksum being correct. > > > There could be drivers relying on it. > We can add a SHOULD not and ask drivers not to look at it, > and we should fix devices. Maybe allowing 0 too is a good idea? Need to think through implemenation concerns. In any case, pls fix devices. > > > --- > > base-commit: b495841a8e80d12c1130f8868f4128866291142d > > change-id: 20241226-reserved-d5960f173223 > > > > Best regards, > > -- > > Akihiko Odaki <akihiko.odaki@daynix.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 7:32 ` Michael S. Tsirkin @ 2024-12-26 7:35 ` Akihiko Odaki 2024-12-26 9:13 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Akihiko Odaki @ 2024-12-26 7:35 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On 2024/12/26 16:32, Michael S. Tsirkin wrote: > On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: >> On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: >>> If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU >>> sets num_buffers to zero while the specification tells to set it to one. >>> >>> Let drivers ignore the field to make them compatible with both >>> vhost_net/QEMU and devices already compliant with the specification. >>> Also, remove statements requiring devices to set the field to one to >>> make vhost_net and QEMU compliant. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> device-types/net/description.tex | 26 +++++++++++--------------- >>> 1 file changed, 11 insertions(+), 15 deletions(-) >>> >>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex >>> index 76585b0..b82fe8f 100644 >>> --- a/device-types/net/description.tex >>> +++ b/device-types/net/description.tex >>> @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi >>> >>> \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} >>> >>> -The device MUST set \field{num_buffers} to the number of descriptors used to >>> -hold the incoming packet. >>> +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set >>> +\field{num_buffers} to the number of descriptors used to hold the incoming >>> +packet. >>> >>> The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF >>> was not negotiated. >>> -\begin{note} >>> -{This means that \field{num_buffers} will always be 1 >>> -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} >>> -\end{note} >>> >>> \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} >>> \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff >>> @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>> Processing incoming packets involves: >>> >>> \begin{enumerate} >>> -\item \field{num_buffers} indicates how many descriptors >>> - this packet is spread over (including this one): this will >>> - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. >>> +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates >>> + how many descriptors this packet is spread over (including this one). >>> This allows receipt of large packets without having to allocate large >>> buffers: a packet that does not fit in a single buffer can flow >>> over to the next buffer, and so on. In this case, there will be >>> @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>> The other buffers will not begin with a struct virtio_net_hdr. >>> >>> \item If >>> - \field{num_buffers} is one, then the entire packet will be >>> - contained within this buffer, immediately following the struct >>> - virtio_net_hdr. >>> + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then >>> + the entire packet will be contained within this buffer, immediately following >>> + the struct virtio_net_hdr. >>> \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the >>> VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be >>> set: if so, device has validated the packet checksum. >>> @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>> \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} >>> \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff >>> >>> -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set >>> -\field{num_buffers} to 1. >>> - >>> If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set >>> \field{num_buffers} to indicate the number of buffers >>> the packet (including the header) is spread over. >>> @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>> This is due to various bugs in implementations. >>> \end{note} >>> >>> +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore >>> +\field{num_buffers}. >>> + >>> If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor >>> VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT >>> rely on the packet checksum being correct. >> >> >> There could be drivers relying on it. >> We can add a SHOULD not and ask drivers not to look at it, The patch adds the following statement: > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST > ignore \field{num_buffers}. Do you suggest changing MUST to SHOULD or rephrase other words in this statement? >> and we should fix devices. > > Maybe allowing 0 too is a good idea? Need to think through > implemenation concerns. > In any case, pls fix devices. I opted allowing any values for devices based on the discussion in the following thread: https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9 Regards, Akihiko Odaki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 7:35 ` Akihiko Odaki @ 2024-12-26 9:13 ` Michael S. Tsirkin 2024-12-26 11:09 ` Akihiko Odaki 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2024-12-26 9:13 UTC (permalink / raw) To: Akihiko Odaki; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On Thu, Dec 26, 2024 at 04:35:55PM +0900, Akihiko Odaki wrote: > On 2024/12/26 16:32, Michael S. Tsirkin wrote: > > On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: > > > On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: > > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU > > > > sets num_buffers to zero while the specification tells to set it to one. > > > > > > > > Let drivers ignore the field to make them compatible with both > > > > vhost_net/QEMU and devices already compliant with the specification. > > > > Also, remove statements requiring devices to set the field to one to > > > > make vhost_net and QEMU compliant. > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > --- > > > > device-types/net/description.tex | 26 +++++++++++--------------- > > > > 1 file changed, 11 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex > > > > index 76585b0..b82fe8f 100644 > > > > --- a/device-types/net/description.tex > > > > +++ b/device-types/net/description.tex > > > > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi > > > > \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} > > > > -The device MUST set \field{num_buffers} to the number of descriptors used to > > > > -hold the incoming packet. > > > > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set > > > > +\field{num_buffers} to the number of descriptors used to hold the incoming > > > > +packet. > > > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > > > > was not negotiated. > > > > -\begin{note} > > > > -{This means that \field{num_buffers} will always be 1 > > > > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} > > > > -\end{note} > > > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > > > \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > > > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > Processing incoming packets involves: > > > > \begin{enumerate} > > > > -\item \field{num_buffers} indicates how many descriptors > > > > - this packet is spread over (including this one): this will > > > > - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. > > > > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates > > > > + how many descriptors this packet is spread over (including this one). > > > > This allows receipt of large packets without having to allocate large > > > > buffers: a packet that does not fit in a single buffer can flow > > > > over to the next buffer, and so on. In this case, there will be > > > > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > The other buffers will not begin with a struct virtio_net_hdr. > > > > \item If > > > > - \field{num_buffers} is one, then the entire packet will be > > > > - contained within this buffer, immediately following the struct > > > > - virtio_net_hdr. > > > > + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then > > > > + the entire packet will be contained within this buffer, immediately following > > > > + the struct virtio_net_hdr. > > > > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the > > > > VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be > > > > set: if so, device has validated the packet checksum. > > > > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > > > \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > > > > -\field{num_buffers} to 1. > > > > - > > > > If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set > > > > \field{num_buffers} to indicate the number of buffers > > > > the packet (including the header) is spread over. > > > > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > This is due to various bugs in implementations. > > > > \end{note} > > > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore > > > > +\field{num_buffers}. > > > > + > > > > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor > > > > VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT > > > > rely on the packet checksum being correct. > > > > > > > > > There could be drivers relying on it. > > > We can add a SHOULD not and ask drivers not to look at it, > > The patch adds the following statement: > > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST > > ignore \field{num_buffers}. > > Do you suggest changing MUST to SHOULD or rephrase other words in this > statement? Yes, I would change MUST to SHOULD. Basically documenting the existing practice. But I do not yes see why we should drop the device requirements the way you suggest. We just need to fix devices, it does not seem hard. > > > and we should fix devices. > > > > Maybe allowing 0 too is a good idea? Need to think through > > implemenation concerns. > > In any case, pls fix devices. > > I opted allowing any values for devices based on the discussion in the > following thread: > https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9 > > Regards, > Akihiko Odaki That one asks for concrete data. -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 9:13 ` Michael S. Tsirkin @ 2024-12-26 11:09 ` Akihiko Odaki 2024-12-26 11:55 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Akihiko Odaki @ 2024-12-26 11:09 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On 2024/12/26 18:13, Michael S. Tsirkin wrote: > On Thu, Dec 26, 2024 at 04:35:55PM +0900, Akihiko Odaki wrote: >> On 2024/12/26 16:32, Michael S. Tsirkin wrote: >>> On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: >>>> On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: >>>>> If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU >>>>> sets num_buffers to zero while the specification tells to set it to one. >>>>> >>>>> Let drivers ignore the field to make them compatible with both >>>>> vhost_net/QEMU and devices already compliant with the specification. >>>>> Also, remove statements requiring devices to set the field to one to >>>>> make vhost_net and QEMU compliant. >>>>> >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> --- >>>>> device-types/net/description.tex | 26 +++++++++++--------------- >>>>> 1 file changed, 11 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex >>>>> index 76585b0..b82fe8f 100644 >>>>> --- a/device-types/net/description.tex >>>>> +++ b/device-types/net/description.tex >>>>> @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi >>>>> \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} >>>>> -The device MUST set \field{num_buffers} to the number of descriptors used to >>>>> -hold the incoming packet. >>>>> +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set >>>>> +\field{num_buffers} to the number of descriptors used to hold the incoming >>>>> +packet. >>>>> The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF >>>>> was not negotiated. >>>>> -\begin{note} >>>>> -{This means that \field{num_buffers} will always be 1 >>>>> -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} >>>>> -\end{note} >>>>> \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} >>>>> \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff >>>>> @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>>>> Processing incoming packets involves: >>>>> \begin{enumerate} >>>>> -\item \field{num_buffers} indicates how many descriptors >>>>> - this packet is spread over (including this one): this will >>>>> - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. >>>>> +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates >>>>> + how many descriptors this packet is spread over (including this one). >>>>> This allows receipt of large packets without having to allocate large >>>>> buffers: a packet that does not fit in a single buffer can flow >>>>> over to the next buffer, and so on. In this case, there will be >>>>> @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>>>> The other buffers will not begin with a struct virtio_net_hdr. >>>>> \item If >>>>> - \field{num_buffers} is one, then the entire packet will be >>>>> - contained within this buffer, immediately following the struct >>>>> - virtio_net_hdr. >>>>> + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then >>>>> + the entire packet will be contained within this buffer, immediately following >>>>> + the struct virtio_net_hdr. >>>>> \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the >>>>> VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be >>>>> set: if so, device has validated the packet checksum. >>>>> @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>>>> \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} >>>>> \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff >>>>> -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set >>>>> -\field{num_buffers} to 1. >>>>> - >>>>> If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set >>>>> \field{num_buffers} to indicate the number of buffers >>>>> the packet (including the header) is spread over. >>>>> @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network >>>>> This is due to various bugs in implementations. >>>>> \end{note} >>>>> +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore >>>>> +\field{num_buffers}. >>>>> + >>>>> If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor >>>>> VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT >>>>> rely on the packet checksum being correct. >>>> >>>> >>>> There could be drivers relying on it. >>>> We can add a SHOULD not and ask drivers not to look at it, >> >> The patch adds the following statement: >>> If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST >>> ignore \field{num_buffers}. >> >> Do you suggest changing MUST to SHOULD or rephrase other words in this >> statement? > > Yes, I would change MUST to SHOULD. Basically documenting the existing > practice. > > But I do not yes see why we should drop the device requirements the way you suggest. > We just need to fix devices, it does not seem hard. Well, I think we have already discussed and decided not to change vhost_net: https://lore.kernel.org/all/CACGkMEt0spn59oLyoCwcJDdLeYUEibePF7gppxdVX1YvmAr72Q@mail.gmail.com/ > >>>> and we should fix devices. >>> >>> Maybe allowing 0 too is a good idea? Need to think through >>> implemenation concerns. >>> In any case, pls fix devices. >> >> I opted allowing any values for devices based on the discussion in the >> following thread: >> https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9 >> >> Regards, >> Akihiko Odaki > > That one asks for concrete data. It asks concrete data that motivates requiring setting the value to zero and I decided removing such a requirement instead. Regards, Akihiko Odaki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 11:09 ` Akihiko Odaki @ 2024-12-26 11:55 ` Michael S. Tsirkin 2024-12-27 1:26 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2024-12-26 11:55 UTC (permalink / raw) To: Akihiko Odaki; +Cc: virtio-comment, Jason Wang, Parav Pandit, devel On Thu, Dec 26, 2024 at 08:09:59PM +0900, Akihiko Odaki wrote: > On 2024/12/26 18:13, Michael S. Tsirkin wrote: > > On Thu, Dec 26, 2024 at 04:35:55PM +0900, Akihiko Odaki wrote: > > > On 2024/12/26 16:32, Michael S. Tsirkin wrote: > > > > On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: > > > > > On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: > > > > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU > > > > > > sets num_buffers to zero while the specification tells to set it to one. > > > > > > > > > > > > Let drivers ignore the field to make them compatible with both > > > > > > vhost_net/QEMU and devices already compliant with the specification. > > > > > > Also, remove statements requiring devices to set the field to one to > > > > > > make vhost_net and QEMU compliant. > > > > > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > > --- > > > > > > device-types/net/description.tex | 26 +++++++++++--------------- > > > > > > 1 file changed, 11 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex > > > > > > index 76585b0..b82fe8f 100644 > > > > > > --- a/device-types/net/description.tex > > > > > > +++ b/device-types/net/description.tex > > > > > > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi > > > > > > \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers} > > > > > > -The device MUST set \field{num_buffers} to the number of descriptors used to > > > > > > -hold the incoming packet. > > > > > > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set > > > > > > +\field{num_buffers} to the number of descriptors used to hold the incoming > > > > > > +packet. > > > > > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > > > > > > was not negotiated. > > > > > > -\begin{note} > > > > > > -{This means that \field{num_buffers} will always be 1 > > > > > > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} > > > > > > -\end{note} > > > > > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > > > > > \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > > > > > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > > > Processing incoming packets involves: > > > > > > \begin{enumerate} > > > > > > -\item \field{num_buffers} indicates how many descriptors > > > > > > - this packet is spread over (including this one): this will > > > > > > - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. > > > > > > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates > > > > > > + how many descriptors this packet is spread over (including this one). > > > > > > This allows receipt of large packets without having to allocate large > > > > > > buffers: a packet that does not fit in a single buffer can flow > > > > > > over to the next buffer, and so on. In this case, there will be > > > > > > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > > > The other buffers will not begin with a struct virtio_net_hdr. > > > > > > \item If > > > > > > - \field{num_buffers} is one, then the entire packet will be > > > > > > - contained within this buffer, immediately following the struct > > > > > > - virtio_net_hdr. > > > > > > + VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then > > > > > > + the entire packet will be contained within this buffer, immediately following > > > > > > + the struct virtio_net_hdr. > > > > > > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the > > > > > > VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be > > > > > > set: if so, device has validated the packet checksum. > > > > > > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > > > \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets} > > > > > > \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff > > > > > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > > > > > > -\field{num_buffers} to 1. > > > > > > - > > > > > > If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set > > > > > > \field{num_buffers} to indicate the number of buffers > > > > > > the packet (including the header) is spread over. > > > > > > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network > > > > > > This is due to various bugs in implementations. > > > > > > \end{note} > > > > > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore > > > > > > +\field{num_buffers}. > > > > > > + > > > > > > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor > > > > > > VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT > > > > > > rely on the packet checksum being correct. > > > > > > > > > > > > > > > There could be drivers relying on it. > > > > > We can add a SHOULD not and ask drivers not to look at it, > > > > > > The patch adds the following statement: > > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST > > > > ignore \field{num_buffers}. > > > > > > Do you suggest changing MUST to SHOULD or rephrase other words in this > > > statement? > > > > Yes, I would change MUST to SHOULD. Basically documenting the existing > > practice. > > > > But I do not yes see why we should drop the device requirements the way you suggest. > > We just need to fix devices, it does not seem hard. > > Well, I think we have already discussed and decided not to change vhost_net: > https://lore.kernel.org/all/CACGkMEt0spn59oLyoCwcJDdLeYUEibePF7gppxdVX1YvmAr72Q@mail.gmail.com/ Looking at this spec made me realize fixing that is less risky. I just replied on that thread and proposed a way to reduce risk even more. Sorry about leading you astray. > > > > > > > and we should fix devices. > > > > > > > > Maybe allowing 0 too is a good idea? Need to think through > > > > implemenation concerns. > > > > In any case, pls fix devices. > > > > > > I opted allowing any values for devices based on the discussion in the > > > following thread: > > > https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9 > > > > > > Regards, > > > Akihiko Odaki > > > > That one asks for concrete data. > > It asks concrete data that motivates requiring setting the value to zero and > I decided removing such a requirement instead. > > Regards, > Akihiko Odaki Same thing really. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Ignore num_buffers when unused 2024-12-26 11:55 ` Michael S. Tsirkin @ 2024-12-27 1:26 ` Jason Wang 0 siblings, 0 replies; 8+ messages in thread From: Jason Wang @ 2024-12-27 1:26 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Akihiko Odaki, virtio-comment, Parav Pandit, devel [-- Attachment #1: Type: text/plain, Size: 8019 bytes --] On Thu, Dec 26, 2024 at 7:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Dec 26, 2024 at 08:09:59PM +0900, Akihiko Odaki wrote: > > On 2024/12/26 18:13, Michael S. Tsirkin wrote: > > > On Thu, Dec 26, 2024 at 04:35:55PM +0900, Akihiko Odaki wrote: > > > > On 2024/12/26 16:32, Michael S. Tsirkin wrote: > > > > > On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote: > > > > > > On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote: > > > > > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net > and QEMU > > > > > > > sets num_buffers to zero while the specification tells to set > it to one. > > > > > > > > > > > > > > Let drivers ignore the field to make them compatible with both > > > > > > > vhost_net/QEMU and devices already compliant with the > specification. > > > > > > > Also, remove statements requiring devices to set the field to > one to > > > > > > > make vhost_net and QEMU compliant. > > > > > > > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > > > --- > > > > > > > device-types/net/description.tex | 26 > +++++++++++--------------- > > > > > > > 1 file changed, 11 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > diff --git a/device-types/net/description.tex > b/device-types/net/description.tex > > > > > > > index 76585b0..b82fe8f 100644 > > > > > > > --- a/device-types/net/description.tex > > > > > > > +++ b/device-types/net/description.tex > > > > > > > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive > Buffers}\label{sec:Device Types / Network Devi > > > > > > > \devicenormative{\paragraph}{Setting Up Receive > Buffers}{Device Types / Network Device / Device Operation / Setting Up > Receive Buffers} > > > > > > > -The device MUST set \field{num_buffers} to the number of > descriptors used to > > > > > > > -hold the incoming packet. > > > > > > > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set > > > > > > > +\field{num_buffers} to the number of descriptors used to hold > the incoming > > > > > > > +packet. > > > > > > > The device MUST use only a single descriptor if > VIRTIO_NET_F_MRG_RXBUF > > > > > > > was not negotiated. > > > > > > > -\begin{note} > > > > > > > -{This means that \field{num_buffers} will always be 1 > > > > > > > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.} > > > > > > > -\end{note} > > > > > > > \subsubsection{Processing of Incoming > Packets}\label{sec:Device Types / Network Device / Device Operation / > Processing of Incoming Packets} > > > > > > > \label{sec:Device Types / Network Device / Device Operation > / Processing of Packets}%old label for latexdiff > > > > > > > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming > Packets}\label{sec:Device Types / Network > > > > > > > Processing incoming packets involves: > > > > > > > \begin{enumerate} > > > > > > > -\item \field{num_buffers} indicates how many descriptors > > > > > > > - this packet is spread over (including this one): this will > > > > > > > - always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated. > > > > > > > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, > \field{num_buffers} indicates > > > > > > > + how many descriptors this packet is spread over (including > this one). > > > > > > > This allows receipt of large packets without having to > allocate large > > > > > > > buffers: a packet that does not fit in a single buffer > can flow > > > > > > > over to the next buffer, and so on. In this case, there > will be > > > > > > > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming > Packets}\label{sec:Device Types / Network > > > > > > > The other buffers will not begin with a struct > virtio_net_hdr. > > > > > > > \item If > > > > > > > - \field{num_buffers} is one, then the entire packet will be > > > > > > > - contained within this buffer, immediately following the > struct > > > > > > > - virtio_net_hdr. > > > > > > > + VIRTIO_NET_F_MRG_RXBUF was not negotiated or > \field{num_buffers} is one, then > > > > > > > + the entire packet will be contained within this buffer, > immediately following > > > > > > > + the struct virtio_net_hdr. > > > > > > > \item If the VIRTIO_NET_F_GUEST_CSUM feature was > negotiated, the > > > > > > > VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be > > > > > > > set: if so, device has validated the packet checksum. > > > > > > > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming > Packets}\label{sec:Device Types / Network > > > > > > > \devicenormative{\paragraph}{Processing of Incoming > Packets}{Device Types / Network Device / Device Operation / Processing of > Incoming Packets} > > > > > > > \label{devicenormative:Device Types / Network Device / > Device Operation / Processing of Packets}%old label for latexdiff > > > > > > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device > MUST set > > > > > > > -\field{num_buffers} to 1. > > > > > > > - > > > > > > > If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device > MUST set > > > > > > > \field{num_buffers} to indicate the number of buffers > > > > > > > the packet (including the header) is spread over. > > > > > > > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming > Packets}\label{sec:Device Types / Network > > > > > > > This is due to various bugs in implementations. > > > > > > > \end{note} > > > > > > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver > MUST ignore > > > > > > > +\field{num_buffers}. > > > > > > > + > > > > > > > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor > > > > > > > VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT > > > > > > > rely on the packet checksum being correct. > > > > > > > > > > > > > > > > > > There could be drivers relying on it. > > > > > > We can add a SHOULD not and ask drivers not to look at it, > > > > > > > > The patch adds the following statement: > > > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST > > > > > ignore \field{num_buffers}. > > > > > > > > Do you suggest changing MUST to SHOULD or rephrase other words in > this > > > > statement? > > > > > > Yes, I would change MUST to SHOULD. Basically documenting the existing > > > practice. > > > > > > But I do not yes see why we should drop the device requirements the > way you suggest. > > > We just need to fix devices, it does not seem hard. > > > > Well, I think we have already discussed and decided not to change > vhost_net: > > > https://lore.kernel.org/all/CACGkMEt0spn59oLyoCwcJDdLeYUEibePF7gppxdVX1YvmAr72Q@mail.gmail.com/ > > Looking at this spec made me realize fixing that is less risky. > It depends on how to do the relaxing, if we want to align the spec to the implementation, can we simply remove this: -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set -\field{num_buffers} to 1. ? Thanks > I just replied on that thread and proposed a way to reduce risk > even more. > Sorry about leading you astray. > > > > > > > > > > and we should fix devices. > > > > > > > > > > Maybe allowing 0 too is a good idea? Need to think through > > > > > implemenation concerns. > > > > > In any case, pls fix devices. > > > > > > > > I opted allowing any values for devices based on the discussion in > the > > > > following thread: > > > > > https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9 > > > > > > > > Regards, > > > > Akihiko Odaki > > > > > > That one asks for concrete data. > > > > It asks concrete data that motivates requiring setting the value to zero > and > > I decided removing such a requirement instead. > > > > Regards, > > Akihiko Odaki > > Same thing really. > > [-- Attachment #2: Type: text/html, Size: 10946 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-12-27 1:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-26 5:37 [PATCH] virtio-net: Ignore num_buffers when unused Akihiko Odaki 2024-12-26 7:29 ` Michael S. Tsirkin 2024-12-26 7:32 ` Michael S. Tsirkin 2024-12-26 7:35 ` Akihiko Odaki 2024-12-26 9:13 ` Michael S. Tsirkin 2024-12-26 11:09 ` Akihiko Odaki 2024-12-26 11:55 ` Michael S. Tsirkin 2024-12-27 1:26 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.