[virtio-comment] [PATCH v2 0/2] virtio-net: Clarify virtio_net_hdr size and rx buffer size

[virtio-comment] [PATCH v2 0/2] virtio-net: Clarify virtio_net_hdr size and rx buffer size

[Parav Pandit]

There are few small issues around size of receive buffer and size of the
struct virtio_net_hdr as following.

1. VIRTIO_NET_F_HASH_REPORT is only applicable on the rx side processing.
   However it changes the size of the struct virtio_net_hdr for the tx side
   as well. This was not clarified when VIRTIO_NET_F_HASH_REPORT feature was
   introduced.

2. Receive buffer size calculation description and requirements did not
   reflect negotiation of VIRTIO_NET_F_HASH_REPORT.

3. Receive buffer size calculation was incorrect for ipv6 guest gso feature.

Above issue 2 was reported in [1].
Since issue 2 and 3 both touches the same set of requirements and description,
they are updated under a new issue along with tx side fix as well touching the
clarification needed for VIRTIO_NET_F_HASH_REPORT.

Hence above three issues are fixed under issue of [2].

Patch summary:
patch-1 fixes receive side description and requirement
patch-2 adds transmit side clarification for VIRTIO_NET_F_HASH_REPORT

changelog:
v1->v2:
- addressed comments from Cornelia
- rephrase buffer size wording without explicit length of virtio_net_hdr
  at 3 places.
- replaced otherwise to ', otherwise'
- rephrase non normative to write as driver normative
v0->v1:
- addressed comments from Xuan to rewrite tx normative

Parav Pandit (2):
  virtio-net: Fix receive buffer size calculation text
  virtio-net: Clarify the size of the struct virtio_net_hdr for tx

 device-types/net/description.tex | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
2.34.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

Receive buffer size calculation is based on the following
negotiated features.

The text has wrong calculation for IPv6 and also it has missed
VIRTIO_NET_F_HASH_REPORT.

The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
in [1], however fix for ipv6 payload length must also be
considered.

Since for the both the fixes touching same requirements, a
new issue is created as [2].

This patch brings following fixes.

1. Fix annotating struct virtio_net_hdr as field
2. Fix receive buffer calculation for guest GSO cases to consider
   ipv6 payload length
3. small grammar corrections for article
4. reword the requirement to consider the virtio_ndr_hdr which is
   depends on the negotiated feature, hence first clarify the
   struct virtio_net_hdr size

[1] https://github.com/oasis-tcs/virtio-spec/issues/170
[2] https://github.com/oasis-tcs/virtio-spec/issues/183

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v1->v2:
- addressed comments from Cornelia
- rephrase buffer size wording without explicit length of virtio_net_hdr
  at 3 places.
- replaced otherwise to ', otherwise'
- rephrase non normative to write as driver normative
---
 device-types/net/description.tex | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index aff5e08..53a56ee 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -657,10 +657,13 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
 If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6
 features are used, the maximum incoming packet
-will be to 65550 bytes long (the maximum size of a
-TCP or UDP packet, plus the 14 byte ethernet header), otherwise
-1514 bytes.  The 12-byte struct virtio_net_hdr is prepended to this,
-making for 65562 or 1526 bytes.
+will be 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes of
+the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
+extension header), otherwise 1514 bytes.
+When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive buffer
+size is either 65601 or 1526 bytes.
+When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
+size is either 65609 or 1534 bytes.
 
 \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
 
@@ -669,18 +672,24 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
   \begin{itemize}
     \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO,
 	VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated, the driver SHOULD populate
-      the receive queue(s) with buffers of at least 65562 bytes.
+      the receive queue(s) with buffers of at least 65609 bytes if
+      VIRTIO_NET_F_HASH_REPORT is negotiated, and of at least 65601 bytes if not.
     \item Otherwise, the driver SHOULD populate the receive queue(s)
-      with buffers of at least 1526 bytes.
+      with buffers of at least 1534 bytes if VIRTIO_NET_F_HASH_REPORT
+      is negotiated, and of at least 1526 bytes if not.
   \end{itemize}
 \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
-least the size of the struct virtio_net_hdr.
+least size of \field{struct virtio_net_hdr},
+i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
 \end{itemize}
 
 \begin{note}
 Obviously each buffer can be split across multiple descriptor elements.
 \end{note}
 
+The driver MUST consider size of field \field{struct virtio_net_hdr}
+20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
+
 If VIRTIO_NET_F_MQ is negotiated, each of receiveq1\ldots receiveqN
 that will be used SHOULD be populated with receive buffers.
 
-- 
2.34.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Cornelia Huck]

On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:

> Receive buffer size calculation is based on the following
> negotiated features.
>
> The text has wrong calculation for IPv6 and also it has missed
> VIRTIO_NET_F_HASH_REPORT.
>
> The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
> in [1], however fix for ipv6 payload length must also be
> considered.
>
> Since for the both the fixes touching same requirements, a
> new issue is created as [2].
>
> This patch brings following fixes.
>
> 1. Fix annotating struct virtio_net_hdr as field
> 2. Fix receive buffer calculation for guest GSO cases to consider
>    ipv6 payload length
> 3. small grammar corrections for article
> 4. reword the requirement to consider the virtio_ndr_hdr which is
>    depends on the negotiated feature, hence first clarify the
>    struct virtio_net_hdr size
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> [2] https://github.com/oasis-tcs/virtio-spec/issues/183
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v1->v2:
> - addressed comments from Cornelia
> - rephrase buffer size wording without explicit length of virtio_net_hdr
>   at 3 places.
> - replaced otherwise to ', otherwise'
> - rephrase non normative to write as driver normative
> ---
>  device-types/net/description.tex | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index aff5e08..53a56ee 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -657,10 +657,13 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
>  If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
>  VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6
>  features are used, the maximum incoming packet
> -will be to 65550 bytes long (the maximum size of a
> -TCP or UDP packet, plus the 14 byte ethernet header), otherwise
> -1514 bytes.  The 12-byte struct virtio_net_hdr is prepended to this,
> -making for 65562 or 1526 bytes.
> +will be 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes of
> +the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
> +extension header), otherwise 1514 bytes.

Hm, I wonder if we should still mention that

"\field{struct virtio_net_header} will be prepended to this, which is
either 20 or 12 bytes long, depending on whether
VIRTIO_NET_F_HASH_REPORT is negotiated or not."

