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

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

On Thu, Jun 22, 2023 at 05:11:24PM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 1:04 PM
> > To: Parav Pandit <parav@nvidia.com>
> > 
> > On Thu, Jun 22, 2023 at 04:54:48PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, June 22, 2023 12:47 PM
> > >
> > > >
> > > > The hardware footprint of keeping this in memory is also fairly
> > > > small :) I care about a messy interface because this mess builds up over time.
> > > >
> > > It is really a simple GET command.
> > > It is actually messy for the device to implement functionality in two places in
> > cfg space and cvq.
> > >
> > > > And I am worried about capabilities really. My bad that I missed
> > > > this change in v13. I only can say in my defence that I already had
> > > > to rewrite huge chunks of this proposal to make it readable so one
> > > > can't say I'm only delaying things, I also made an effort to help
> > > > this progress faster :)
> > > >
> > > > I feel we need a single place where device capabilities can live. So
> > > > far they were in config space.  It's consistent, yes I get this has
> > > > hardware costs *if* there's a huge number of VFs and *if* there's a
> > > > way to provision each VF with a different configuration.
> > > All the ifs are valid today.
> > >
> > > > And yes querying VFs over MMIO is kind of ugly. But it does at least
> > > > work, and works fine while VF is assigned.  So we can build
> > > > migration around that *today*.
> > > >
> > > Other way to say migration can be skipped for this feature bit, and it still
> > works for rest.
> > 
> > If VF is assigned then we can't really control what does guest enable.
> >
> All parameters set over the CVQ needs to be accessible to the migration entity.
> Including RSS and including this bit.
> Either by trapping the CVQ or by having AQ command to query member dev capabilities.

Yes, parameters set needs to be trapped, so they can be
replayed. This is a capability though. After guest has
driven the CVQ for a while submitting a command to it
so we get capability - I don't see how that works.

> > > > But querying over cvq while VF is assigned clearly *doesn't* work.
> > > >
> > > That is not the idea at all.
> > > Querying VF capabilities is the role of the admin command for which we built
> > it.
> > 
> > This GET is exactly that though.
> > 
> Not exactly.
> This GET command is needed for the member driver to know what is supported for the device it got.

yes. but how will hypervisor get the capability?

> > > > So what is the solution proposed for this?
> > > >
> > > 1. Query member device capabilities via admin command
> > 
> > But that's not 1.3 material.
> > 
> > > > Yes the current migration is broken in many ways but that's what we
> > > > have. Let's build something better sure but that is not 1.3 material.
> > >
> > > True, it is not 1.3 material, hence the proposal was to have the GET command.
> > > Once/if we reach agreement that no new fields to be added to config space
> > starting 1.4 and should be queried using non intercepted cfgvq, it makes sense
> > to let this go in cfg space.
> > > Else GET command seems the elegant and right approach.
> > 
> > I expect no such agreement at all. Instead, I expect that we'll have an alternative
> > way to access config space. guest virtio core then needs to learn both ways, and
> > devices can support one or both.
> > 
> Yeah, we disagree.
> Because alternative way that you propose is not predictable way to build the device efficiently.
> It always needs to account for old driver to support.
> This is clearly sub-optimal as the capabilities grow.

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.

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

-- 
MST