[virtio-dev] Re: [PATCH] virtio_net: support split header

2022-08-01 Thread Heng Qi
The content of the mail just sent is about "[PATCH v6] virtio_net: 
support split header".


在 2022/8/1 下午2:59, Heng Qi 写道:

From: Xuan Zhuo 

The purpose of this feature is to split the header and the payload of
the packet.

|receive buffer|
|   0th descriptor | 1th descriptor|
| virtnet hdr | mac | ip hdr | tcp hdr|<-- hold -->|   payload |

We can use a buffer plus a separate page when allocating the receive
buffer. In this way, we can ensure that all payloads can be
independently in a page, which is very beneficial for the zerocopy
implemented by the upper layer.

Signed-off-by: Xuan Zhuo 
Signed-off-by: Heng Qi 
Reviewed-by: Kangjie Xu 
---

v6:
 1. Fix some syntax issues. @Cornelia Huck
 2. Clarify some paragraphs. @Cornelia Huck
 3. Determine the device what to do if it does not perform header split on 
a packet.

v5:
 1. Determine when hdr_len is credible in the process of rx
 2. Clean up the use of buffers and descriptors
 3. Clarify the meaning of used lenght if the first descriptor is skipped 
in the case of merge

v4:
 1. fix typo @Cornelia Huck @Jason Wang
 2. do not split header for IP fragmentation packet. @Jason Wang

v3:
 1. Fix some syntax issues
 2. Fix some terminology issues
 3. It is not unified with ip alignment, so ip alignment is not included
 4. Make it clear that the device must support four types, in the case of
 successful negotiation.

  conformance.tex |   2 +
  content.tex | 111 ++--
  2 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/conformance.tex b/conformance.tex
index 2b86fc6..bd0f463 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -150,6 +150,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Offloads State Configuration / Setting Offloads State}
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) }
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
  \end{itemize}
  
  \conformance{\subsection}{Block Driver Conformance}\label{sec:Conformance / Driver Conformance / Block Driver Conformance}

@@ -415,6 +416,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Automatic receive steering in multiqueue mode}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
  \end{itemize}
  
  \conformance{\subsection}{Block Device Conformance}\label{sec:Conformance / Device Conformance / Block Device Conformance}

diff --git a/content.tex b/content.tex
index e863709..74c36fe 100644
--- a/content.tex
+++ b/content.tex
@@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
Network Device / Feature bits
  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
  channel.
  
+\item[VIRTIO_NET_F_SPLIT_HEADER (52)] Device supports splitting the protocol

+header and the payload.
+
  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
  
  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.

@@ -3140,6 +3143,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device 
Types / Network Device
  \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
  \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or 
VIRTIO_NET_F_HOST_TSO6.
  \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_SPLIT_HEADER] Requires VIRTIO_NET_F_CTRL_VQ.
  \end{description}
  
  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}

@@ -3371,6 +3375,7 @@ \subsection{Device Operation}\label{sec:Device Types / 
Network Device / Device O
  #define VIRTIO_NET_HDR_F_NEEDS_CSUM1
  #define VIRTIO_NET_HDR_F_DATA_VALID2
  #define VIRTIO_NET_HDR_F_RSC_INFO  4
+#define VIRTIO_NET_HDR_F_SPLIT_HEADER  8
  u8 flags;
  #define VIRTIO_NET_HDR_GSO_NONE0
  #define VIRTIO_NET_HDR_GSO_TCPV4   1
@@ -3799,9 +3804,10 @@ \subsubsection{Processing of Incoming 
Packets}\label{sec:Device Types / Network
  not set VIRTIO_NET_HDR_F_RSC_INFO bit in 

[virtio-dev] [PATCH] virtio_net: support split header

2022-08-01 Thread Heng Qi
From: Xuan Zhuo 

The purpose of this feature is to split the header and the payload of
the packet.

|receive buffer|
|   0th descriptor | 1th descriptor|
| virtnet hdr | mac | ip hdr | tcp hdr|<-- hold -->|   payload |

We can use a buffer plus a separate page when allocating the receive
buffer. In this way, we can ensure that all payloads can be
independently in a page, which is very beneficial for the zerocopy
implemented by the upper layer.

Signed-off-by: Xuan Zhuo 
Signed-off-by: Heng Qi 
Reviewed-by: Kangjie Xu 
---

v6:
1. Fix some syntax issues. @Cornelia Huck
2. Clarify some paragraphs. @Cornelia Huck
3. Determine the device what to do if it does not perform header split on a 
packet.

v5:
1. Determine when hdr_len is credible in the process of rx
2. Clean up the use of buffers and descriptors
3. Clarify the meaning of used lenght if the first descriptor is skipped in 
the case of merge

v4:
1. fix typo @Cornelia Huck @Jason Wang
2. do not split header for IP fragmentation packet. @Jason Wang

v3:
1. Fix some syntax issues
2. Fix some terminology issues
3. It is not unified with ip alignment, so ip alignment is not included
4. Make it clear that the device must support four types, in the case of
successful negotiation.

 conformance.tex |   2 +
 content.tex | 111 ++--
 2 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/conformance.tex b/conformance.tex
index 2b86fc6..bd0f463 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -150,6 +150,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Offloads State Configuration / Setting Offloads State}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) }
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
 \end{itemize}
 
 \conformance{\subsection}{Block Driver Conformance}\label{sec:Conformance / 
Driver Conformance / Block Driver Conformance}
@@ -415,6 +416,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Automatic receive steering in multiqueue mode}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
 \end{itemize}
 
 \conformance{\subsection}{Block Device Conformance}\label{sec:Conformance / 
Device Conformance / Block Device Conformance}
diff --git a/content.tex b/content.tex
index e863709..74c36fe 100644
--- a/content.tex
+++ b/content.tex
@@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
Network Device / Feature bits
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
 channel.
 