> +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive buffer
> +size is either 65601 or 1526 bytes.
> +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
> +size is either 65609 or 1534 bytes.
>  
>  \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
>  
> @@ -669,18 +672,24 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
>    \begin{itemize}
>      \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO,
>  	VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated, the driver SHOULD populate
> -      the receive queue(s) with buffers of at least 65562 bytes.
> +      the receive queue(s) with buffers of at least 65609 bytes if
> +      VIRTIO_NET_F_HASH_REPORT is negotiated, and of at least 65601 bytes if not.
>      \item Otherwise, the driver SHOULD populate the receive queue(s)
> -      with buffers of at least 1526 bytes.
> +      with buffers of at least 1534 bytes if VIRTIO_NET_F_HASH_REPORT
> +      is negotiated, and of at least 1526 bytes if not.
>    \end{itemize}
>  \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> -least the size of the struct virtio_net_hdr.
> +least size of \field{struct virtio_net_hdr},
> +i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
>  \end{itemize}
>  
>  \begin{note}
>  Obviously each buffer can be split across multiple descriptor elements.
>  \end{note}
>  
> +The driver MUST consider size of field \field{struct virtio_net_hdr}
> +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
> +

Requiring the driver to consider the size of something to be its actual
size seems a bit odd :) I don't think we need this, as the length can be
derived from looking at the definitions, and is already spelled out
explicitly, if you consider my suggestion above.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, January 15, 2024 10:14 PM

> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> 
> > Receive buffer size calculation is based on the following negotiated
> > features.
> >
> > The text has wrong calculation for IPv6 and also it has missed
> > VIRTIO_NET_F_HASH_REPORT.
> >
> > The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported in
> > [1], however fix for ipv6 payload length must also be considered.
> >
> > Since for the both the fixes touching same requirements, a new issue
> > is created as [2].
> >
> > This patch brings following fixes.
> >
> > 1. Fix annotating struct virtio_net_hdr as field 2. Fix receive buffer
> > calculation for guest GSO cases to consider
> >    ipv6 payload length
> > 3. small grammar corrections for article 4. reword the requirement to
> > consider the virtio_ndr_hdr which is
> >    depends on the negotiated feature, hence first clarify the
> >    struct virtio_net_hdr size
> >
> > [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> > [2] https://github.com/oasis-tcs/virtio-spec/issues/183
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> >
> > ---
> > changelog:
> > v1->v2:
> > - addressed comments from Cornelia
> > - rephrase buffer size wording without explicit length of virtio_net_hdr
> >   at 3 places.
> > - replaced otherwise to ', otherwise'
> > - rephrase non normative to write as driver normative
> > ---
> >  device-types/net/description.tex | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index aff5e08..53a56ee 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -657,10 +657,13 @@ \subsubsection{Setting Up Receive
> > Buffers}\label{sec:Device Types / Network Devi  If the
> > VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> > VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or
> > VIRTIO_NET_F_GUEST_USO6  features are used, the maximum incoming
> > packet -will be to 65550 bytes long (the maximum size of a -TCP or UDP
> > packet, plus the 14 byte ethernet header), otherwise
> > -1514 bytes.  The 12-byte struct virtio_net_hdr is prepended to this,
> > -making for 65562 or 1526 bytes.
> > +will be 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes
> > +of the IPv6 header, plus 65535 bytes of maximum IPv6 payload
> > +including any extension header), otherwise 1514 bytes.
> 
> Hm, I wonder if we should still mention that
> 
> "\field{struct virtio_net_header} will be prepended to this, which is either 20
> or 12 bytes long, depending on whether VIRTIO_NET_F_HASH_REPORT is
> negotiated or not."
>
Ah I lost that in this change. I will add it. Good point.
 
> > +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required
> receive
> > +buffer size is either 65601 or 1526 bytes.
> > +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive
> > +buffer size is either 65609 or 1534 bytes.
> >
> >  \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types
> > / Network Device / Device Operation / Setting Up Receive Buffers}
> >
> > @@ -669,18 +672,24 @@ \subsubsection{Setting Up Receive
> Buffers}\label{sec:Device Types / Network Devi
> >    \begin{itemize}
> >      \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> VIRTIO_NET_F_GUEST_UFO,
> >  	VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are
> negotiated, the driver SHOULD populate
> > -      the receive queue(s) with buffers of at least 65562 bytes.
> > +      the receive queue(s) with buffers of at least 65609 bytes if
> > +      VIRTIO_NET_F_HASH_REPORT is negotiated, and of at least 65601
> bytes if not.
> >      \item Otherwise, the driver SHOULD populate the receive queue(s)
> > -      with buffers of at least 1526 bytes.
> > +      with buffers of at least 1534 bytes if VIRTIO_NET_F_HASH_REPORT
> > +      is negotiated, and of at least 1526 bytes if not.
> >    \end{itemize}
> >  \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> > -least the size of the struct virtio_net_hdr.
> > +least size of \field{struct virtio_net_hdr}, i.e. 20 bytes if
> > +VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
> >  \end{itemize}
> >
> >  \begin{note}
> >  Obviously each buffer can be split across multiple descriptor elements.
> >  \end{note}
> >
> > +The driver MUST consider size of field \field{struct virtio_net_hdr}
> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if
> not.
> > +
> 
> Requiring the driver to consider the size of something to be its actual size
> seems a bit odd :) I don't think we need this, as the length can be derived
> from looking at the definitions, and is already spelled out explicitly, if you
> consider my suggestion above.
We need this because tx side also needs to refer to the virtio_net_hdr in patch 2 to be same as that of the rx side.
And hence, this normative sets base line for tx side too. Relying on rest of the receive packet normative is not enough.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Cornelia Huck]

On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:

>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: Monday, January 15, 2024 10:14 PM
>
>> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
>> > +The driver MUST consider size of field \field{struct virtio_net_hdr}
>> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if
>> not.
>> > +
>> 
>> Requiring the driver to consider the size of something to be its actual size
>> seems a bit odd :) I don't think we need this, as the length can be derived
>> from looking at the definitions, and is already spelled out explicitly, if you
>> consider my suggestion above.
> We need this because tx side also needs to refer to the virtio_net_hdr in patch 2 to be same as that of the rx side.
> And hence, this normative sets base line for tx side too. Relying on rest of the receive packet normative is not enough.

