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

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

On Tue, Apr 25, 2023 at 09:39:28PM +0000, Parav Pandit wrote:
> 
> > 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.

No, this is ROM, not RAM.

> CVQ already exists and provides the SET command. There is no reason to do GET in some other way.

Spoken looking at just hardware cost :)
The reason is that this is device specific. Maintainance overhead and
system RAM cost for the code to support this should not be ignored.

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

I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are
these costs.  What I had in mind is extending proposed vq transport to
support sriov. I don't see why we can not have exactly 0 bytes of memory
per VF.

And if we care about single bytes of PF memory (do we? there's only one
PF per SRIOV device ...), what we should do is a variant of pci
transport that aggressively saves memory.


-- 
MST