Re: [PATCH] virtio-net: Define configuration field layout before its description

["Michael S. Tsirkin" <mst@redhat.com>]

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."