Hm, why? If struct virtio_net_hdr is well-defined, its size is
well-defined as well, and we do not need to state it explictly?


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, January 16, 2024 4:33 PM
> To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org;
> mst@redhat.com
> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> yuri.benditovich@daynix.com
> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text
> 
> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Monday, January 15, 2024 10:14 PM
> >
> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> > +The driver MUST consider size of field \field{struct
> >> > +virtio_net_hdr}
> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes
> >> > +if
> >> not.
> >> > +
> >>
> >> Requiring the driver to consider the size of something to be its
> >> actual size seems a bit odd :) I don't think we need this, as the
> >> length can be derived from looking at the definitions, and is already
> >> spelled out explicitly, if you consider my suggestion above.
> > We need this because tx side also needs to refer to the virtio_net_hdr in
> patch 2 to be same as that of the rx side.
> > And hence, this normative sets base line for tx side too. Relying on rest of the
> receive packet normative is not enough.
> 
> Hm, why? If struct virtio_net_hdr is well-defined, its size is well-defined as
> well, and we do not need to state it explictly?
Because, 
the size of virtio_net_hdr is derived from the rx side features.
Today there is no normative line that says that even though you are using A, B, C Rx features, due to which your tx side virtio_net_hdr also changes.
The 2nd patch in this series adds this explicit normative as explained in the cover letter.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Cornelia Huck]

On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:

>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: Tuesday, January 16, 2024 4:33 PM
>> To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org;
>> mst@redhat.com
>> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
>> yuri.benditovich@daynix.com
>> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text
>> 
>> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
>> 
>> >> From: Cornelia Huck <cohuck@redhat.com>
>> >> Sent: Monday, January 15, 2024 10:14 PM
>> >
>> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
>> >> > +The driver MUST consider size of field \field{struct
>> >> > +virtio_net_hdr}
>> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes
>> >> > +if
>> >> not.
>> >> > +
>> >>
>> >> Requiring the driver to consider the size of something to be its
>> >> actual size seems a bit odd :) I don't think we need this, as the
>> >> length can be derived from looking at the definitions, and is already
>> >> spelled out explicitly, if you consider my suggestion above.
>> > We need this because tx side also needs to refer to the virtio_net_hdr in
>> patch 2 to be same as that of the rx side.
>> > And hence, this normative sets base line for tx side too. Relying on rest of the
>> receive packet normative is not enough.
>> 
>> Hm, why? If struct virtio_net_hdr is well-defined, its size is well-defined as
>> well, and we do not need to state it explictly?
> Because, 
> the size of virtio_net_hdr is derived from the rx side features.
> Today there is no normative line that says that even though you are using A, B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> The 2nd patch in this series adds this explicit normative as explained in the cover letter.

Let's step back a bit.

struct virtio_net_hdr is defined at the beginning of the "Device
Operation" section; the definition clearly says that the last three
fields depend on VIRTIO_NET_F_HASH_REPORT being negotiated. The device
and the driver agree on whether HASH_REPORT is negotiated, and therefore
should also agree on the size of virtio_net_hdr?

Or is the problem that we did not state explicitly that the last three
fields of virtio_net_hdr do not exist without HASH_REPORT (and are not
merely invalid)? If yes, we should spell this out, instead of adding
normative statements about what the size of virtio_net_hdr should be
considered to be. If virtio_net_hdr has a fixed size, we shouldn't need
the second patch, either.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, January 16, 2024 6:08 PM
> To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org;
> mst@redhat.com
> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> yuri.benditovich@daynix.com
> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text
> 
> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Tuesday, January 16, 2024 4:33 PM
> >> To: Parav Pandit <parav@nvidia.com>;
> >> virtio-comment@lists.oasis-open.org;
> >> mst@redhat.com
> >> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> >> yuri.benditovich@daynix.com
> >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> >> calculation text
> >>
> >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> >>
> >> >> From: Cornelia Huck <cohuck@redhat.com>
> >> >> Sent: Monday, January 15, 2024 10:14 PM
> >> >
> >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> >> > +The driver MUST consider size of field \field{struct
> >> >> > +virtio_net_hdr}
> >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
> >> >> > +bytes if
> >> >> not.
> >> >> > +
> >> >>
> >> >> Requiring the driver to consider the size of something to be its
> >> >> actual size seems a bit odd :) I don't think we need this, as the
> >> >> length can be derived from looking at the definitions, and is
> >> >> already spelled out explicitly, if you consider my suggestion above.
> >> > We need this because tx side also needs to refer to the
> >> > virtio_net_hdr in
> >> patch 2 to be same as that of the rx side.
> >> > And hence, this normative sets base line for tx side too. Relying
> >> > on rest of the
> >> receive packet normative is not enough.
> >>
> >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
> >> well-defined as well, and we do not need to state it explictly?
> > Because,
> > the size of virtio_net_hdr is derived from the rx side features.
> > Today there is no normative line that says that even though you are using A,
> B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > The 2nd patch in this series adds this explicit normative as explained in the
> cover letter.
> 
> Let's step back a bit.
> 
> struct virtio_net_hdr is defined at the beginning of the "Device Operation"
> section; the definition clearly says that the last three fields depend on
> VIRTIO_NET_F_HASH_REPORT being negotiated. The device and the driver
> agree on whether HASH_REPORT is negotiated, and therefore should also
> agree on the size of virtio_net_hdr?
> 
Do you imply that device operation description is enough to not add normative?
If so, for this case and possibly new things if we write as device operation, would it be enough?

> Or is the problem that we did not state explicitly that the last three fields of
> virtio_net_hdr do not exist without HASH_REPORT (and are not merely
> invalid)? If yes, we should spell this out, instead of adding normative
> statements about what the size of virtio_net_hdr should be considered to be.
This suggested normative is added in this patch.

> If virtio_net_hdr has a fixed size, we shouldn't need the second patch, either.
The fact that HASH_REPORT is only for the rx, if we have to go back in time, there is no need for the tx to force also to follow the rx virtio_net_hdr.
There is no explicit normative indicating the virtio_net_hdr for the TX is forced by the RX even though it has no relation to hash report.

If you say device operation is enough, than I am sort of lost of when normative is needed, and when device operation is enough.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Michael S. Tsirkin]

