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

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

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>