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

["Michael S. Tsirkin" <mst@redhat.com>]

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>