On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Tuesday, January 16, 2024 6:08 PM
> > To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org;
> > mst@redhat.com
> > Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> > yuri.benditovich@daynix.com
> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text
> > 
> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > 
> > >> From: Cornelia Huck <cohuck@redhat.com>
> > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > >> To: Parav Pandit <parav@nvidia.com>;
> > >> virtio-comment@lists.oasis-open.org;
> > >> mst@redhat.com
> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> > >> yuri.benditovich@daynix.com
> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > >> calculation text
> > >>
> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >>
> > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > >> >> Sent: Monday, January 15, 2024 10:14 PM
> > >> >
> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >> >> > +The driver MUST consider size of field \field{struct
> > >> >> > +virtio_net_hdr}
> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
> > >> >> > +bytes if
> > >> >> not.
> > >> >> > +
> > >> >>
> > >> >> Requiring the driver to consider the size of something to be its
> > >> >> actual size seems a bit odd :) I don't think we need this, as the
> > >> >> length can be derived from looking at the definitions, and is
> > >> >> already spelled out explicitly, if you consider my suggestion above.
> > >> > We need this because tx side also needs to refer to the
> > >> > virtio_net_hdr in
> > >> patch 2 to be same as that of the rx side.
> > >> > And hence, this normative sets base line for tx side too. Relying
> > >> > on rest of the
> > >> receive packet normative is not enough.
> > >>
> > >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
> > >> well-defined as well, and we do not need to state it explictly?
> > > Because,
> > > the size of virtio_net_hdr is derived from the rx side features.
> > > Today there is no normative line that says that even though you are using A,
> > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > > The 2nd patch in this series adds this explicit normative as explained in the
> > cover letter.
> > 
> > Let's step back a bit.
> > 
> > struct virtio_net_hdr is defined at the beginning of the "Device Operation"
> > section; the definition clearly says that the last three fields depend on
> > VIRTIO_NET_F_HASH_REPORT being negotiated. The device and the driver
> > agree on whether HASH_REPORT is negotiated, and therefore should also
> > agree on the size of virtio_net_hdr?
> > 
> Do you imply that device operation description is enough to not add normative?
> If so, for this case and possibly new things if we write as device operation, would it be enough?
> 
> > Or is the problem that we did not state explicitly that the last three fields of
> > virtio_net_hdr do not exist without HASH_REPORT (and are not merely
> > invalid)? If yes, we should spell this out, instead of adding normative
> > statements about what the size of virtio_net_hdr should be considered to be.
> This suggested normative is added in this patch.
> 
> > If virtio_net_hdr has a fixed size, we shouldn't need the second patch, either.
> The fact that HASH_REPORT is only for the rx, if we have to go back in time, there is no need for the tx to force also to follow the rx virtio_net_hdr.
> There is no explicit normative indicating the virtio_net_hdr for the TX is forced by the RX even though it has no relation to hash report.
> 
> If you say device operation is enough, than I am sort of lost of when normative is needed, and when device operation is enough.


Generally we start adding normatives when we see that something is
unclear. But I think generally I agree with Parav, if someone has
the time to write the normative, it's all good.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

Hi Cornelia,

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, January 16, 2024 6:59 PM
> 
> On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> >
> > > From: Cornelia Huck <cohuck@redhat.com>
> > > Sent: Tuesday, January 16, 2024 6:08 PM
> > > To: Parav Pandit <parav@nvidia.com>;
> > > virtio-comment@lists.oasis-open.org;
> > > mst@redhat.com
> > > Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> > > yuri.benditovich@daynix.com
> > > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > calculation text
> > >
> > > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > >> From: Cornelia Huck <cohuck@redhat.com>
> > > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > > >> To: Parav Pandit <parav@nvidia.com>;
> > > >> virtio-comment@lists.oasis-open.org;
> > > >> mst@redhat.com
> > > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > >> calculation text
> > > >>
> > > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > >>
> > > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > > >> >> Sent: Monday, January 15, 2024 10:14 PM
> > > >> >
> > > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > >> >> > +The driver MUST consider size of field \field{struct
> > > >> >> > +virtio_net_hdr}
> > > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
> > > >> >> > +bytes if
> > > >> >> not.
> > > >> >> > +
> > > >> >>
> > > >> >> Requiring the driver to consider the size of something to be
> > > >> >> its actual size seems a bit odd :) I don't think we need this,
> > > >> >> as the length can be derived from looking at the definitions,
> > > >> >> and is already spelled out explicitly, if you consider my suggestion
> above.
> > > >> > We need this because tx side also needs to refer to the
> > > >> > virtio_net_hdr in
> > > >> patch 2 to be same as that of the rx side.
> > > >> > And hence, this normative sets base line for tx side too.
> > > >> > Relying on rest of the
> > > >> receive packet normative is not enough.
> > > >>
> > > >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
> > > >> well-defined as well, and we do not need to state it explictly?
> > > > Because,
> > > > the size of virtio_net_hdr is derived from the rx side features.
> > > > Today there is no normative line that says that even though you
> > > > are using A,
> > > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > > > The 2nd patch in this series adds this explicit normative as
> > > > explained in the
> > > cover letter.
> > >
> > > Let's step back a bit.
> > >
> > > struct virtio_net_hdr is defined at the beginning of the "Device Operation"
> > > section; the definition clearly says that the last three fields
> > > depend on VIRTIO_NET_F_HASH_REPORT being negotiated. The device
> and
> > > the driver agree on whether HASH_REPORT is negotiated, and therefore
> > > should also agree on the size of virtio_net_hdr?
> > >
> > Do you imply that device operation description is enough to not add
> normative?
> > If so, for this case and possibly new things if we write as device operation,
> would it be enough?
> >
> > > Or is the problem that we did not state explicitly that the last
> > > three fields of virtio_net_hdr do not exist without HASH_REPORT (and
> > > are not merely invalid)? If yes, we should spell this out, instead
> > > of adding normative statements about what the size of virtio_net_hdr
> should be considered to be.
> > This suggested normative is added in this patch.
> >
> > > If virtio_net_hdr has a fixed size, we shouldn't need the second patch,
> either.
> > The fact that HASH_REPORT is only for the rx, if we have to go back in time,
> there is no need for the tx to force also to follow the rx virtio_net_hdr.
> > There is no explicit normative indicating the virtio_net_hdr for the TX is
> forced by the RX even though it has no relation to hash report.
> >
> > If you say device operation is enough, than I am sort of lost of when
> normative is needed, and when device operation is enough.
> 
> 
> Generally we start adding normatives when we see that something is unclear.
> But I think generally I agree with Parav, if someone has the time to write the
> normative, it's all good.

Are you ok with the v3?
I kept the clarification for tx and addressed rest of your and Xuan's comments in it.
https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

Hi Cornelia,

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Parav Pandit
> Sent: Friday, January 19, 2024 5:56 PM
> 
> Hi Cornelia,
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, January 16, 2024 6:59 PM
> >
> > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> > >
> > > > From: Cornelia Huck <cohuck@redhat.com>
> > > > Sent: Tuesday, January 16, 2024 6:08 PM
> > > > To: Parav Pandit <parav@nvidia.com>;
> > > > virtio-comment@lists.oasis-open.org;
> > > > mst@redhat.com
> > > > Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > > calculation text
> > > >
> > > > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > >> From: Cornelia Huck <cohuck@redhat.com>
> > > > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > > > >> To: Parav Pandit <parav@nvidia.com>;
> > > > >> virtio-comment@lists.oasis-open.org;
> > > > >> mst@redhat.com
> > > > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > > >> calculation text
> > > > >>
> > > > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > >>
> > > > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > > > >> >> Sent: Monday, January 15, 2024 10:14 PM
> > > > >> >
> > > > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > >> >> > +The driver MUST consider size of field \field{struct
> > > > >> >> > +virtio_net_hdr}
> > > > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and
> > > > >> >> > +12 bytes if
> > > > >> >> not.
> > > > >> >> > +
> > > > >> >>
> > > > >> >> Requiring the driver to consider the size of something to be
> > > > >> >> its actual size seems a bit odd :) I don't think we need
> > > > >> >> this, as the length can be derived from looking at the
> > > > >> >> definitions, and is already spelled out explicitly, if you
> > > > >> >> consider my suggestion
> > above.
> > > > >> > We need this because tx side also needs to refer to the
> > > > >> > virtio_net_hdr in
> > > > >> patch 2 to be same as that of the rx side.
> > > > >> > And hence, this normative sets base line for tx side too.
> > > > >> > Relying on rest of the
> > > > >> receive packet normative is not enough.
> > > > >>
> > > > >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
> > > > >> well-defined as well, and we do not need to state it explictly?
> > > > > Because,
> > > > > the size of virtio_net_hdr is derived from the rx side features.
> > > > > Today there is no normative line that says that even though you
> > > > > are using A,
> > > > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > > > > The 2nd patch in this series adds this explicit normative as
> > > > > explained in the
> > > > cover letter.
> > > >
> > > > Let's step back a bit.
> > > >
> > > > struct virtio_net_hdr is defined at the beginning of the "Device
> Operation"
> > > > section; the definition clearly says that the last three fields
> > > > depend on VIRTIO_NET_F_HASH_REPORT being negotiated. The device
> > and
> > > > the driver agree on whether HASH_REPORT is negotiated, and
> > > > therefore should also agree on the size of virtio_net_hdr?
> > > >
> > > Do you imply that device operation description is enough to not add
> > normative?
> > > If so, for this case and possibly new things if we write as device
> > > operation,
> > would it be enough?
> > >
> > > > Or is the problem that we did not state explicitly that the last
> > > > three fields of virtio_net_hdr do not exist without HASH_REPORT
> > > > (and are not merely invalid)? If yes, we should spell this out,
> > > > instead of adding normative statements about what the size of
> > > > virtio_net_hdr
> > should be considered to be.
> > > This suggested normative is added in this patch.
> > >
> > > > If virtio_net_hdr has a fixed size, we shouldn't need the second
> > > > patch,
> > either.
> > > The fact that HASH_REPORT is only for the rx, if we have to go back
> > > in time,
> > there is no need for the tx to force also to follow the rx virtio_net_hdr.
> > > There is no explicit normative indicating the virtio_net_hdr for the
> > > TX is
> > forced by the RX even though it has no relation to hash report.
> > >
> > > If you say device operation is enough, than I am sort of lost of
> > > when
> > normative is needed, and when device operation is enough.
> >
> >
> > Generally we start adding normatives when we see that something is
> unclear.
> > But I think generally I agree with Parav, if someone has the time to
> > write the normative, it's all good.
> 
> Are you ok with the v3?
> I kept the clarification for tx and addressed rest of your and Xuan's comments
> in it.
> https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html
> 

Its been nearly two weeks now.
I assume v3 is ok.
Can you please create the voting ballot?
I updated the github issue to refer to the v3 as well.
https://github.com/oasis-tcs/virtio-spec/issues/183
https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html


> 
> This publicly archived list offers a means to provide input to theOASIS Virtual
> I/O Device (VIRTIO) TC.In order to verify user consent to the Feedback
> License terms andto minimize spam in the list archive, subscription is
> requiredbefore posting.Subscribe: virtio-comment-subscribe@lists.oasis-
> open.orgUnsubscribe: virtio-comment-unsubscribe@lists.oasis-open.orgList
> help: virtio-comment-help@lists.oasis-open.orgList archive:
> https://lists.oasis-open.org/archives/virtio-comment/Feedback License:
> https://www.oasis-open.org/who/ipr/feedback_license.pdfList Guidelines:
> https://www.oasis-open.org/policies-guidelines/mailing-listsCommittee:
> https://www.oasis-open.org/committees/virtio/Join OASIS:
> https://www.oasis-open.org/join/


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

Hi Cornelia,

