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

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 6:41 AM

> > > So just quickly add the new capability in the spec and then the
> > > number of linux releases that will have the new feature but not
> > > config command or whatever that is will be too small for vendors to care.
> > >
> > I didn't follow this suggestion.
> 
> It is very simple though. 1.3 has inner hash feature. Imagine that instead of
> endless flamewars rehashing same arguments, immediately post

For sure if you call this discussion a war, I didn't start it and I didn't delay it to this point either :)

Patch added GET and SET command, GET returns device static and dynamic value, without burdening the device in consistent way.
Will moving to config space, take away the GET command? No?

Then what did one gain other than extra complexity of additional config space? Just more device memory burning.

> 1.3 we vote on an interface to access config space through DMA (not sure it's a
> VQ BTW but that's a separate discussion).  This should include a way to expose
> a subset of DMA features through MMIO, for compatibility.

I think you are missing the main point that I try to highlight but presumably it didn't come across. :(

Having an optional DMA interface does _not_ help device to optimize, because device must be support old drivers.

The proposal I explained in previous email is:
a. All new fields via vq
This means it is _guaranteed_ to be offchip and zero reason for backward compatibility, because there is no backward compat of non existing fields.

b. all existing fields stay in cfg space

c. Optionally existing fields can also be queried via cfg space

> If we are lucky guest support can land in the same Linux release that has inner
> hash.  Drivers do not need to care: they just do virtio_cread and that is it.
>
virtio_cread for extended config space accessible only via DMA or VQ will surely be hidden inside th virtio_cread(). Easy.
 
If we agree on this approach, great, one additional field like this can be added to config space.

> So a vendor that builds device with inner hash, can expose inner hash only
> through DMA and not through MMIO.
> 
> I conclude that there's no reason to block this feature just because it uses config
> space.
>
I must repeat that I am not blocking, I replied to way back on both the approaches in v14 or even before that.
And also explained part of the proposal.

This is why one should discuss the design and not keep asking for patches..
 
> 
> > > >
> > > > > A good implementation of virtio_cread can abstract that easily
> > > > > so we don't need to change drivers.
> > > >
> > > > There is no backward compat issue for the GET command being new.
> > >
> > > It's just a shortcut replacing what we really want.  As long as a
> > > shortcut is available people will keep using exactly that.  So I
> > > fully expect more proposals for such GET commands on the pretext
> > > that one is there so why not another one. Adding more tech debt for
> > > whoever finally gets around to building a config space access gateway.
> > >
> > Not really. as suggested, the first addition of new field to the config space in
> 1.4-time frame, should add the cfgvq, and not follow the previous example.
> > Because this is being thought through now, it is not at all hard for any new
> things to follow the guideline.
> 
> Really. Oh great, so there will be 3 ways to provision things!
>
Not sure how you concluded 3 ways...

Provision is via single command via AQ by the owner device.
New config fields via VQ of member device itself, again single way.
Existing config fields access via config space, single way.

> I have not seen this patch yet.  And how long will this take to materialize? I don't
> believe all TC work must be blocked until this happens, or alternatively use ad-
> hock hacks.
It is surely not the right way to ask for the patch when doing the design discussion and claiming that it is not ready.

> 
> I get it you want to save on chip memory. So work on a consistent soltion for
> this.
> 
> All config space accesses should go through DMA.
This is not predictable due to backward compat need.
Hence the ask is all *new* config space access go through DMA.

> Thus no special guidelines should be necessary, and drivers can just keep doing
> virtio_cread like they always did.
>
This can be very easily achieved in sw by knowing extended config space offset to use DMA.
 
> To add to that, it will allow cheaper devices as some existing config space will be
> able to move quickly.
This is possible only in those config when device side can know early enough, through a new feature bit and new registers for DMA, which will pay off for larger config space extended region.


I want to summarize my humble input:

1. all new extended config space fields to be accessed only via DMA/VQ by member device itself.
2. all existing fields to stay in config space without any interface change
3. optionally existing fields can be accessed vid DMA/VQ by member device itself.

4. If above proposal looks reasonable, sure, have the inner hash field of this patch as part of existing config space. I have no problem with it.

5. any new fields proposed in extended config space in 1.4-time frame must implement DMA/VQ interface as part of the proposal.

6. And if this needs more discussion, lets spend quality time on it and differ the release by a week and at least agree on the path forward.
No need to draft the spec normative language for design.