virtio-net: Improve dev config layout
v3 cover virtio-net: Improve dev config layout
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: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 | 48 ++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-)
Signature · 18 lines
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: [email protected] Unsubscribe: [email protected] List help: [email protected] 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/
PATCH v3 1/2 virtio-net: Describe dev cfg fields read only
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.
---
changelog:v2->v3:
- split as new patch
--- device-types/net/description.tex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
device-types/net/description.tex +7 -5
@@ -156,10 +156,10 @@ \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 +Device configuration fields are listed below. 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: +bits (for the driver) are currently defined for the status field: VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. \begin{lstlisting}
@@ -167,14 +167,14 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device #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
@@ -261,6 +261,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout} +All the device configuration fields are read-only for the driver. + 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
Signature · 1 line
2.26.2
Re: [virtio-dev] [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 8 lines · Parav Pandit
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.
Quoted 52 lines
---
changelog:v2->v3:
- split as new patch--- device-types/net/description.tex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)device-types/net/description.tex +7
-5@@ -156,10 +156,10 @@ \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 +Device configuration fields are listed below. 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: +bits (for the driver) are currently defined for the status field: VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. \begin{lstlisting}@@ -167,14 +167,14 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device #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@@ -261,6 +261,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout} +All the device configuration fields are read-only for the driver. + 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
Signature · 5 lines
I'm not the reason you're looking for redemption. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
In reply to Parav PanditQuoted 21 lines · Parav Pandit
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.
Signed-off-by Parav Pandit <[email protected]>---
changelog:v2->v3:
- split as new patch--- device-types/net/description.tex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)device-types/net/description.tex +0
-0
device-types/net/description.tex \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network · line 159
Quoted 5 lines
device-types/net/description.tex +1
-1@@ -156,10 +156,10 @@ \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 +Device configuration fields are listed below. The \field{mac} address field
I would not remove this here, as I don't think we should move a simple statement into the conformance section (see below.) It does makes sense to remove the duplicate read-only annotations from the individual fields.
device-types/net/description.tex \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network · line 162
Quoted 4 lines
device-types/net/description.tex +1
-1always 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: +bits (for the driver) are currently defined for the status field:
What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Quoted 22 lines
device-types/net/description.tex +3
-3VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. \begin{lstlisting}@@ -167,14 +167,14 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device #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
device-types/net/description.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 264
Quoted 4 lines
device-types/net/description.tex +1
-0@@ -261,6 +261,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout} +All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the driver something actionable:
"A driver SHOULD NOT try to write to any of the device configuration fields."
device-types/net/description.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 268
Quoted 4 lines
device-types/net/description.tex +1
-0+ 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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 34 lines · Cornelia Huck
Quoted 21 lines · Parav Pandit
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.
Signed-off-by Parav Pandit <[email protected]>---
changelog:v2->v3:
- split as new patch--- device-types/net/description.tex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)device-types/net/description.tex +0
-0device-types/net/description.tex \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network · line 159
Quoted 5 lines
device-types/net/description.tex +1
-1@@ -156,10 +156,10 @@ \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 +Device configuration fields are listed below. The \field{mac} address fieldI would not remove this here, as I don't think we should move a simple statement into the conformance section (see below.) It does makes sense to remove the duplicate read-only annotations from the individual fields.
Quoted 7 lines
device-types/net/description.tex \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network · line 162
Quoted 4 lines
device-types/net/description.tex +1
-1always 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: +bits (for the driver) are currently defined for the status field:What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without reading the result? Pls don't.
Quoted 25 lines
Quoted 25 lines
device-types/net/description.tex +3
-3VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. \begin{lstlisting}@@ -167,14 +167,14 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device #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@@ -261,6 +261,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
Quoted 7 lines
device-types/net/description.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 264
Quoted 1 line
device-types/net/description.tex +1
-0+All the device configuration fields are read-only for the driver.Not sure if this makes a good normative clause, I would rather give the driver something actionable:
"A driver SHOULD NOT try to write to any of the device configuration fields."
Agree it's not a normative statement as is.
MUST NOT actually - they were always read only.
And no need to "try" just don't write period.
Quoted 4 lines
Quoted 4 lines
device-types/net/description.tex +1
-0+ 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
RE: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 11 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:42 PMQuoted 3 lines
What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without reading the result? Pls don't.
It was wrong to keep the "for the driver".
I will fix this.
Quoted 37 lines
Quoted 5 lines
Review \subsection{Device configuration
Quoted 5 lines
VIRTIO_NET_S_LINK_UPandVIRTIO_NET_S_ANNOUNCE.\begin{lstlisting}
(quoted diff) +0
-0@@ -167,14 +167,14 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 10 lines
Quoted 10 lines
device-types/net/description.tex +0
-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_MQorVIRTIO_NET_F_RSSis 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 isnegotiated.
Quoted 20 lines
Quoted 12 lines
(quoted diff) +0
-2-The following driver-read-only field, \field{mtu} only exists if -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU forthe driver to
(quoted diff) +2
-1+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 @@ -261,6 +261,8 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 4 lines
\drivernormative{\subsubsection}{Device configuration
layout}{Device Types / Network Device / Device configuration layout}+All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the driver something actionable:
Quoted 7 lines
Quoted 2 lines
"A driver SHOULD NOT try to write to any of the device configuration fields."
Agree it's not a normative statement as is. MUST NOT actually - they were always read only. And no need to "try" just don't write period.
Saying driver must not write it, doesn't make it read only for the device.
Hence, it should be mentioned as read-only fields, so when the driver writes something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
+
A driver SHOULD negotiateVIRTIO_NET_F_MACif the device offers it. If the driver negotiates theVIRTIO_NET_F_MACfeature, the driver
MUST set the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 51 lines · Parav Pandit
Quoted 11 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:42 PMQuoted 3 lines
What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without reading the result? Pls don't.
It was wrong to keep the "for the driver".
I will fix this.Quoted 37 lines
Quoted 5 lines
Review \subsection{Device configuration
Quoted 5 lines
VIRTIO_NET_S_LINK_UPandVIRTIO_NET_S_ANNOUNCE.\begin{lstlisting}
(quoted diff) +0
-0@@ -167,14 +167,14 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 10 lines
Quoted 10 lines
device-types/net/description.tex +0
-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_MQorVIRTIO_NET_F_RSSis 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 isnegotiated.
Quoted 20 lines
Quoted 12 lines
(quoted diff) +0
-2-The following driver-read-only field, \field{mtu} only exists if -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU forthe driver to
(quoted diff) +2
-1+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 @@ -261,6 +261,8 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 4 lines
\drivernormative{\subsubsection}{Device configuration
layout}{Device Types / Network Device / Device configuration layout}+All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the driver something actionable:
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
"A driver SHOULD NOT try to write to any of the device configuration fields."
Agree it's not a normative statement as is. MUST NOT actually - they were always read only. And no need to "try" just don't write period.
Saying driver must not write it, doesn't make it read only for the device.
no but this is not what your patch said either. It's read only for the driver.
Quoted 2 lines
Hence, it should be mentioned as read-only fields, so when the driver writes something to read-only fields, it can be considered as undefined behavior on such fields.
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
+
A driver SHOULD negotiateVIRTIO_NET_F_MACif the device offers it. If the driver negotiates theVIRTIO_NET_F_MACfeature, the driver
MUST set the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
RE: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 64 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 59 lines · Parav Pandit
Quoted 11 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:42 PMQuoted 3 lines
What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without reading the result? Pls don't.
It was wrong to keep the "for the driver".
I will fix this.Quoted 45 lines
Quoted 5 lines
Review \subsection{Device configuration
Quoted 5 lines
VIRTIO_NET_S_LINK_UPandVIRTIO_NET_S_ANNOUNCE.\begin{lstlisting}
(quoted diff) +0
-0@@ -167,14 +167,14 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 12 lines
Quoted 4 lines
device-types/net/description.tex +0
-1#define VIRTIO_NET_S_ANNOUNCE 2 \end{lstlisting} -The following driver-read-only field,Quoted 8 lines
\field{max_virtqueue_pairs} only exists if
+The following field, \field{max_virtqueue_pairs} only exists if
VIRTIO_NET_F_MQorVIRTIO_NET_F_RSSis 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 isnegotiated.
Quoted 25 lines
Quoted 14 lines
-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
(quoted diff) +3
-0+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 @@
-261,6 +261,8 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Network DeviceQuoted 5 lines
\drivernormative{\subsubsection}{Device configuration
layout}{Device Types / Network Device / Device configuration layout}+All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the driver something actionable:
"A driver SHOULD NOT try to write to any of the device configuration fields."
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Agree it's not a normative statement as is.
MUST NOT actually - they were always read only.
And no need to "try" just don't write period.Saying driver must not write it, doesn't make it read only for the device.
no but this is not what your patch said either. It's read only for the driver.
It is read-only for the driver because the device doesn't treat them as writable. Do you mean it should be better written as,
These fields are read-only for the driver, hence any writes by the driver to it will be ignored by the device.
?
Quoted 8 lines
Quoted 1 line
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.
Driver section:
Driver must not write to read-only fields.
Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 70 lines · Parav Pandit
Quoted 70 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 64 lines · Parav Pandit
Quoted 11 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:42 PMQuoted 3 lines
What does "bits (for the driver)" mean? It made sense together with "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without reading the result? Pls don't.
It was wrong to keep the "for the driver".
I will fix this.Quoted 42 lines
Quoted 5 lines
Review \subsection{Device configuration
Quoted 5 lines
VIRTIO_NET_S_LINK_UPandVIRTIO_NET_S_ANNOUNCE.\begin{lstlisting}
(quoted diff) +0
-0@@ -167,14 +167,14 @@ \subsection{Device configurationlayout}\label{sec:Device Types / Network Device
Quoted 12 lines
Quoted 4 lines
device-types/net/description.tex +0
-1#define VIRTIO_NET_S_ANNOUNCE 2 \end{lstlisting} -The following driver-read-only field,Quoted 8 lines
\field{max_virtqueue_pairs} only exists if
+The following field, \field{max_virtqueue_pairs} only exists if
VIRTIO_NET_F_MQorVIRTIO_NET_F_RSSis 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 isnegotiated.
Quoted 23 lines
Quoted 14 lines
-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
(quoted diff) +3
-0+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 @@
-261,6 +261,8 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Network DeviceQuoted 5 lines
\drivernormative{\subsubsection}{Device configuration
layout}{Device Types / Network Device / Device configuration layout}+All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the driver something actionable:
Quoted 7 lines
Quoted 2 lines
"A driver SHOULD NOT try to write to any of the device configuration fields."
Agree it's not a normative statement as is.
MUST NOT actually - they were always read only.
And no need to "try" just don't write period.Saying driver must not write it, doesn't make it read only for the device.
Quoted 8 lines
Quoted 2 lines
no but this is not what your patch said either. It's read only for the driver.
It is read-only for the driver because the device doesn't treat them as writable. Do you mean it should be better written as,
These fields are read-only for the driver, hence any writes by the driver to it will be ignored by the device.
?
I just concur with Cornelia.
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.
writes is plural so "are ignored"
but more importantly use rfc terms in normative sections.
Quoted 3 lines
Driver section:
Driver must not write to read-only fields.
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 20 lines · Michael S. Tsirkin
Quoted 10 lines · Parav Pandit
Quoted 10 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 1 line · Parav Pandit
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.
Quoted 3 lines
writes is plural so "are ignored"
but more importantly use rfc terms in normative sections.
I don't think you need to talk about "read-only" in the normative sections (that belongs to the descriptive sections.) I'd use
"The device MUST ignore any writes to config space fields by the driver."
Quoted 4 lines
Quoted 4 lines
Driver section:
Driver must not write to read-only fields.
"The driver MUST NOT write to any config space field."
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 24 lines · Cornelia Huck
Quoted 22 lines · Michael S. Tsirkin
Quoted 10 lines · Parav Pandit
Quoted 10 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 1 line · Parav Pandit
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.writes is plural so "are ignored"
Quoted 7 lines
Quoted 1 line
but more importantly use rfc terms in normative sections.
I don't think you need to talk about "read-only" in the normative sections (that belongs to the descriptive sections.) I'd use
"The device MUST ignore any writes to config space fields by the driver."
Hmm. Is this something we previously required for read only fields?
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Driver section:
Driver must not write to read-only fields."The driver MUST NOT write to any config space field."
[virtio-comment] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 27 lines · Michael S. Tsirkin
Quoted 26 lines · Cornelia Huck
Quoted 23 lines · Michael S. Tsirkin
Quoted 10 lines · Parav Pandit
Quoted 10 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 1 line · Parav Pandit
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.writes is plural so "are ignored"
but more importantly use rfc terms in normative sections.
Quoted 7 lines
Quoted 5 lines
I don't think you need to talk about "read-only" in the normative sections (that belongs to the descriptive sections.) I'd use
"The device MUST ignore any writes to config space fields by the driver."
Hmm. Is this something we previously required for read only fields?
So, better make it SHOULD?
(The only alternative to ignoring I see is breaking the device, and I think ignoring is preferable.)
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Driver section:
Driver must not write to read-only fields."The driver MUST NOT write to any config space field."
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: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
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: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 35 lines · Cornelia Huck
Quoted 33 lines · Michael S. Tsirkin
Quoted 31 lines · Cornelia Huck
Quoted 20 lines · Michael S. Tsirkin
Quoted 10 lines · Parav Pandit
Quoted 10 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 1 line · Parav Pandit
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.Quoted 3 lines
writes is plural so "are ignored"
but more importantly use rfc terms in normative sections.
I don't think you need to talk about "read-only" in the normative sections (that belongs to the descriptive sections.) I'd use
"The device MUST ignore any writes to config space fields by the driver."
Quoted 6 lines
Quoted 1 line
Hmm. Is this something we previously required for read only fields?
So, better make it SHOULD?
(The only alternative to ignoring I see is breaking the device, and I think ignoring is preferable.)
We can just skip adding a new requirement completely - we'll never get there with a compliant driver. This is what we do e.g. for MMIO. Why not?
This has an advantage as this allows backing config with regular RAM. Also I feel that since it always said "read only for driver" then this implies a restriction on driver not the device.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Driver section:
Driver must not write to read-only fields."The driver MUST NOT write to any config space field."
[virtio-comment] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 38 lines · Michael S. Tsirkin
Quoted 37 lines · Cornelia Huck
Quoted 34 lines · Michael S. Tsirkin
Quoted 24 lines · Cornelia Huck
Quoted 22 lines · Michael S. Tsirkin
Quoted 10 lines · Parav Pandit
Quoted 10 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, February 21, 2023 12:52 PMQuoted 1 line · Parav Pandit
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on such fields.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
In the description not in the normative statements. normative sections just tell driver what it must and must not do, in the standard RFC terms.
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are read-only fields for the driver.writes is plural so "are ignored"
Quoted 7 lines
Quoted 1 line
but more importantly use rfc terms in normative sections.
I don't think you need to talk about "read-only" in the normative sections (that belongs to the descriptive sections.) I'd use
"The device MUST ignore any writes to config space fields by the driver."
Hmm. Is this something we previously required for read only fields?
Quoted 8 lines
Quoted 4 lines
So, better make it SHOULD?
(The only alternative to ignoring I see is breaking the device, and I think ignoring is preferable.)
We can just skip adding a new requirement completely - we'll never get there with a compliant driver. This is what we do e.g. for MMIO. Why not?
That would be fine with me as well.
Quoted 3 lines
This has an advantage as this allows backing config with regular RAM. Also I feel that since it always said "read only for driver" then this implies a restriction on driver not the device.
Indeed. The normative statement below should be enough to make that "read-only for the driver" thing obvious.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Driver section:
Driver must not write to read-only fields."The driver MUST NOT write to any config space field."
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: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
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: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
Quoted 9 lines
From: Cornelia Huck <[email protected]>
Sent: Wednesday, February 22, 2023 8:27 AMQuoted 3 lines
We can just skip adding a new requirement completely - we'll never get there with a compliant driver. This is what we do e.g. for MMIO. Why not?
Quoted 18 lines
That would be fine with me as well.
Quoted 3 lines
This has an advantage as this allows backing config with regular RAM. Also I feel that since it always said "read only for driver" then this implies a restriction on driver not the device.
Indeed. The normative statement below should be enough to make that "read- only for the driver" thing obvious.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Driver section:
Driver must not write to read-only fields."The driver MUST NOT write to any config space field."
I addressed the comments in v4 at [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00523.html
Please review.
PATCH v3 2/2 virtio-net: Define cfg fields before description
In reply to Parav PanditCurrently 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
---
changelog:v2->v3:
- split the patch for read only description as prepration patch 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 | 40 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-)
device-types/net/description.tex +22 -18
@@ -156,11 +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. 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 (for the driver) are currently defined for the status field: -VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. +The network device has the following device configuration layout. + +\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
@@ -190,19 +207,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.
Signature · 1 line
2.26.2
Re: [virtio-comment] [PATCH v3 2/2] virtio-net: Define cfg fields before description
Quoted 6 lines · Parav Pandit
Currently some fields of the
virtio_net_configstructure are defined before introducing the structure and some are defined after introducingvirtio_net_config.
Better to define the configuration layout first followed by description of all the fields.
Quoted 7 lines
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
Quoted 71 lines
---
changelog:v2->v3:
- split the patch for read only description as prepration patchv1->v2:
- remove read-only wording from multiple placesv0->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 | 40 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-)device-types/net/description.tex +22
-18@@ -156,11 +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. 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 (for the driver) are currently defined for the status field: -VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE. +The network device has the following device configuration layout. + +\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@@ -190,19 +207,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.
Signature · 17 lines
Tell me sweet little lies. 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: [email protected] Unsubscribe: [email protected] List help: [email protected] 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/
Quoted 4 lines
From: Parav Pandit <[email protected]>
Sent: Friday, February 17, 2023 10:45 AM
Quoted 8 lines
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:
v2->v3:
- split into two patches
- move read only description to driver requirements section
These 2 patches are reviewed.
Can you please open a ballot for it?
Github issue: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00405.html
And the github description has the latest link this time. :)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Cornelia,
Quoted 29 lines
-----Original Message-----
From: Cornelia Huck <[email protected]>
Sent: Monday, March 6, 2023 9:32 AM
To: Parav Pandit <[email protected]>; [email protected]; [email protected] open.org
Cc: [email protected]; Shahaf Shuler <[email protected]>
Subject: RE: [PATCH v3 0/2] virtio-net: Improve dev config layoutQuoted 13 lines · Parav Pandit
Quoted 8 lines
From: Parav Pandit <[email protected]>
Sent: Friday, February 17, 2023 10:45 AMPatch summary:
patch-1: consolidate read only field at one place in driver requirements
patch-2: define device configuration layout before describing its fields.Quoted 4 lines
changelog:
v2->v3:
- split into two patches
- move read only description to driver requirements sectionQuoted 5 lines
These 2 patches are reviewed.
Can you please open a ballot for it?
Github issue:
https://lists.oasis-open.org/archives/virtio-dev/202302/msg00405.html
And the github description has the latest link this time. :)
Quoted 3 lines
I think something went wrong here... the link above points to this series (i.e. not the latest one), and so does the github issue, and therefore the ballot. I think we should withdraw the ballot?
Please withdraw.
Right link is: [1].
Please create the new ballot with link [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00556.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: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
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/
Quoted 35 lines · Parav Pandit
Hi Cornelia,
Quoted 29 lines
-----Original Message-----
From: Cornelia Huck <[email protected]>
Sent: Monday, March 6, 2023 9:32 AM
To: Parav Pandit <[email protected]>; [email protected]; [email protected] open.org
Cc: [email protected]; Shahaf Shuler <[email protected]>
Subject: RE: [PATCH v3 0/2] virtio-net: Improve dev config layoutQuoted 13 lines · Parav Pandit
Quoted 8 lines
From: Parav Pandit <[email protected]>
Sent: Friday, February 17, 2023 10:45 AMPatch summary:
patch-1: consolidate read only field at one place in driver requirements
patch-2: define device configuration layout before describing its fields.Quoted 4 lines
changelog:
v2->v3:
- split into two patches
- move read only description to driver requirements sectionQuoted 5 lines
These 2 patches are reviewed.
Can you please open a ballot for it?
Github issue:
https://lists.oasis-open.org/archives/virtio-dev/202302/msg00405.html
And the github description has the latest link this time. :)Quoted 3 lines
I think something went wrong here... the link above points to this series (i.e. not the latest one), and so does the github issue, and therefore the ballot. I think we should withdraw the ballot?
Quoted 6 lines
Please withdraw.
Right link is: [1].
Please create the new ballot with link [1].[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00556.html
Done.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]