> From: Parav Pandit <parav@nvidia.com>
> Sent: Thursday, February 1, 2024 11:27 AM
> 
> Hi Cornelia,
> 
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Parav Pandit
> > Sent: Friday, January 19, 2024 5:56 PM
> >
> > Hi Cornelia,
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, January 16, 2024 6:59 PM
> > >
> > > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Cornelia Huck <cohuck@redhat.com>
> > > > > Sent: Tuesday, January 16, 2024 6:08 PM
> > > > > To: Parav Pandit <parav@nvidia.com>;
> > > > > virtio-comment@lists.oasis-open.org;
> > > > > mst@redhat.com
> > > > > Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > > xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > > > calculation text
> > > > >
> > > > > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > >> From: Cornelia Huck <cohuck@redhat.com>
> > > > > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > > > > >> To: Parav Pandit <parav@nvidia.com>;
> > > > > >> virtio-comment@lists.oasis-open.org;
> > > > > >> mst@redhat.com
> > > > > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer
> > > > > >> size calculation text
> > > > > >>
> > > > > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > > >>
> > > > > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > > > > >> >> Sent: Monday, January 15, 2024 10:14 PM
> > > > > >> >
> > > > > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > > >> >> > +The driver MUST consider size of field \field{struct
> > > > > >> >> > +virtio_net_hdr}
> > > > > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and
> > > > > >> >> > +12 bytes if
> > > > > >> >> not.
> > > > > >> >> > +
> > > > > >> >>
> > > > > >> >> Requiring the driver to consider the size of something to
> > > > > >> >> be its actual size seems a bit odd :) I don't think we
> > > > > >> >> need this, as the length can be derived from looking at
> > > > > >> >> the definitions, and is already spelled out explicitly, if
> > > > > >> >> you consider my suggestion
> > > above.
> > > > > >> > We need this because tx side also needs to refer to the
> > > > > >> > virtio_net_hdr in
> > > > > >> patch 2 to be same as that of the rx side.
> > > > > >> > And hence, this normative sets base line for tx side too.
> > > > > >> > Relying on rest of the
> > > > > >> receive packet normative is not enough.
> > > > > >>
> > > > > >> Hm, why? If struct virtio_net_hdr is well-defined, its size
> > > > > >> is well-defined as well, and we do not need to state it explictly?
> > > > > > Because,
> > > > > > the size of virtio_net_hdr is derived from the rx side features.
> > > > > > Today there is no normative line that says that even though
> > > > > > you are using A,
> > > > > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > > > > > The 2nd patch in this series adds this explicit normative as
> > > > > > explained in the
> > > > > cover letter.
> > > > >
> > > > > Let's step back a bit.
> > > > >
> > > > > struct virtio_net_hdr is defined at the beginning of the "Device
> > Operation"
> > > > > section; the definition clearly says that the last three fields
> > > > > depend on VIRTIO_NET_F_HASH_REPORT being negotiated. The device
> > > and
> > > > > the driver agree on whether HASH_REPORT is negotiated, and
> > > > > therefore should also agree on the size of virtio_net_hdr?
> > > > >
> > > > Do you imply that device operation description is enough to not
> > > > add
> > > normative?
> > > > If so, for this case and possibly new things if we write as device
> > > > operation,
> > > would it be enough?
> > > >
> > > > > Or is the problem that we did not state explicitly that the last
> > > > > three fields of virtio_net_hdr do not exist without HASH_REPORT
> > > > > (and are not merely invalid)? If yes, we should spell this out,
> > > > > instead of adding normative statements about what the size of
> > > > > virtio_net_hdr
> > > should be considered to be.
> > > > This suggested normative is added in this patch.
> > > >
> > > > > If virtio_net_hdr has a fixed size, we shouldn't need the second
> > > > > patch,
> > > either.
> > > > The fact that HASH_REPORT is only for the rx, if we have to go
> > > > back in time,
> > > there is no need for the tx to force also to follow the rx virtio_net_hdr.
> > > > There is no explicit normative indicating the virtio_net_hdr for
> > > > the TX is
> > > forced by the RX even though it has no relation to hash report.
> > > >
> > > > If you say device operation is enough, than I am sort of lost of
> > > > when
> > > normative is needed, and when device operation is enough.
> > >
> > >
> > > Generally we start adding normatives when we see that something is
> > unclear.
> > > But I think generally I agree with Parav, if someone has the time to
> > > write the normative, it's all good.
> >
> > Are you ok with the v3?
> > I kept the clarification for tx and addressed rest of your and Xuan's
> > comments in it.
> > https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.h
> > tml
> >
> 
> Its been nearly two weeks now.
> I assume v3 is ok.
> Can you please create the voting ballot?
> I updated the github issue to refer to the v3 as well.
> https://github.com/oasis-tcs/virtio-spec/issues/183
> https://lists.oasis-open.org/archives/virtio-
> comment/202401/msg00070.html

This thread [2] is waiting your response from 19th January 2023.
Can you please either respond or open v3 [1] for vote?

[1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html
[2] https://lore.kernel.org/virtio-comment/PH0PR12MB5481F74BF29A751E6BC249D4DC702@PH0PR12MB5481.namprd12.prod.outlook.com/


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Cornelia Huck]

On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
>> 
>> > From: Cornelia Huck <cohuck@redhat.com>
>> > Sent: Tuesday, January 16, 2024 6:08 PM
>> > To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org;
>> > mst@redhat.com
>> > Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
>> > yuri.benditovich@daynix.com
>> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text
>> > 
>> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
>> > 
>> > >> From: Cornelia Huck <cohuck@redhat.com>
>> > >> Sent: Tuesday, January 16, 2024 4:33 PM
>> > >> To: Parav Pandit <parav@nvidia.com>;
>> > >> virtio-comment@lists.oasis-open.org;
>> > >> mst@redhat.com
>> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
>> > >> yuri.benditovich@daynix.com
>> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
>> > >> calculation text
>> > >>
>> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
>> > >>
>> > >> >> From: Cornelia Huck <cohuck@redhat.com>
>> > >> >> Sent: Monday, January 15, 2024 10:14 PM
>> > >> >
>> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
>> > >> >> > +The driver MUST consider size of field \field{struct
>> > >> >> > +virtio_net_hdr}
>> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
>> > >> >> > +bytes if
>> > >> >> not.
>> > >> >> > +
>> > >> >>
>> > >> >> Requiring the driver to consider the size of something to be its
>> > >> >> actual size seems a bit odd :) I don't think we need this, as the

My comment here still holds.

>> > >> >> length can be derived from looking at the definitions, and is
>> > >> >> already spelled out explicitly, if you consider my suggestion above.
>> > >> > We need this because tx side also needs to refer to the
>> > >> > virtio_net_hdr in
>> > >> patch 2 to be same as that of the rx side.
>> > >> > And hence, this normative sets base line for tx side too. Relying
>> > >> > on rest of the
>> > >> receive packet normative is not enough.
>> > >>
>> > >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
>> > >> well-defined as well, and we do not need to state it explictly?
>> > > Because,
>> > > the size of virtio_net_hdr is derived from the rx side features.
>> > > Today there is no normative line that says that even though you are using A,
>> > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
>> > > The 2nd patch in this series adds this explicit normative as explained in the
>> > cover letter.
>> > 
>> > Let's step back a bit.
>> > 
>> > struct virtio_net_hdr is defined at the beginning of the "Device Operation"
>> > section; the definition clearly says that the last three fields depend on
>> > VIRTIO_NET_F_HASH_REPORT being negotiated. The device and the driver
>> > agree on whether HASH_REPORT is negotiated, and therefore should also
>> > agree on the size of virtio_net_hdr?
>> > 
>> Do you imply that device operation description is enough to not add normative?
>> If so, for this case and possibly new things if we write as device operation, would it be enough?
>> 
>> > Or is the problem that we did not state explicitly that the last three fields of
>> > virtio_net_hdr do not exist without HASH_REPORT (and are not merely
>> > invalid)? If yes, we should spell this out, instead of adding normative
>> > statements about what the size of virtio_net_hdr should be considered to be.
>> This suggested normative is added in this patch.
>> 
>> > If virtio_net_hdr has a fixed size, we shouldn't need the second patch, either.
>> The fact that HASH_REPORT is only for the rx, if we have to go back in time, there is no need for the tx to force also to follow the rx virtio_net_hdr.
>> There is no explicit normative indicating the virtio_net_hdr for the TX is forced by the RX even though it has no relation to hash report.
>> 
>> If you say device operation is enough, than I am sort of lost of when normative is needed, and when device operation is enough.
>
>
> Generally we start adding normatives when we see that something is
> unclear. But I think generally I agree with Parav, if someone has
> the time to write the normative, it's all good.

