[PATCH] virtio-net: Ignore num_buffers when unused

[PATCH] virtio-net: Ignore num_buffers when unused

[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>

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Michael S. Tsirkin]

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>

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Michael S. Tsirkin]

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>

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Akihiko Odaki]

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

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Akihiko Odaki]

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

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Michael S. Tsirkin]

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.

Re: [PATCH] virtio-net: Ignore num_buffers when unused

[Jason Wang]

[-- 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 --]