[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] virtio-net: Define configuration field layout before its description
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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]