So if we really want to have normatives, can we come up with something
that reads less weirdly? I currently lack the capacity to suggest
something.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, February 5, 2024 8:50 PM
> 
> On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> >>
> >> > From: Cornelia Huck <cohuck@redhat.com>
> >> > Sent: Tuesday, January 16, 2024 6:08 PM
> >> > To: Parav Pandit <parav@nvidia.com>;
> >> > virtio-comment@lists.oasis-open.org;
> >> > mst@redhat.com
> >> > Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> >> > yuri.benditovich@daynix.com
> >> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> >> > calculation text
> >> >
> >> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> >
> >> > >> From: Cornelia Huck <cohuck@redhat.com>
> >> > >> Sent: Tuesday, January 16, 2024 4:33 PM
> >> > >> To: Parav Pandit <parav@nvidia.com>;
> >> > >> virtio-comment@lists.oasis-open.org;
> >> > >> mst@redhat.com
> >> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> >> > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> >> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> >> > >> calculation text
> >> > >>
> >> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> > >>
> >> > >> >> From: Cornelia Huck <cohuck@redhat.com>
> >> > >> >> Sent: Monday, January 15, 2024 10:14 PM
> >> > >> >
> >> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> > >> >> > +The driver MUST consider size of field \field{struct
> >> > >> >> > +virtio_net_hdr}
> >> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
> >> > >> >> > +bytes if
> >> > >> >> not.
> >> > >> >> > +
> >> > >> >>
> >> > >> >> Requiring the driver to consider the size of something to be
> >> > >> >> its actual size seems a bit odd :) I don't think we need
> >> > >> >> this, as the
> 
> My comment here still holds.
>
It is a variable length data structure with few trailing fields as not valid.
The "actual size" depends on negotiated feature bit, which is what is written here.
 
How about below?

When calculating the size of \field{struct virtio_net_hdr}, the driver must consider all the fields
inclusive up to \field{padding_reserved}, i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated,
and 12 bytes if not.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Cornelia Huck]

On Mon, Feb 05 2024, Parav Pandit <parav@nvidia.com> wrote:

>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: Monday, February 5, 2024 8:50 PM
>> 
>> On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
>> >>
>> >> > From: Cornelia Huck <cohuck@redhat.com>
>> >> > Sent: Tuesday, January 16, 2024 6:08 PM
>> >> > To: Parav Pandit <parav@nvidia.com>;
>> >> > virtio-comment@lists.oasis-open.org;
>> >> > mst@redhat.com
>> >> > Cc: Shahaf Shuler <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
>> >> > yuri.benditovich@daynix.com
>> >> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
>> >> > calculation text
>> >> >
>> >> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
>> >> >
>> >> > >> From: Cornelia Huck <cohuck@redhat.com>
>> >> > >> Sent: Tuesday, January 16, 2024 4:33 PM
>> >> > >> To: Parav Pandit <parav@nvidia.com>;
>> >> > >> virtio-comment@lists.oasis-open.org;
>> >> > >> mst@redhat.com
>> >> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
>> >> > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
>> >> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
>> >> > >> calculation text
>> >> > >>
>> >> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
>> >> > >>
>> >> > >> >> From: Cornelia Huck <cohuck@redhat.com>
>> >> > >> >> Sent: Monday, January 15, 2024 10:14 PM
>> >> > >> >
>> >> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
>> >> > >> >> > +The driver MUST consider size of field \field{struct
>> >> > >> >> > +virtio_net_hdr}
>> >> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12
>> >> > >> >> > +bytes if
>> >> > >> >> not.
>> >> > >> >> > +
>> >> > >> >>
>> >> > >> >> Requiring the driver to consider the size of something to be
>> >> > >> >> its actual size seems a bit odd :) I don't think we need
>> >> > >> >> this, as the
>> 
>> My comment here still holds.
>>
> It is a variable length data structure with few trailing fields as not valid.
> The "actual size" depends on negotiated feature bit, which is what is written here.
>  
> How about below?
>
> When calculating the size of \field{struct virtio_net_hdr}, the driver must consider all the fields
> inclusive up to \field{padding_reserved}, i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated,
> and 12 bytes if not.

It's up to padding_reserved with F_HASH_REPORT and up to num_buffers if
not, isn't it?


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, February 6, 2024 10:32 PM
> 
> On Mon, Feb 05 2024, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Monday, February 5, 2024 8:50 PM
> >>
> >> On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>
> >> > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> >> >>
> >> >> > From: Cornelia Huck <cohuck@redhat.com>
> >> >> > Sent: Tuesday, January 16, 2024 6:08 PM
> >> >> > To: Parav Pandit <parav@nvidia.com>;
> >> >> > virtio-comment@lists.oasis-open.org;
> >> >> > mst@redhat.com
> >> >> > Cc: Shahaf Shuler <shahafs@nvidia.com>;
> >> >> > xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> >> >> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> >> >> > calculation text
> >> >> >
> >> >> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> >> >
> >> >> > >> From: Cornelia Huck <cohuck@redhat.com>
> >> >> > >> Sent: Tuesday, January 16, 2024 4:33 PM
> >> >> > >> To: Parav Pandit <parav@nvidia.com>;
> >> >> > >> virtio-comment@lists.oasis-open.org;
> >> >> > >> mst@redhat.com
> >> >> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> >> >> > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> >> >> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer
> >> >> > >> size calculation text
> >> >> > >>
> >> >> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> >> > >>
> >> >> > >> >> From: Cornelia Huck <cohuck@redhat.com>
> >> >> > >> >> Sent: Monday, January 15, 2024 10:14 PM
> >> >> > >> >
> >> >> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> >> >> > >> >> > +The driver MUST consider size of field \field{struct
> >> >> > >> >> > +virtio_net_hdr}
> >> >> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and
> >> >> > >> >> > +12 bytes if
> >> >> > >> >> not.
> >> >> > >> >> > +
> >> >> > >> >>
> >> >> > >> >> Requiring the driver to consider the size of something to
> >> >> > >> >> be its actual size seems a bit odd :) I don't think we
> >> >> > >> >> need this, as the
> >>
> >> My comment here still holds.
> >>
> > It is a variable length data structure with few trailing fields as not valid.
> > The "actual size" depends on negotiated feature bit, which is what is written
> here.
> >
> > How about below?
> >
> > When calculating the size of \field{struct virtio_net_hdr}, the driver
> > must consider all the fields inclusive up to \field{padding_reserved},
> > i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if
> not.
> 
> It's up to padding_reserved with F_HASH_REPORT and up to num_buffers if
> not, isn't it?