+\item[VIRTIO_NET_F_SPLIT_HEADER (52)] Device supports splitting the protocol
+header and the payload.
+
 \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
 
 \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
@@ -3140,6 +3143,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device 
Types / Network Device
 \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or 
VIRTIO_NET_F_HOST_TSO6.
 \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_SPLIT_HEADER] Requires VIRTIO_NET_F_CTRL_VQ.
 \end{description}
 
 \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -3371,6 +3375,7 @@ \subsection{Device Operation}\label{sec:Device Types / 
Network Device / Device O
 #define VIRTIO_NET_HDR_F_NEEDS_CSUM1
 #define VIRTIO_NET_HDR_F_DATA_VALID2
 #define VIRTIO_NET_HDR_F_RSC_INFO  4
+#define VIRTIO_NET_HDR_F_SPLIT_HEADER  8
 u8 flags;
 #define VIRTIO_NET_HDR_GSO_NONE0
 #define VIRTIO_NET_HDR_GSO_TCPV4   1
@@ -3799,9 +3804,10 @@ \subsubsection{Processing of Incoming 
Packets}\label{sec:Device Types / Network
 not set VIRTIO_NET_HDR_F_RSC_INFO bit in \field{flags}.
 
 If one of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
-been negotiated, the device SHOULD set \field{hdr_len} to a value
+been 

[virtio-dev] Re: [PATCH v6 4/5] Add admin_queue_index register to PCI common configuration structure

2022-08-01 Thread Michael S. Tsirkin
On Mon, Aug 01, 2022 at 03:11:58AM +0300, Max Gurtovoy wrote:
> 
> On 8/1/2022 12:03 AM, Michael S. Tsirkin wrote:
> > On Sun, Jul 31, 2022 at 06:43:53PM +0300, Max Gurtovoy wrote:
> > > This new register will be used for querying the index of the admin
> > > virtqueue of a virtio device. To configure, reset or enable the admin
> > > virtqueue, the driver should follow existing queue configuration/setup
> > > sequence.
> > > 
> > > Reviewed-by: Parav Pandit 
> > > Signed-off-by: Max Gurtovoy 
> > 
> > Can you please at least add text to MMIO and CCW that drivers and
> > devices must not negotiate the new feature bit? Will help avoid confusion.
> 
> why this is needed ? who will create a device and expose bits that it
> doesn't know how to implement.

Any multi-transport implementation actually.
For example, on a Linux guest the default is to add a feature bit to all
transports. Extra care is needed to actually only add it
to the PCI transport. With qemu, same applies to features in
include/hw/virtio/virtio.h.

> And if I'll add it, we might forget to remove it later on.

When we add the implementation I'm reasonably sure we won't forget to
remove text saying it's not implemented.

> > 
> > > ---
> > >   content.tex | 10 ++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index c15423e..5fda1a0 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -904,6 +904,9 @@ \subsubsection{Common configuration structure 
> > > layout}\label{sec:Virtio Transport
> > >   le64 queue_device;  /* read-write */
> > >   le16 queue_notify_data; /* read-only for driver */
> > >   le16 queue_reset;   /* read-write */
> > > +
> > > +/* About admin virtqueue. */
> > > +le16 admin_queue_index; /* read-only for driver */
> > >   };
> > >   \end{lstlisting}
> > > @@ -989,6 +992,10 @@ \subsubsection{Common configuration structure 
> > > layout}\label{sec:Virtio Transport
> > >   This field exists only if VIRTIO_F_RING_RESET has been
> > >   negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / 
> > > Virtqueues / Virtqueue Reset}).
> > > +\item[\field{admin_queue_index}]
> > > +The device uses this to report the index of the admin virtqueue.
> > > +This field always exists. Its value is valid only if 
> > > VIRTIO_F_ADMIN_VQ has been negotiated.
> > > +
> > >   \end{description}
> > >   \devicenormative{\paragraph}{Common configuration structure 
> > > layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device 
> > > Layout / Common configuration structure layout}
> > we have a mess with this exists versus valid. I think exists is the same
> 
> the bigest mess that we have is for things like ring_reset that are
> optionally exist according to bit negotiation.

I think this optionally exist does not mean much, just that
it should not be accessed

> about valid - the driver shouldn't even look on registers that it didn't
> negotiated it's feature bits. There is no reason to do so.

> 
> So yes, there is a mess but not in our proposal.

Right. I propose removing "This field always exists." just
say that it's valid with VIRTIO_F_ADMIN_VQ.

> > is valid personally. Do others object if we say same as for reset here?
> > Not a big deal either way, we need to clean this up later.
> > 
> > > @@ -1075,6 +1082,9 @@ \subsubsection{Common configuration structure 
> > > layout}\label{sec:Virtio Transport
> > >   were used before the queue reset.
> > >   (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / 
> > > Virtqueue Reset}).
> > > +For configuring the admin virtqueue, the driver MUST use the value of 
> > > \field{admin_queue_index}.
> > > +For more details on virtqueue configuration see section \ref{sec:Virtio 
> > > Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And 
> > > Device Operation / Device Initialization / Virtqueue Configuration}.
> > > +
> > >   \subsubsection{Notification structure layout}\label{sec:Virtio 
> > > Transport Options / Virtio Over PCI Bus / PCI Device Layout / 
> > > Notification capability}
> > >   The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
> > > -- 
> > > 2.21.0


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [PATCH v6 3/5] Introduce virtio admin virtqueue

2022-08-01 Thread Michael S. Tsirkin
On Mon, Aug 01, 2022 at 02:07:26AM +0300, Max Gurtovoy wrote:
> 
> On 8/1/2022 12:00 AM, Michael S. Tsirkin wrote:
> > On Sun, Jul 31, 2022 at 06:43:52PM +0300, Max Gurtovoy wrote:
> > > In one of the many use cases a user wants to manipulate features and
> > > configuration of the virtio devices regardless of the device type
> > > (net/block/console). For that the admin command set introduced. The
> > > admin virtqueue will be the first management interface to issue admin
> > > commands.
> > > 
> > > Currently virtio specification defines control virtqueue to manipulate
> > > features and configuration of the device it operates on. However,
> > > control virtqueue commands are device type specific, which makes it very
> > > difficult to extend for device agnostic commands.
> > > 
> > > To support this requirement in elegant way, this patch introduces a new
> > > admin virtqueue interface.
> > > 
> > > Manipulate features via admin virtqueue is asynchronous, scalable, easy
> > > to extend and doesn't require additional and expensive on-die resources
> > > to be allocated for every new feature that will be added in the future.
> > > 
> > > Reviewed-by: Parav Pandit 
> > > Signed-off-by: Max Gurtovoy 
> > > ---
> > >   admin.tex   | 12 
> > >   content.tex |  6 --
> > >   2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index d14f8f7..26ac60e 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -80,3 +80,15 @@ \section{Administration command set}\label{sec:Basic 
> > > Facilities of a Virtio Devi
> > >   8000h - h   & Reserved\\
> > >   \hline
> > >   \end{tabular}
> > > +
> > > +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device 
> > > / Admin Virtqueues}
> > > +
> > > +An admin virtqueue is a management interface of a device that can be 
> > > used to send administrative
> > > +commands (see \ref{sec:Basic Facilities of a Virtio Device / 
> > > Administration command set}) to manipulate
> > > +various features of the device and/or to manipulate various features, if 
> > > possible, of another device.
> > > +
> > > +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ 
> > > feature is
> > > +negotiated. The index of the admin virtqueue is exposed by the device in 
> > > a
> > > +transport specific manner.
> > > +
> > > +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin 
> > > virtqueue to send all admin commands.
> > > diff --git a/content.tex b/content.tex
> > > index caab298..c15423e 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of 
> > > a Virtio Device / Feature B
> > >   \begin{description}
> > >   \item[0 to 23, and 50 to 127] Feature bits for the specific device type
> > > -\item[24 to 40] Feature bits reserved for extensions to the queue and
> > > +\item[24 to 41] Feature bits reserved for extensions to the queue and
> > > feature negotiation mechanisms
> > > -\item[41 to 49, and 128 and above] Feature bits reserved for future 
> > > extensions.
> > > +\item[42 to 49, and 128 and above] Feature bits reserved for future 
> > > extensions.
> > >   \end{description}
> > >   \begin{note}
> > > @@ -6841,6 +6841,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
> > > Feature Bits}
> > > that the driver can reset a queue individually.
> > > See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / 
> > > Virtqueue Reset}.
> > > +  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an 
> > > administration virtqueue is supported.
> > mention here that index is in transport specific manner too?
> 
> is this ok ?
> 
> +  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an
> administration virtqueue is supported.
> +  The index of the administration virtqueue exposed in a transport specific
> manner.

s/exposed/is specified/

> 
> > 
> > > +
> > >   \end{description}
> > >   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > -- 
> > > 2.21.0


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org