On Tue, Feb 07, 2023 at 03:02:08PM +0000, Parav Pandit wrote:
>
> > From: Cornelia Huck <[email protected]>
> > Sent: Tuesday, February 7, 2023 8:49 AM
> >
> > On Fri, Feb 03 2023, Parav Pandit <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
