[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 01:56:34AM +0000, Parav Pandit wrote: > > > > From: Michael S. Tsirkin <mst@redhat.com> > > Sent: Wednesday, June 28, 2023 3:45 PM > > > > Maybe I get it. You want to use the new features as a carrot to > > > > force drivers to implement DMA? You suspect they will ignore the > > > > spec requirement just because things seem to work? > > > > > > > Right because it is not a must normative. > > > > Well SHOULD also does not mean "ok to just ignore". > > > > This word, or the adjective "RECOMMENDED", mean that there > > may exist valid reasons in particular circumstances to ignore a > > particular item, but the full implications must be understood and > > carefully weighed before choosing a different course. > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. > So rather a good design is device tells the starting offset for the extended config space. > And extended config space MUST be accessed using a DMA. > With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from. > This also gives the ability to maintain current config as MMIO for backward compatibility. > > > > > > > > > There's some logic here, for sure. you just might be right. > > > > > > > > However, surely we can discuss this small tweak in 1.4 timeframe? > > > > > > Sure, if we prefer the DMA approach I don't have a problem in adding > > temporary one field to config space. > > > > > > I propose to add a line to the spec " Device Configuration Space" > > > section, something like, > > > > > > Note: Any new device configuration space fields additional MUST consider > > accessing such fields via a DMA interface. > > > > > > And this will guide the new patches of what to do instead of last moment > > rush. > > > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to > > MMIO might be an option for qemu helping us debug DMA issues. > > > There are too many queues whose debugging is needed and MMIO likely not the way to debug. > > > The time to discuss this detail would be around when proposal for the DMA > > access to config space is on list though: I feel this SHOULD vs MUST is a small > > enough detail. > > > From implementation POV it is certainly critical and good step forward to optimize virtio interface. > > > Going back to inner hash. If we move supported_tunnels back to config space, > > do you feel we still need GET or just drop it? I note we do not have GET for > > either hash or rss config. > > > For hash and rss config, debugging is missing. :) > Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config. > I would like to make sure if we're aligned. The new version should contain the following: 1. The supported_tunnel_types are placed in the device config space; 2. Reserve the following structure: struct virtnet_hash_tunnel { le32 enabled_tunnel_types; }; 3. Reserve the SET command for enabled_tunnel_types and remove the GET command for enabled_tunnel_types. If there is no problem, I will modify it accordingly. Thanks! > > And if we no longer have GET is there still a reason for a separate command as > > opposed to a field in virtio_net_hash_config? > > I know this was done in v11 but there it was misaligned. > > We went with a command because we needed it for supported_tunnels but > > now that is no longer the case and there are reserved words in > > virtio_net_hash_config ... > > > > Let me know how you feel it about that, not critical for me. > > struct virtio_net_hash_config reserved is fine.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]