[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]