RE: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, April 25, 2023 5:06 PM
> > On 4/23/2023 3:35 AM, Heng Qi wrote:
> > >   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > Types / Network Device / Feature bits / Legacy Interface: Feature bits} @@
> -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
> Types / Network Device
> > >           u8 rss_max_key_size;
> > >           le16 rss_max_indirection_table_length;
> > >           le32 supported_hash_types;
> > > +        le32 supported_tunnel_hash_types;
> 
> this needs a comment explaining it only exists with some feature bits.
> 
Yes, it is already there.
+Field \field{supported_tunnel_hash_types} only exists if the device supports inner header hash, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
+
I think it should be changed from "device supports" to "driver negotiated".

> > >   };
> > In v12 I was asking this to move to above field from the config area
> > to the GET command in comment [1] as,
> >
> > "With that no need to define two fields at two different places in
> > config area and also in cvq."
> 
> I think I disagree.
> the proposed design is consistent with regular tunneling.
> 
Sure.
I understand how config space has evolved from 0.9.5 to know without much attention, but really expanding this way is not helpful.
It requires building more and more RAM based devices even for PCI PFs, which is sub optimal.
CVQ already exists and provides the SET command. There is no reason to do GET in some other way.
Device has single channel to provide a value and hence it doesn't need any internal synchronization between two paths.

So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an improvement.
But it still comes with a cost which device cannot mitigate.
The cost is, 
1. a driver may not negotiate such feature bit, and device ends up burning memory.
1.b Device provisioning becomes a factor of what some guests may use/not use/already using on the live system.

2. Every device needs AQ even when the CVQ exists.

Hence, better to avoid expanding current structure for any new fields, specially which has the SET equivalent.

But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine.
More things can evolve for generic things like config space over AQ.