Yes, shall I send v4 as?
When calculating the size of \field{struct virtio_net_hdr}, the driver must consider all the fields inclusive up to \field{padding_reserved},
i.e. 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and upto \field{num_buffers}, i.e. 12 bytes if not.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit]

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Parav Pandit
> Sent: Monday, February 5, 2024 9:54 PM
> To: Cornelia Huck <cohuck@redhat.com>; Michael S. Tsirkin
> <mst@redhat.com>
> Cc: virtio-comment@lists.oasis-open.org; Shahaf Shuler
> <shahafs@nvidia.com>; xuanzhuo@linux.alibaba.com;
> yuri.benditovich@daynix.com
> Subject: RE: [virtio-comment] Re: [PATCH v2 1/2] virtio-net: Fix receive buffer
> size calculation text
> 
> 
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, February 5, 2024 8:50 PM
> >
> > On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> > >>
> > >> > From: Cornelia Huck <cohuck@redhat.com>
> > >> > Sent: Tuesday, January 16, 2024 6:08 PM
> > >> > To: Parav Pandit <parav@nvidia.com>;
> > >> > virtio-comment@lists.oasis-open.org;
> > >> > mst@redhat.com
> > >> > Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > >> > xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > >> > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > >> > calculation text
> > >> >
> > >> > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >> >
> > >> > >> From: Cornelia Huck <cohuck@redhat.com>
> > >> > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > >> > >> To: Parav Pandit <parav@nvidia.com>;
> > >> > >> virtio-comment@lists.oasis-open.org;
> > >> > >> mst@redhat.com
> > >> > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > >> > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > >> > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer
> > >> > >> size calculation text
> > >> > >>
> > >> > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >> > >>
> > >> > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > >> > >> >> Sent: Monday, January 15, 2024 10:14 PM
> > >> > >> >
> > >> > >> >> On Mon, Jan 15 2024, Parav Pandit <parav@nvidia.com> wrote:
> > >> > >> >> > +The driver MUST consider size of field \field{struct
> > >> > >> >> > +virtio_net_hdr}
> > >> > >> >> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and
> > >> > >> >> > +12 bytes if
> > >> > >> >> not.
> > >> > >> >> > +
> > >> > >> >>
> > >> > >> >> Requiring the driver to consider the size of something to
> > >> > >> >> be its actual size seems a bit odd :) I don't think we need
> > >> > >> >> this, as the
> >
> > My comment here still holds.
> >
> It is a variable length data structure with few trailing fields as not valid.
> The "actual size" depends on negotiated feature bit, which is what is written
> here.
> 
> How about below?
> 
> When calculating the size of \field{struct virtio_net_hdr}, the driver must
> consider all the fields inclusive up to \field{padding_reserved}, i.e. 20 bytes if
> VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.

I will shortly post v4 with this as there is no objection for this one line change.
Thanks.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v2 2/2] virtio-net: Clarify the size of the struct virtio_net_hdr for tx

[Parav Pandit]

The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it was
not clarified that the size of the struct virtio_net_hdr on the packet
transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
negotiated.

Explicitly clarify this.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- addressed comments from Xuan to rewrite tx normative
---
 device-types/net/description.tex | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 53a56ee..e2de016 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -528,6 +528,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 
 \drivernormative{\paragraph}{Packet Transmission}{Device Types / Network Device / Device Operation / Packet Transmission}
 
+The driver MUST use the same size of the field \field{struct virtio_net_hdr}
+for transmit and receive packet buffers.
+
 The driver MUST set \field{num_buffers} to zero.
 
 If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
-- 
2.34.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [PATCH v2 2/2] virtio-net: Clarify the size of the struct virtio_net_hdr for tx

[Xuan Zhuo]

On Mon, 15 Jan 2024 11:31:41 +0200, Parav Pandit <parav@nvidia.com> wrote:
> The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it was
> not clarified that the size of the struct virtio_net_hdr on the packet
> transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> negotiated.
>
> Explicitly clarify this.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v0->v1:
> - addressed comments from Xuan to rewrite tx normative
> ---
>  device-types/net/description.tex | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 53a56ee..e2de016 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -528,6 +528,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>
>  \drivernormative{\paragraph}{Packet Transmission}{Device Types / Network Device / Device Operation / Packet Transmission}
>
> +The driver MUST use the same size of the field \field{struct virtio_net_hdr}
> +for transmit and receive packet buffers.

s/field/structure/ ?

Thanks.


> +
>  The driver MUST set \field{num_buffers} to zero.
>
>  If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> --
> 2.34.1
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] RE: [PATCH v2 2/2] virtio-net: Clarify the size of the struct virtio_net_hdr for tx

[Parav Pandit]

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, January 16, 2024 9:05 AM
> 
> On Mon, 15 Jan 2024 11:31:41 +0200, Parav Pandit <parav@nvidia.com>
> wrote:
> > The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> > However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it
> was
> > not clarified that the size of the struct virtio_net_hdr on the packet
> > transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> > negotiated.
> >
> > Explicitly clarify this.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> >
> > ---
> > changelog:
> > v0->v1:
> > - addressed comments from Xuan to rewrite tx normative
> > ---
> >  device-types/net/description.tex | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index 53a56ee..e2de016 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -528,6 +528,9 @@ \subsubsection{Packet
> > Transmission}\label{sec:Device Types / Network Device / De
> >
> >  \drivernormative{\paragraph}{Packet Transmission}{Device Types /
> > Network Device / Device Operation / Packet Transmission}
> >
> > +The driver MUST use the same size of the field \field{struct
> > +virtio_net_hdr} for transmit and receive packet buffers.
> 
> s/field/structure/ ?
>
Ok. will change in v3.
 
> Thanks.
> 
> 
> > +
> >  The driver MUST set \field{num_buffers} to zero.
> >
> >  If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> > --
> > 2.34.1
> >

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/