* [virtio-comment] [PATCH v4 0/2] virtio-net: Improve dev config layout
@ 2023-02-23 2:35 Parav Pandit
2023-02-23 2:35 ` [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only Parav Pandit
2023-02-23 2:35 ` [PATCH v4 2/2] virtio-net: Define cfg fields before description Parav Pandit
0 siblings, 2 replies; 6+ messages in thread
From: Parav Pandit @ 2023-02-23 2:35 UTC (permalink / raw)
To: mst, virtio-dev, cohuck; +Cc: virtio-comment, shahafs, Parav Pandit
This two patches improve dev configuration layout in two ways.
1. Define device configuration layout before describing it fields
2. Avoid duplicate description of its read only fields and whole
layout
Patch summary:
patch-1: consolidate read only field at one place in driver
requirements
patch-2: define device configuration layout before describing its
fields.
changelog:
v3->v4:
- wrote driver requirement as normative statement
- moved read only line to description section
v2->v3:
- split into two patches
- move read only description to driver requirements section
Parav Pandit (2):
virtio-net: Describe dev cfg fields read only
virtio-net: Define cfg fields before description
device-types/net/description.tex | 49 ++++++++++++++++++--------------
1 file changed, 28 insertions(+), 21 deletions(-)
--
2.26.2
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only
2023-02-23 2:35 [virtio-comment] [PATCH v4 0/2] virtio-net: Improve dev config layout Parav Pandit
@ 2023-02-23 2:35 ` Parav Pandit
2023-02-23 10:10 ` David Edmondson
2023-02-23 2:35 ` [PATCH v4 2/2] virtio-net: Define cfg fields before description Parav Pandit
1 sibling, 1 reply; 6+ messages in thread
From: Parav Pandit @ 2023-02-23 2:35 UTC (permalink / raw)
To: mst, virtio-dev, cohuck
Cc: virtio-comment, shahafs, Parav Pandit, David Edmondson
Device configuration fields are read only. Avoid duplicating this
description for multiple fields.
Instead describe it one time and do it in the driver requirements
section.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v3->v4:
- write driver requirement as normative statement
- add back read only wording in the description
v2->v3:
- split as new patch
---
device-types/net/description.tex | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 73501b6..821f7b0 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -156,25 +156,28 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
\subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
-Device configuration fields are listed below, they are read-only for a driver. The \field{mac} address field
-always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
-\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
-read-only bits (for the driver) are currently defined for the status field:
-VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
+Device configuration fields are listed below. All the device
+configuration fields are read-only for the driver.
+
+The \field{mac} address field always exists (though is only
+valid if VIRTIO_NET_F_MAC is set), and \field{status} only
+exists if VIRTIO_NET_F_STATUS is set. Two bits are currently
+defined for the status field: VIRTIO_NET_S_LINK_UP and
+VIRTIO_NET_S_ANNOUNCE.
\begin{lstlisting}
#define VIRTIO_NET_S_LINK_UP 1
#define VIRTIO_NET_S_ANNOUNCE 2
\end{lstlisting}
-The following driver-read-only field, \field{max_virtqueue_pairs} only exists if
+The following field, \field{max_virtqueue_pairs} only exists if
VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies the maximum number
of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
and transmitq1\ldots transmitqN respectively) that can be configured once at least one of these features
is negotiated.
-The following driver-read-only field, \field{mtu} only exists if
-VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
+The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU
+is set. This field specifies the maximum MTU for the driver to
use.
The following two fields, \field{speed} and \field{duplex}, only
@@ -264,6 +267,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
+The driver MUST NOT write to any of the device configuration fields.
+
A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] virtio-net: Define cfg fields before description
2023-02-23 2:35 [virtio-comment] [PATCH v4 0/2] virtio-net: Improve dev config layout Parav Pandit
2023-02-23 2:35 ` [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only Parav Pandit
@ 2023-02-23 2:35 ` Parav Pandit
2023-02-23 10:12 ` [virtio-comment] " David Edmondson
1 sibling, 1 reply; 6+ messages in thread
From: Parav Pandit @ 2023-02-23 2:35 UTC (permalink / raw)
To: mst, virtio-dev, cohuck
Cc: virtio-comment, shahafs, Parav Pandit, David Edmondson
Currently some fields of the virtio_net_config structure are defined
before introducing the structure and some are defined after
introducing virtio_net_config.
Better to define the configuration layout first followed by
description of all the fields.
Device configuration fields are described in the section. Change wording
from 'listed' to 'described' as suggested in patch [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00004.html
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v2->v3:
- split the patch for read only description as prepration patch
- rebased
v1->v2:
- remove read-only wording from multiple places
v0->v1:
- Change wording about device configuration field introduction
- remove duplicate read-only wording for status field
- reword sentence to read it better
---
device-types/net/description.tex | 42 +++++++++++++++++---------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 821f7b0..7033594 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -156,14 +156,29 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
\subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
-Device configuration fields are listed below. All the device
-configuration fields are read-only for the driver.
+The network device has the following device configuration layout. All the
+device configuration fields are read-only for the driver.
-The \field{mac} address field always exists (though is only
-valid if VIRTIO_NET_F_MAC is set), and \field{status} only
-exists if VIRTIO_NET_F_STATUS is set. Two bits are currently
-defined for the status field: VIRTIO_NET_S_LINK_UP and
-VIRTIO_NET_S_ANNOUNCE.
+\begin{lstlisting}
+struct virtio_net_config {
+ u8 mac[6];
+ le16 status;
+ le16 max_virtqueue_pairs;
+ le16 mtu;
+ le32 speed;
+ u8 duplex;
+ u8 rss_max_key_size;
+ le16 rss_max_indirection_table_length;
+ le32 supported_hash_types;
+};
+\end{lstlisting}
+
+The \field{mac} address field always exists (although it is only
+valid if VIRTIO_NET_F_MAC is set).
+
+The \field{status} only exists if VIRTIO_NET_F_STATUS is set.
+Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
+and VIRTIO_NET_S_ANNOUNCE.
\begin{lstlisting}
#define VIRTIO_NET_S_LINK_UP 1
@@ -193,19 +208,6 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
is expected to re-read these values after receiving a
configuration change notification.
-\begin{lstlisting}
-struct virtio_net_config {
- u8 mac[6];
- le16 status;
- le16 max_virtqueue_pairs;
- le16 mtu;
- le32 speed;
- u8 duplex;
- u8 rss_max_key_size;
- le16 rss_max_indirection_table_length;
- le32 supported_hash_types;
-};
-\end{lstlisting}
The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT is set.
It specifies the maximum supported length of RSS key in bytes.
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only
2023-02-23 2:35 ` [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only Parav Pandit
@ 2023-02-23 10:10 ` David Edmondson
0 siblings, 0 replies; 6+ messages in thread
From: David Edmondson @ 2023-02-23 10:10 UTC (permalink / raw)
To: Parav Pandit; +Cc: mst, virtio-dev, cohuck, virtio-comment, shahafs
On Thursday, 2023-02-23 at 04:35:20 +02, Parav Pandit wrote:
> Device configuration fields are read only. Avoid duplicating this
> description for multiple fields.
>
> Instead describe it one time and do it in the driver requirements
> section.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
Minor comment below.
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> ---
> changelog:
> v3->v4:
> - write driver requirement as normative statement
> - add back read only wording in the description
> v2->v3:
> - split as new patch
> ---
> device-types/net/description.tex | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 73501b6..821f7b0 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -156,25 +156,28 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
> \subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
>
> -Device configuration fields are listed below, they are read-only for a driver. The \field{mac} address field
> -always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> -\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
> -read-only bits (for the driver) are currently defined for the status field:
> -VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> +Device configuration fields are listed below. All the device
"All of the"
> +configuration fields are read-only for the driver.
> +
> +The \field{mac} address field always exists (though is only
> +valid if VIRTIO_NET_F_MAC is set), and \field{status} only
> +exists if VIRTIO_NET_F_STATUS is set. Two bits are currently
> +defined for the status field: VIRTIO_NET_S_LINK_UP and
> +VIRTIO_NET_S_ANNOUNCE.
>
> \begin{lstlisting}
> #define VIRTIO_NET_S_LINK_UP 1
> #define VIRTIO_NET_S_ANNOUNCE 2
> \end{lstlisting}
>
> -The following driver-read-only field, \field{max_virtqueue_pairs} only exists if
> +The following field, \field{max_virtqueue_pairs} only exists if
> VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies the maximum number
> of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> and transmitq1\ldots transmitqN respectively) that can be configured once at least one of these features
> is negotiated.
>
> -The following driver-read-only field, \field{mtu} only exists if
> -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> +The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU
> +is set. This field specifies the maximum MTU for the driver to
> use.
>
> The following two fields, \field{speed} and \field{duplex}, only
> @@ -264,6 +267,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>
> \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>
> +The driver MUST NOT write to any of the device configuration fields.
> +
> A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
> the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
--
What did you learn today? I learnt nothing.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [virtio-comment] Re: [PATCH v4 2/2] virtio-net: Define cfg fields before description
2023-02-23 2:35 ` [PATCH v4 2/2] virtio-net: Define cfg fields before description Parav Pandit
@ 2023-02-23 10:12 ` David Edmondson
2023-02-23 18:16 ` Parav Pandit
0 siblings, 1 reply; 6+ messages in thread
From: David Edmondson @ 2023-02-23 10:12 UTC (permalink / raw)
To: Parav Pandit; +Cc: mst, virtio-dev, cohuck, virtio-comment, shahafs
On Thursday, 2023-02-23 at 04:35:21 +02, Parav Pandit wrote:
> Currently some fields of the virtio_net_config structure are defined
> before introducing the structure and some are defined after
> introducing virtio_net_config.
Don't need the trailing "introducing virtio_net_config".
> Better to define the configuration layout first followed by
> description of all the fields.
>
> Device configuration fields are described in the section. Change wording
> from 'listed' to 'described' as suggested in patch [1].
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00004.html
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
> changelog:
> v2->v3:
> - split the patch for read only description as prepration patch
> - rebased
> v1->v2:
> - remove read-only wording from multiple places
> v0->v1:
> - Change wording about device configuration field introduction
> - remove duplicate read-only wording for status field
> - reword sentence to read it better
> ---
> device-types/net/description.tex | 42 +++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 821f7b0..7033594 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -156,14 +156,29 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
> \subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
>
> -Device configuration fields are listed below. All the device
> -configuration fields are read-only for the driver.
> +The network device has the following device configuration layout. All the
> +device configuration fields are read-only for the driver.
>
> -The \field{mac} address field always exists (though is only
> -valid if VIRTIO_NET_F_MAC is set), and \field{status} only
> -exists if VIRTIO_NET_F_STATUS is set. Two bits are currently
> -defined for the status field: VIRTIO_NET_S_LINK_UP and
> -VIRTIO_NET_S_ANNOUNCE.
> +\begin{lstlisting}
> +struct virtio_net_config {
> + u8 mac[6];
> + le16 status;
> + le16 max_virtqueue_pairs;
> + le16 mtu;
> + le32 speed;
> + u8 duplex;
> + u8 rss_max_key_size;
> + le16 rss_max_indirection_table_length;
> + le32 supported_hash_types;
> +};
> +\end{lstlisting}
> +
> +The \field{mac} address field always exists (although it is only
> +valid if VIRTIO_NET_F_MAC is set).
> +
> +The \field{status} only exists if VIRTIO_NET_F_STATUS is set.
> +Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
> +and VIRTIO_NET_S_ANNOUNCE.
>
> \begin{lstlisting}
> #define VIRTIO_NET_S_LINK_UP 1
> @@ -193,19 +208,6 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
> is expected to re-read these values after receiving a
> configuration change notification.
>
> -\begin{lstlisting}
> -struct virtio_net_config {
> - u8 mac[6];
> - le16 status;
> - le16 max_virtqueue_pairs;
> - le16 mtu;
> - le32 speed;
> - u8 duplex;
> - u8 rss_max_key_size;
> - le16 rss_max_indirection_table_length;
> - le32 supported_hash_types;
> -};
> -\end{lstlisting}
> The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT is set.
> It specifies the maximum supported length of RSS key in bytes.
--
I used to get mad at my school, the teachers who taught me weren't cool.
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v4 2/2] virtio-net: Define cfg fields before description
2023-02-23 10:12 ` [virtio-comment] " David Edmondson
@ 2023-02-23 18:16 ` Parav Pandit
0 siblings, 0 replies; 6+ messages in thread
From: Parav Pandit @ 2023-02-23 18:16 UTC (permalink / raw)
To: David Edmondson
Cc: mst@redhat.com, virtio-dev@lists.oasis-open.org,
cohuck@redhat.com, virtio-comment@lists.oasis-open.org,
Shahaf Shuler
> From: David Edmondson <david.edmondson@oracle.com>
> Sent: Thursday, February 23, 2023 5:12 AM
>
>
> On Thursday, 2023-02-23 at 04:35:21 +02, Parav Pandit wrote:
> > Currently some fields of the virtio_net_config structure are defined
> > before introducing the structure and some are defined after
> > introducing virtio_net_config.
>
> Don't need the trailing "introducing virtio_net_config".
Thanks. Addressed this and another comment in v5 at [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00556.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-23 18:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-23 2:35 [virtio-comment] [PATCH v4 0/2] virtio-net: Improve dev config layout Parav Pandit
2023-02-23 2:35 ` [PATCH v4 1/2] virtio-net: Describe dev cfg fields read only Parav Pandit
2023-02-23 10:10 ` David Edmondson
2023-02-23 2:35 ` [PATCH v4 2/2] virtio-net: Define cfg fields before description Parav Pandit
2023-02-23 10:12 ` [virtio-comment] " David Edmondson
2023-02-23 18:16 ` Parav Pandit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox