* [PATCH] virtio-net: Define configuration field layout before its description @ 2023-02-03 14:43 Parav Pandit 2023-02-07 13:49 ` [virtio-comment] " Cornelia Huck 0 siblings, 1 reply; 8+ messages in thread From: Parav Pandit @ 2023-02-03 14:43 UTC (permalink / raw) To: mst, virtio-dev, cohuck; +Cc: virtio-comment, shahafs, Parav Pandit 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 Signed-off-by: Parav Pandit <parav@nvidia.com> --- device-types/net/description.tex | 39 +++++++++++++++++--------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/device-types/net/description.tex b/device-types/net/description.tex index dedd6b1..d4f598b 100644 --- a/device-types/net/description.tex +++ b/device-types/net/description.tex @@ -154,11 +154,27 @@ \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 described below, they are read-only for a driver. + +\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 (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. \begin{lstlisting} #define VIRTIO_NET_S_LINK_UP 1 @@ -188,19 +204,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] 8+ messages in thread
* [virtio-comment] Re: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-03 14:43 [PATCH] virtio-net: Define configuration field layout before its description Parav Pandit @ 2023-02-07 13:49 ` Cornelia Huck 2023-02-07 15:02 ` Parav Pandit 0 siblings, 1 reply; 8+ messages in thread From: Cornelia Huck @ 2023-02-07 13:49 UTC (permalink / raw) To: Parav Pandit, mst, virtio-dev; +Cc: virtio-comment, shahafs, Parav Pandit On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > Better to define the configuration layout first followed by > description of all the fields. I see that some other devices (e.g. block) list the config layout _after_ all of the descriptions, although I think listing first and then describing is the better approach. However, in-between is the worst order, and just cleaning up this one right now makes sense. > > 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 > > Signed-off-by: Parav Pandit <parav@nvidia.com> > --- > device-types/net/description.tex | 39 +++++++++++++++++--------------- > 1 file changed, 21 insertions(+), 18 deletions(-) > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex > index dedd6b1..d4f598b 100644 > --- a/device-types/net/description.tex > +++ b/device-types/net/description.tex > @@ -154,11 +154,27 @@ \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 described below, they are read-only for a driver. Maybe replace that with: "The network device uses the following device configuration layout. The fields are read-only for the driver." > + > +\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 (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. As you are touching this anyway, maybe break it up? "The \field{mac} address field always exists (although it is only valid if VIRTIO_NET_F_MAC is set). \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." > > \begin{lstlisting} > #define VIRTIO_NET_S_LINK_UP 1 > @@ -188,19 +204,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. 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] 8+ messages in thread
* RE: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 13:49 ` [virtio-comment] " Cornelia Huck @ 2023-02-07 15:02 ` Parav Pandit 2023-02-07 15:12 ` Michael S. Tsirkin 2023-02-07 15:49 ` [virtio-comment] " Cornelia Huck 0 siblings, 2 replies; 8+ messages in thread From: Parav Pandit @ 2023-02-07 15:02 UTC (permalink / raw) To: Cornelia Huck, mst@redhat.com, virtio-dev@lists.oasis-open.org Cc: virtio-comment@lists.oasis-open.org, Shahaf Shuler > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, February 7, 2023 8:49 AM > > On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > > Better to define the configuration layout first followed by > > description of all the fields. > > I see that some other devices (e.g. block) list the config layout _after_ all of the > descriptions, although I think listing first and then describing is the better > approach. However, in-between is the worst order, and just cleaning up this > one right now makes sense. > Yes. block can be improved too. I will send separate patch for block side later. > > > > 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 > > > > Signed-off-by: Parav Pandit <parav@nvidia.com> > > --- > > device-types/net/description.tex | 39 > > +++++++++++++++++--------------- > > 1 file changed, 21 insertions(+), 18 deletions(-) > > > > diff --git a/device-types/net/description.tex > > b/device-types/net/description.tex > > index dedd6b1..d4f598b 100644 > > --- a/device-types/net/description.tex > > +++ b/device-types/net/description.tex > > @@ -154,11 +154,27 @@ \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 described below, they are read-only for a > driver. > > Maybe replace that with: > > "The network device uses the following device configuration layout. The fields > are read-only for the driver." > I want to avoid "uses" term. Because it is the device configuration layout built in the device. How about, 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 (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. > > As you are touching this anyway, maybe break it up? > > "The \field{mac} address field always exists (although it is only valid if > VIRTIO_NET_F_MAC is set). > I want to avoid such change in this patch. This whole section about "exist" is very confusing. Because structure layout is not going to change when field don't "exist". But that is counter intuitive for the term "exist". And hence the "exist" wording is incorrect. The size of the configuration layout is totally defined by the transport. And validity of the field is driven by the feature bit and at some extent structure size can be shorter depending on feature. So I want to park this "exist" cleanup at later point. > \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." > > > > > \begin{lstlisting} > > #define VIRTIO_NET_S_LINK_UP 1 > > @@ -188,19 +204,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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 15:02 ` Parav Pandit @ 2023-02-07 15:12 ` Michael S. Tsirkin 2023-02-07 15:49 ` [virtio-comment] " Cornelia Huck 1 sibling, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2023-02-07 15:12 UTC (permalink / raw) To: Parav Pandit Cc: Cornelia Huck, virtio-dev@lists.oasis-open.org, virtio-comment@lists.oasis-open.org, Shahaf Shuler On Tue, Feb 07, 2023 at 03:02:08PM +0000, Parav Pandit wrote: > > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Tuesday, February 7, 2023 8:49 AM > > > > On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > > > Better to define the configuration layout first followed by > > > description of all the fields. > > > > I see that some other devices (e.g. block) list the config layout _after_ all of the > > descriptions, although I think listing first and then describing is the better > > approach. However, in-between is the worst order, and just cleaning up this > > one right now makes sense. > > > Yes. block can be improved too. > I will send separate patch for block side later. > > > > > > > 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 > > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com> > > > --- > > > device-types/net/description.tex | 39 > > > +++++++++++++++++--------------- > > > 1 file changed, 21 insertions(+), 18 deletions(-) > > > > > > diff --git a/device-types/net/description.tex > > > b/device-types/net/description.tex > > > index dedd6b1..d4f598b 100644 > > > --- a/device-types/net/description.tex > > > +++ b/device-types/net/description.tex > > > @@ -154,11 +154,27 @@ \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 described below, they are read-only for a > > driver. > > > > Maybe replace that with: > > > > "The network device uses the following device configuration layout. The fields > > are read-only for the driver." > > > I want to avoid "uses" term. Because it is the device configuration layout built in the device. > How about, > 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 (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. > > > > As you are touching this anyway, maybe break it up? > > > > "The \field{mac} address field always exists (although it is only valid if > > VIRTIO_NET_F_MAC is set). > > > I want to avoid such change in this patch. > This whole section about "exist" is very confusing. Because structure layout is not going to change when field don't "exist". But that is counter intuitive for the term "exist". > And hence the "exist" wording is incorrect. > The size of the configuration layout is totally defined by the transport. > And validity of the field is driven by the feature bit and at some extent structure size can be shorter depending on feature. > So I want to park this "exist" cleanup at later point. Yes it's a thorny issue generally. We also have confusion between "valid if feature negotiated" and "valid if feature offered". and generally it is vague what does "feature negotiated" mean before FEATURES_OK is set. I had a patch for that at some point, but there are old drivers that violate almost all thinkable rule you can come up with so we need to at least put in a bunch of disclaimers and be as permissive as we can. > > \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." > > > > > > > > \begin{lstlisting} > > > #define VIRTIO_NET_S_LINK_UP 1 > > > @@ -188,19 +204,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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [virtio-comment] RE: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 15:02 ` Parav Pandit 2023-02-07 15:12 ` Michael S. Tsirkin @ 2023-02-07 15:49 ` Cornelia Huck 2023-02-07 21:33 ` Parav Pandit 1 sibling, 1 reply; 8+ messages in thread From: Cornelia Huck @ 2023-02-07 15:49 UTC (permalink / raw) To: Parav Pandit, mst@redhat.com, virtio-dev@lists.oasis-open.org Cc: virtio-comment@lists.oasis-open.org, Shahaf Shuler On Tue, Feb 07 2023, Parav Pandit <parav@nvidia.com> wrote: >> From: Cornelia Huck <cohuck@redhat.com> >> Sent: Tuesday, February 7, 2023 8:49 AM >> >> On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. >> > Better to define the configuration layout first followed by >> > description of all the fields. >> >> I see that some other devices (e.g. block) list the config layout _after_ all of the >> descriptions, although I think listing first and then describing is the better >> approach. However, in-between is the worst order, and just cleaning up this >> one right now makes sense. >> > Yes. block can be improved too. > I will send separate patch for block side later. I think there were one or two others; but I consider none of this urgent :) > >> > >> > 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 >> > >> > Signed-off-by: Parav Pandit <parav@nvidia.com> >> > --- >> > device-types/net/description.tex | 39 >> > +++++++++++++++++--------------- >> > 1 file changed, 21 insertions(+), 18 deletions(-) >> > >> > diff --git a/device-types/net/description.tex >> > b/device-types/net/description.tex >> > index dedd6b1..d4f598b 100644 >> > --- a/device-types/net/description.tex >> > +++ b/device-types/net/description.tex >> > @@ -154,11 +154,27 @@ \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 described below, they are read-only for a >> driver. >> >> Maybe replace that with: >> >> "The network device uses the following device configuration layout. The fields >> are read-only for the driver." >> > I want to avoid "uses" term. Because it is the device configuration layout built in the device. > How about, > The network device has the following device configuration layout. Works for me. > >> > + >> > +\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 (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. >> >> As you are touching this anyway, maybe break it up? >> >> "The \field{mac} address field always exists (although it is only valid if >> VIRTIO_NET_F_MAC is set). >> > I want to avoid such change in this patch. This is only splitting up the sentence and tweaking the grammar, which I consider a rather minor change. > This whole section about "exist" is very confusing. Because structure layout is not going to change when field don't "exist". But that is counter intuitive for the term "exist". > And hence the "exist" wording is incorrect. I think we have been through that discussion before... would need to look through the archives. > The size of the configuration layout is totally defined by the transport. No, the transport only defines how the config is accessed? > And validity of the field is driven by the feature bit and at some extent structure size can be shorter depending on feature. > So I want to park this "exist" cleanup at later point. Certainly, this patch should only do a simple cleanup. >> \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." 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] 8+ messages in thread
* RE: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 15:49 ` [virtio-comment] " Cornelia Huck @ 2023-02-07 21:33 ` Parav Pandit 2023-02-07 21:58 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Parav Pandit @ 2023-02-07 21:33 UTC (permalink / raw) To: Cornelia Huck, mst@redhat.com, virtio-dev@lists.oasis-open.org Cc: virtio-comment@lists.oasis-open.org, Shahaf Shuler > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, February 7, 2023 10:50 AM > > On Tue, Feb 07 2023, Parav Pandit <parav@nvidia.com> wrote: > > >> From: Cornelia Huck <cohuck@redhat.com> > >> Sent: Tuesday, February 7, 2023 8:49 AM > >> > >> On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > >> > Better to define the configuration layout first followed by > >> > description of all the fields. > >> > >> I see that some other devices (e.g. block) list the config layout > >> _after_ all of the descriptions, although I think listing first and > >> then describing is the better approach. However, in-between is the > >> worst order, and just cleaning up this one right now makes sense. > >> > > Yes. block can be improved too. > > I will send separate patch for block side later. > > I think there were one or two others; but I consider none of this urgent > :) > o.k. Will look into it. > > > >> > > >> > 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.ht > >> > ml > >> > > >> > Signed-off-by: Parav Pandit <parav@nvidia.com> > >> > --- > >> > device-types/net/description.tex | 39 > >> > +++++++++++++++++--------------- > >> > 1 file changed, 21 insertions(+), 18 deletions(-) > >> > > >> > diff --git a/device-types/net/description.tex > >> > b/device-types/net/description.tex > >> > index dedd6b1..d4f598b 100644 > >> > --- a/device-types/net/description.tex > >> > +++ b/device-types/net/description.tex > >> > @@ -154,11 +154,27 @@ \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 described below, they are > >> > +read-only for a > >> driver. > >> > >> Maybe replace that with: > >> > >> "The network device uses the following device configuration layout. > >> The fields are read-only for the driver." > >> > > I want to avoid "uses" term. Because it is the device configuration layout built > in the device. > > How about, > > The network device has the following device configuration layout. > > Works for me. > Ok. > > > >> > + > >> > +\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 (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. > >> > >> As you are touching this anyway, maybe break it up? > >> > >> "The \field{mac} address field always exists (although it is only > >> valid if VIRTIO_NET_F_MAC is set). > >> > > I want to avoid such change in this patch. > > This is only splitting up the sentence and tweaking the grammar, which I > consider a rather minor change. > > > This whole section about "exist" is very confusing. Because structure layout is > not going to change when field don't "exist". But that is counter intuitive for the > term "exist". > > And hence the "exist" wording is incorrect. > > I think we have been through that discussion before... would need to look > through the archives. > > > The size of the configuration layout is totally defined by the transport. > > No, the transport only defines how the config is accessed? > VIRTIO_PCI_CAP_DEVICE_CFG for PCI and length field in virtio_pci_cap tells how big the config layout. Rest of the "existence" complexity I explained above. > > And validity of the field is driven by the feature bit and at some extent > structure size can be shorter depending on feature. > > So I want to park this "exist" cleanup at later point. > > Certainly, this patch should only do a simple cleanup. > Ok. Will send v2. > >> \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." ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 21:33 ` Parav Pandit @ 2023-02-07 21:58 ` Michael S. Tsirkin 2023-02-07 22:45 ` Parav Pandit 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2023-02-07 21:58 UTC (permalink / raw) To: Parav Pandit Cc: Cornelia Huck, virtio-dev@lists.oasis-open.org, virtio-comment@lists.oasis-open.org, Shahaf Shuler On Tue, Feb 07, 2023 at 09:33:11PM +0000, Parav Pandit wrote: > > > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Tuesday, February 7, 2023 10:50 AM > > > > On Tue, Feb 07 2023, Parav Pandit <parav@nvidia.com> wrote: > > > > >> From: Cornelia Huck <cohuck@redhat.com> > > >> Sent: Tuesday, February 7, 2023 8:49 AM > > >> > > >> On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > > >> > Better to define the configuration layout first followed by > > >> > description of all the fields. > > >> > > >> I see that some other devices (e.g. block) list the config layout > > >> _after_ all of the descriptions, although I think listing first and > > >> then describing is the better approach. However, in-between is the > > >> worst order, and just cleaning up this one right now makes sense. > > >> > > > Yes. block can be improved too. > > > I will send separate patch for block side later. > > > > I think there were one or two others; but I consider none of this urgent > > :) > > > o.k. > Will look into it. > > > > > > >> > > > >> > 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.ht > > >> > ml > > >> > > > >> > Signed-off-by: Parav Pandit <parav@nvidia.com> > > >> > --- > > >> > device-types/net/description.tex | 39 > > >> > +++++++++++++++++--------------- > > >> > 1 file changed, 21 insertions(+), 18 deletions(-) > > >> > > > >> > diff --git a/device-types/net/description.tex > > >> > b/device-types/net/description.tex > > >> > index dedd6b1..d4f598b 100644 > > >> > --- a/device-types/net/description.tex > > >> > +++ b/device-types/net/description.tex > > >> > @@ -154,11 +154,27 @@ \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 described below, they are > > >> > +read-only for a > > >> driver. > > >> > > >> Maybe replace that with: > > >> > > >> "The network device uses the following device configuration layout. > > >> The fields are read-only for the driver." > > >> > > > I want to avoid "uses" term. Because it is the device configuration layout built > > in the device. > > > How about, > > > The network device has the following device configuration layout. > > > > Works for me. > > > Ok. > > > > > > >> > + > > >> > +\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 (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. > > >> > > >> As you are touching this anyway, maybe break it up? > > >> > > >> "The \field{mac} address field always exists (although it is only > > >> valid if VIRTIO_NET_F_MAC is set). > > >> > > > I want to avoid such change in this patch. > > > > This is only splitting up the sentence and tweaking the grammar, which I > > consider a rather minor change. > > > > > This whole section about "exist" is very confusing. Because structure layout is > > not going to change when field don't "exist". But that is counter intuitive for the > > term "exist". > > > And hence the "exist" wording is incorrect. > > > > I think we have been through that discussion before... would need to look > > through the archives. > > > > > The size of the configuration layout is totally defined by the transport. > > > > No, the transport only defines how the config is accessed? > > > VIRTIO_PCI_CAP_DEVICE_CFG for PCI and length field in virtio_pci_cap tells how big the config layout. There's explicit text in the spec saying this can be larger than necessary. So length there does not indicate field existence at all. > Rest of the "existence" complexity I explained above. > > > > And validity of the field is driven by the feature bit and at some extent > > structure size can be shorter depending on feature. > > > So I want to park this "exist" cleanup at later point. > > > > Certainly, this patch should only do a simple cleanup. > > > Ok. Will send v2. > > > >> \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." ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] virtio-net: Define configuration field layout before its description 2023-02-07 21:58 ` Michael S. Tsirkin @ 2023-02-07 22:45 ` Parav Pandit 0 siblings, 0 replies; 8+ messages in thread From: Parav Pandit @ 2023-02-07 22:45 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Cornelia Huck, virtio-dev@lists.oasis-open.org, virtio-comment@lists.oasis-open.org, Shahaf Shuler > From: Michael S. Tsirkin <mst@redhat.com> > Sent: Tuesday, February 7, 2023 4:58 PM > > On Tue, Feb 07, 2023 at 09:33:11PM +0000, Parav Pandit wrote: > > > > > > > From: Cornelia Huck <cohuck@redhat.com> > > > Sent: Tuesday, February 7, 2023 10:50 AM > > > > > > On Tue, Feb 07 2023, Parav Pandit <parav@nvidia.com> wrote: > > > > > > >> From: Cornelia Huck <cohuck@redhat.com> > > > >> Sent: Tuesday, February 7, 2023 8:49 AM > > > >> > > > >> On Fri, Feb 03 2023, Parav Pandit <parav@nvidia.com> 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. > > > >> > Better to define the configuration layout first followed by > > > >> > description of all the fields. > > > >> > > > >> I see that some other devices (e.g. block) list the config layout > > > >> _after_ all of the descriptions, although I think listing first > > > >> and then describing is the better approach. However, in-between > > > >> is the worst order, and just cleaning up this one right now makes sense. > > > >> > > > > Yes. block can be improved too. > > > > I will send separate patch for block side later. > > > > > > I think there were one or two others; but I consider none of this > > > urgent > > > :) > > > > > o.k. > > Will look into it. > > > > > > > > > >> > > > > >> > 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/msg0000 > > > >> > 4.ht > > > >> > ml > > > >> > > > > >> > Signed-off-by: Parav Pandit <parav@nvidia.com> > > > >> > --- > > > >> > device-types/net/description.tex | 39 > > > >> > +++++++++++++++++--------------- > > > >> > 1 file changed, 21 insertions(+), 18 deletions(-) > > > >> > > > > >> > diff --git a/device-types/net/description.tex > > > >> > b/device-types/net/description.tex > > > >> > index dedd6b1..d4f598b 100644 > > > >> > --- a/device-types/net/description.tex > > > >> > +++ b/device-types/net/description.tex > > > >> > @@ -154,11 +154,27 @@ \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 described below, they are > > > >> > +read-only for a > > > >> driver. > > > >> > > > >> Maybe replace that with: > > > >> > > > >> "The network device uses the following device configuration layout. > > > >> The fields are read-only for the driver." > > > >> > > > > I want to avoid "uses" term. Because it is the device > > > > configuration layout built > > > in the device. > > > > How about, > > > > The network device has the following device configuration layout. > > > > > > Works for me. > > > > > Ok. > > > > > > > > > >> > + > > > >> > +\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 (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. > > > >> > > > >> As you are touching this anyway, maybe break it up? > > > >> > > > >> "The \field{mac} address field always exists (although it is only > > > >> valid if VIRTIO_NET_F_MAC is set). > > > >> > > > > I want to avoid such change in this patch. > > > > > > This is only splitting up the sentence and tweaking the grammar, > > > which I consider a rather minor change. > > > > > > > This whole section about "exist" is very confusing. Because > > > > structure layout is > > > not going to change when field don't "exist". But that is counter > > > intuitive for the term "exist". > > > > And hence the "exist" wording is incorrect. > > > > > > I think we have been through that discussion before... would need to > > > look through the archives. > > > > > > > The size of the configuration layout is totally defined by the transport. > > > > > > No, the transport only defines how the config is accessed? > > > > > VIRTIO_PCI_CAP_DEVICE_CFG for PCI and length field in virtio_pci_cap tells > how big the config layout. > > There's explicit text in the spec saying this can be larger than necessary. So > length there does not indicate field existence at all. > It can be larger but has to be minimum upto the length that accommodate the published features bits. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-07 22:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-03 14:43 [PATCH] virtio-net: Define configuration field layout before its description Parav Pandit 2023-02-07 13:49 ` [virtio-comment] " Cornelia Huck 2023-02-07 15:02 ` Parav Pandit 2023-02-07 15:12 ` Michael S. Tsirkin 2023-02-07 15:49 ` [virtio-comment] " Cornelia Huck 2023-02-07 21:33 ` Parav Pandit 2023-02-07 21:58 ` Michael S. Tsirkin 2023-02-07 22:45 ` Parav Pandit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.