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

2023-06-22 Thread Parav Pandit



> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 3:02 PM


> > Existing fields used by existing drivers must be in MMIO.
> > Device does not have a choice.
> 
> They can if they like mask some less important features reducing the 
> footprint.
How do to that? How would device know to not have existing fields on MMIO?

> This will be the cost/benefit analysis each vendor does separately.
> 
> Case in point, if inner hash support is in 1.3, and linux drivers don't 
> really use
> that at all for a year or two because it's only used by alibaba in their 
> stack, and
> by the time linux starts using them it also supports the new commands, then
> practically devices do not care and can just mask the feature bit in MMIO.
> 
Once it lands to config space, it becomes existing fields.
More below.
> 

> > > If they want to cut at 1.3 time, they can, but they also can cut it
> > > at 1.2 time, this means some features will not be accessible through
> > > MMIO only through the new commands.
> > >
> > New commands services new fields by newer driver.
> > Newer driver can call new command to get new and old fields both.
> 
> yes.
> 
> > So 1.3 and 1.4 devices cannot optimize for older drivers.
> 
> 
> I don't know what this means.
> 
I guess next below question answers it.
 
> >
> > > > Once cfgvq is present for new fields, from the day 1, device does
> > > > not need to
> > > store any newly defined fields on MMIO.
> > >
> > > Exactly.
> > >
> > > > And 10 to 20 years, it can stop for existing fields..
> > > >
> > > > The clear benefit is fields defined in 1.4 no longer needs to be
> > > > stored starting
> > > from 2023-24.
> > >
> > > And why is it a problem that someone can also build a 1.4 device with
> MMIO?
> > >
> > Why to build device using large MMIO when those fields are accessible via 
> > vq.
> 
> If you don't want to - don't. But it's simple and handy for software.
> Not all devices are complex beasts like net.

Every single device has a VQ to my knowledge.
So VQ is already simple for communication even without cvq.

> 
> Apropos, I am not yet sure the best way is simply adding admin vq to vfs, in 
> that
> this only works for fields not needed before DRIVER_OK.
> Ideally I think we should fix all of config space, including device and common
> config. I have some ideas but let's not get ahead of ourselves.
> 
> > VQ construct exists so it is already simple.
> >
> > If the spec is defined in a way, that new fields must be accessible via vq, 
> > and
> optionally via MMIO, than it is ok.
> > One can choose to build via MMIO.
> > So MMIO for new fields must not be mandatory.
> 
> For devices, yes. I am suggesting mandating it for drivers.
It must be mandatory for the driver to support VQ, only than it works.
If driver does not implement, and device only offers via VQ, it does not work.

> Or maybe it's enough to make it a SHOULD.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 06:50:16PM +, Parav Pandit wrote:
> 
> 
> > From: virtio-comm...@lists.oasis-open.org  > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 2:40 PM
> > To: Parav Pandit 
> > Cc: Heng Qi ; virtio-comm...@lists.oasis-open.org;
> > virtio-dev@lists.oasis-open.org; Jason Wang ; Yuri
> > Benditovich ; Xuan Zhuo
> > ; Cornelia Huck 
> > Subject: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 
> > v18]
> > virtio-net: support inner header hash
> > 
> > On Thu, Jun 22, 2023 at 06:17:54PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: virtio-dev@lists.oasis-open.org
> > > >  On Behalf Of Michael S. Tsirkin
> > > > Sent: Thursday, June 22, 2023 2:12 PM
> > >
> > > [..]
> > >
> > > > > > > The proposal for 1.4 is literally very simple as below.
> > > > > > > 1. All existing fields of cfg space stays in cfg space 2. Any
> > > > > > > new capabilities to be queried, query using a vq (aq, cfgvq,
> > whatevervq).
> > > > > > > 3. Optionally existing fields can be queries over vq of #2
> > > > > > > Once this arrive, no need for new GET commands.
> > > > > > > Till that time, don't keep infinitely grow the cfg space.
> > > > > > > Any next addition to cfg space, should work on defining the cfgvq.
> > > > > >
> > > > > > Simple, but short sighted. I know you guys don't support your
> > > > > > hardware for 10-
> > > > > > 20 years but for software people do.
> > > > > > And so "All existing fields of cfg space stays in cfg space" is
> > > > > > a bad idea simply because this does not allow removing things
> > > > > > from config space not in 10 not in
> > > > > > 20 years not ever.
> > > > > >
> > > > > #1 is for backward compat for existing drivers.
> > > > >  You missed about #3. Existing cfg space fields can be queries
> > > > > using the cfgvq
> > > > too.
> > > >
> > > > Then #1 does not matter. We can give devices choice.
> > > >
> > > > > >
> > > > > > Instead we need to allow two ways to access config space.  Teach
> > > > > > drivers about both, actually mandate supporting both.  And then
> > > > > > devices will make their own cost/benefit decision about which
> > > > > > features they
> > > > want to support in MMIO.
> > > > >
> > > > > If both method is mandated, I don't see benefit at all of two methods.
> > > >
> > > > Mandated for driver.
> > > > Benefit is for devices, they will have the choice which drivers to
> > > > support. In 10-
> > > > 20 years all drivers support cfg command and then people can start
> > > > shipping devices without MMIO access to any registers.
> > > >
> > > Until that point devices are forced to burn memory, which is not needed 
> > > at all
> > in < 3 years.
> > 
> > No they are not. They can make their own decision which fields to support in
> > MMIO space.
> Existing fields used by existing drivers must be in MMIO.
> Device does not have a choice.

They can if they like mask some less important features reducing the footprint.
This will be the cost/benefit analysis each vendor does separately.

Case in point, if inner hash support is in 1.3, and linux drivers don't
really use that at all for a year or two because it's only used by
alibaba in their stack, and by the time linux starts using
them it also supports the new commands, then practically
devices do not care and can just mask the feature bit in MMIO.


> > If they want to cut at 1.3 time, they can, but they also can cut it at 1.2 
> > time, this
> > means some features will not be accessible through MMIO only through the
> > new commands.
> >
> New commands services new fields by newer driver.
> Newer driver can call new command to get new and old fields both.

yes.

> So 1.3 and 1.4 devices cannot optimize for older drivers.


I don't know what this means.

>  
> > > Once cfgvq is present for new fields, from the day 1, device does not 
> > > need to
> > store any newly defined fields on MMIO.
> > 
> > Exactly.
> > 
> > > And 10 to 20 years, it can stop for existing fields..
> > >
> > > The clear benefit is fields defined in 1.4 no longer needs to be stored 
> > > starting
> > from 2023-24.
> > 
> > And why is it a problem that someone can also build a 1.4 device with MMIO?
> >
> Why to build device using large MMIO when those fields are accessible via vq.

If you don't want to - don't. But it's simple and handy for software.
Not all devices are complex beasts like net.

Apropos, I am not yet sure the best way is simply adding admin vq to
vfs, in that this only works for fields not needed before DRIVER_OK.
Ideally I think we should fix all of config space, including device and
common config. I have some ideas but let's not get ahead of ourselves.

> VQ construct exists so it is already simple.
> 
> If the spec is defined in a way, that new fields must be accessible via vq, 
> and optionally via MMIO, than it is ok.
> One can choose to build via MMIO.
> So MMIO for new fields must not be mandatory.

For devices, yes. I am suggesting mandating it

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

2023-06-22 Thread Parav Pandit



> From: virtio-comm...@lists.oasis-open.org  open.org> On Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 22, 2023 2:40 PM
> To: Parav Pandit 
> Cc: Heng Qi ; virtio-comm...@lists.oasis-open.org;
> virtio-dev@lists.oasis-open.org; Jason Wang ; Yuri
> Benditovich ; Xuan Zhuo
> ; Cornelia Huck 
> Subject: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 
> v18]
> virtio-net: support inner header hash
> 
> On Thu, Jun 22, 2023 at 06:17:54PM +, Parav Pandit wrote:
> >
> >
> > > From: virtio-dev@lists.oasis-open.org
> > >  On Behalf Of Michael S. Tsirkin
> > > Sent: Thursday, June 22, 2023 2:12 PM
> >
> > [..]
> >
> > > > > > The proposal for 1.4 is literally very simple as below.
> > > > > > 1. All existing fields of cfg space stays in cfg space 2. Any
> > > > > > new capabilities to be queried, query using a vq (aq, cfgvq,
> whatevervq).
> > > > > > 3. Optionally existing fields can be queries over vq of #2
> > > > > > Once this arrive, no need for new GET commands.
> > > > > > Till that time, don't keep infinitely grow the cfg space.
> > > > > > Any next addition to cfg space, should work on defining the cfgvq.
> > > > >
> > > > > Simple, but short sighted. I know you guys don't support your
> > > > > hardware for 10-
> > > > > 20 years but for software people do.
> > > > > And so "All existing fields of cfg space stays in cfg space" is
> > > > > a bad idea simply because this does not allow removing things
> > > > > from config space not in 10 not in
> > > > > 20 years not ever.
> > > > >
> > > > #1 is for backward compat for existing drivers.
> > > >  You missed about #3. Existing cfg space fields can be queries
> > > > using the cfgvq
> > > too.
> > >
> > > Then #1 does not matter. We can give devices choice.
> > >
> > > > >
> > > > > Instead we need to allow two ways to access config space.  Teach
> > > > > drivers about both, actually mandate supporting both.  And then
> > > > > devices will make their own cost/benefit decision about which
> > > > > features they
> > > want to support in MMIO.
> > > >
> > > > If both method is mandated, I don't see benefit at all of two methods.
> > >
> > > Mandated for driver.
> > > Benefit is for devices, they will have the choice which drivers to
> > > support. In 10-
> > > 20 years all drivers support cfg command and then people can start
> > > shipping devices without MMIO access to any registers.
> > >
> > Until that point devices are forced to burn memory, which is not needed at 
> > all
> in < 3 years.
> 
> No they are not. They can make their own decision which fields to support in
> MMIO space.
Existing fields used by existing drivers must be in MMIO.
Device does not have a choice.

> If they want to cut at 1.3 time, they can, but they also can cut it at 1.2 
> time, this
> means some features will not be accessible through MMIO only through the
> new commands.
>
New commands services new fields by newer driver.
Newer driver can call new command to get new and old fields both.

So 1.3 and 1.4 devices cannot optimize for older drivers.

 
> > Once cfgvq is present for new fields, from the day 1, device does not need 
> > to
> store any newly defined fields on MMIO.
> 
> Exactly.
> 
> > And 10 to 20 years, it can stop for existing fields..
> >
> > The clear benefit is fields defined in 1.4 no longer needs to be stored 
> > starting
> from 2023-24.
> 
> And why is it a problem that someone can also build a 1.4 device with MMIO?
>
Why to build device using large MMIO when those fields are accessible via vq.
VQ construct exists so it is already simple.

If the spec is defined in a way, that new fields must be accessible via vq, and 
optionally via MMIO, than it is ok.
One can choose to build via MMIO.
So MMIO for new fields must not be mandatory.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 06:17:54PM +, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org  On
> > Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 2:12 PM
> 
> [..]
> 
> > > > > The proposal for 1.4 is literally very simple as below.
> > > > > 1. All existing fields of cfg space stays in cfg space 2. Any new
> > > > > capabilities to be queried, query using a vq (aq, cfgvq, whatevervq).
> > > > > 3. Optionally existing fields can be queries over vq of #2 Once
> > > > > this arrive, no need for new GET commands.
> > > > > Till that time, don't keep infinitely grow the cfg space.
> > > > > Any next addition to cfg space, should work on defining the cfgvq.
> > > >
> > > > Simple, but short sighted. I know you guys don't support your
> > > > hardware for 10-
> > > > 20 years but for software people do.
> > > > And so "All existing fields of cfg space stays in cfg space" is a
> > > > bad idea simply because this does not allow removing things from
> > > > config space not in 10 not in
> > > > 20 years not ever.
> > > >
> > > #1 is for backward compat for existing drivers.
> > >  You missed about #3. Existing cfg space fields can be queries using the 
> > > cfgvq
> > too.
> > 
> > Then #1 does not matter. We can give devices choice.
> > 
> > > >
> > > > Instead we need to allow two ways to access config space.  Teach
> > > > drivers about both, actually mandate supporting both.  And then
> > > > devices will make their own cost/benefit decision about which features 
> > > > they
> > want to support in MMIO.
> > >
> > > If both method is mandated, I don't see benefit at all of two methods.
> > 
> > Mandated for driver.
> > Benefit is for devices, they will have the choice which drivers to support. 
> > In 10-
> > 20 years all drivers support cfg command and then people can start shipping
> > devices without MMIO access to any registers.
> >
> Until that point devices are forced to burn memory, which is not needed at 
> all in < 3 years.

No they are not. They can make their own decision which fields to support in 
MMIO space.
If they want to cut at 1.3 time, they can, but they also can cut it
at 1.2 time, this means some features will not be accessible through
MMIO only through the new commands.

> Once cfgvq is present for new fields, from the day 1, device does not need to 
> store any newly defined fields on MMIO.

Exactly.

> And 10 to 20 years, it can stop for existing fields..
> 
> The clear benefit is fields defined in 1.4 no longer needs to be stored 
> starting from 2023-24.

And why is it a problem that someone can also build a 1.4 device with MMIO?

> > > VQ is generic part of the spec for slow and fast operation, so it is not 
> > > at all a
> > cost for config reading.
> > 
> > This will depend. E.g. if there's a single command to get all of config in 
> > one go,
> > then it actually can be a speedup, reducing the # of VM exits.
> 
> Yes, one command to get all at least device specific config.


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 06:12:09PM +, Parav Pandit wrote:
> > From: Michael S. Tsirkin 
> > Sent: Thursday, June 22, 2023 1:44 PM
> > 
> > On Thu, Jun 22, 2023 at 05:20:23PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Thursday, June 22, 2023 1:15 PM
> > > >
> > > > On Thu, Jun 22, 2023 at 05:04:04PM +, Parav Pandit wrote:
> > > > >
> > > > >
> > > > > > From: virtio-dev@lists.oasis-open.org
> > > > > >  On Behalf Of Michael S.
> > > > > > Tsirkin
> > > > > > Sent: Thursday, June 22, 2023 12:54 PM
> > > > >
> > > > > > > Admin command as I recall are not accessible directly by the
> > > > > > > member driver to
> > > > > > the member device.
> > > > > > > So a cmdq or cfgq is needed.
> > > > > >
> > > > > > Possible, sure. Or we actually discussed a self group. I took it
> > > > > > away until it had a user.
> > > > > >
> > > > > The problematic part of AQ is that its index is placed in the yet
> > > > > another onchip
> > > > die register that does not scale as each member device has different
> > > > queue count.
> > > > > When admin queue was discussed, it was only for group owner, (you
> > > > answered to Jiri).
> > > > > Hence the scale is relatively less, so it was acceptable.
> > > > >
> > > > > Now having unique numbers for VFs is not good.
> > > > > Max proposal was the last index after existing defined VQs of
> > > > > num_queues,
> > > > that saves the storage space on device.
> > > >
> > > > Surely, you can just have a very large index and be done with it?
> > > >
> > > There is count of AQ too.
> > 
> > Make that same across VFs?
> >
> Queues are not infinite, so when one doesn't need it, better to not make same 
> number.

I don't get it. Can you show an example configuration where there's
a problem? Which device type, # of data queues and admin queues desired and etc 
etc.

> > > For receive flow filters one may want to have multiple flowfilter_vqs as 
> > > the
> > perf req is high for some vms.
> > >
> > > And device to build non linear PCI steering on the driver notification 
> > > for this
> > very high q count.
> > > It is optimal to have finite and linear q max value.
> > 
> > What does this have to do with AQ? These are data vqs.
> > 
> 
> I think further. Ignore this comment.
> We need few bare minimum fields to bootstrap the device.
> So num_aq is one of them to absorb.
> This is ok.
> 
> > So 1.4 will maybe have new migration capabilities, and that is great.
> > But I do not like it that we are adding in 1.3 features that can't be 
> > supported
> > with current migration capabilities.
> For 1.3 vdpa style solution are anyway trapping the CVQ so no problem for it 
> either.

OK I get the idea. True.
Trapping is not the same as driving or actually emulating though.
That part would be new. Certainly annoying.
Practical? Needs a bit of thought.


What's even more annoying is that provisioning would suffer too, instead
of treating all fields the same this one will have to be treated
differently.

Can't say something can't be done, but it's unfortunate that
we are adding to technical debt.

It's late here, I think I will sleep on this.


-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: virtio-dev@lists.oasis-open.org  On
> Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 22, 2023 2:12 PM

[..]

> > > > The proposal for 1.4 is literally very simple as below.
> > > > 1. All existing fields of cfg space stays in cfg space 2. Any new
> > > > capabilities to be queried, query using a vq (aq, cfgvq, whatevervq).
> > > > 3. Optionally existing fields can be queries over vq of #2 Once
> > > > this arrive, no need for new GET commands.
> > > > Till that time, don't keep infinitely grow the cfg space.
> > > > Any next addition to cfg space, should work on defining the cfgvq.
> > >
> > > Simple, but short sighted. I know you guys don't support your
> > > hardware for 10-
> > > 20 years but for software people do.
> > > And so "All existing fields of cfg space stays in cfg space" is a
> > > bad idea simply because this does not allow removing things from
> > > config space not in 10 not in
> > > 20 years not ever.
> > >
> > #1 is for backward compat for existing drivers.
> >  You missed about #3. Existing cfg space fields can be queries using the 
> > cfgvq
> too.
> 
> Then #1 does not matter. We can give devices choice.
> 
> > >
> > > Instead we need to allow two ways to access config space.  Teach
> > > drivers about both, actually mandate supporting both.  And then
> > > devices will make their own cost/benefit decision about which features 
> > > they
> want to support in MMIO.
> >
> > If both method is mandated, I don't see benefit at all of two methods.
> 
> Mandated for driver.
> Benefit is for devices, they will have the choice which drivers to support. 
> In 10-
> 20 years all drivers support cfg command and then people can start shipping
> devices without MMIO access to any registers.
>
Until that point devices are forced to burn memory, which is not needed at all 
in < 3 years.
Once cfgvq is present for new fields, from the day 1, device does not need to 
store any newly defined fields on MMIO.

And 10 to 20 years, it can stop for existing fields..

The clear benefit is fields defined in 1.4 no longer needs to be stored 
starting from 2023-24.
 
> > VQ is generic part of the spec for slow and fast operation, so it is not at 
> > all a
> cost for config reading.
> 
> This will depend. E.g. if there's a single command to get all of config in 
> one go,
> then it actually can be a speedup, reducing the # of VM exits.

Yes, one command to get all at least device specific config.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit
> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 1:44 PM
> 
> On Thu, Jun 22, 2023 at 05:20:23PM +, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin 
> > > Sent: Thursday, June 22, 2023 1:15 PM
> > >
> > > On Thu, Jun 22, 2023 at 05:04:04PM +, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: virtio-dev@lists.oasis-open.org
> > > > >  On Behalf Of Michael S.
> > > > > Tsirkin
> > > > > Sent: Thursday, June 22, 2023 12:54 PM
> > > >
> > > > > > Admin command as I recall are not accessible directly by the
> > > > > > member driver to
> > > > > the member device.
> > > > > > So a cmdq or cfgq is needed.
> > > > >
> > > > > Possible, sure. Or we actually discussed a self group. I took it
> > > > > away until it had a user.
> > > > >
> > > > The problematic part of AQ is that its index is placed in the yet
> > > > another onchip
> > > die register that does not scale as each member device has different
> > > queue count.
> > > > When admin queue was discussed, it was only for group owner, (you
> > > answered to Jiri).
> > > > Hence the scale is relatively less, so it was acceptable.
> > > >
> > > > Now having unique numbers for VFs is not good.
> > > > Max proposal was the last index after existing defined VQs of
> > > > num_queues,
> > > that saves the storage space on device.
> > >
> > > Surely, you can just have a very large index and be done with it?
> > >
> > There is count of AQ too.
> 
> Make that same across VFs?
>
Queues are not infinite, so when one doesn't need it, better to not make same 
number.
 
> > For receive flow filters one may want to have multiple flowfilter_vqs as the
> perf req is high for some vms.
> >
> > And device to build non linear PCI steering on the driver notification for 
> > this
> very high q count.
> > It is optimal to have finite and linear q max value.
> 
> What does this have to do with AQ? These are data vqs.
> 

I think further. Ignore this comment.
We need few bare minimum fields to bootstrap the device.
So num_aq is one of them to absorb.
This is ok.

> So 1.4 will maybe have new migration capabilities, and that is great.
> But I do not like it that we are adding in 1.3 features that can't be 
> supported
> with current migration capabilities.
For 1.3 vdpa style solution are anyway trapping the CVQ so no problem for it 
either.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 05:51:41PM +, Parav Pandit wrote:
> 
> > From: virtio-dev@lists.oasis-open.org  On
> > Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 1:38 PM
> > 
> > On Thu, Jun 22, 2023 at 05:15:50PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Thursday, June 22, 2023 1:11 PM
> > >
> > > > > Provisioning driver usually do not attach to the member device 
> > > > > directly.
> > > > > This requires device reset, followed by reaching _DRIVER stage,
> > > > > querying
> > > > features etc and config area.
> > > > > And unbinding it and second reset by member driver. Ugh.
> > > > > Provisioning driver also needs to get the state or capabilities
> > > > > even when
> > > > member driver is already attached.
> > > > > So config space is not much a gain either.
> > > >
> > > > Actually it's RO so you *can* read it without any issues:
> > >
> > > It is RO but not same across all devices.
> > 
> > If you provision VFs differently. I got it.
> > 
> > > > - block guest access to status
> > > > - check DRIVER.
> > > > If set:
> > > > - read features, config
> > > > If not set:
> > > > - read features, config
> > > > - reset
> > > >
> > > This is what I explained.
> > > It is more messy if you equate to GET command has mess.
> > 
> > At least it works.
> >
> And new GET command also works when CVQ is trapped.

Not just trapped. You need to issue these commands.
That will require
- driving cvq at guest boot, sending commands, then reset
- poking at guest memory to change CVQ contents on
  the fly to mask commands.

How bad or hard is that? I'll need to ponder this a bit.


> And also works when AQ is querying capabilities of a member device using WIP 
> cmd.

yes that's the way forward in 1.4.

> > > > I am not saying it is elegant but then all of vdpa pile of hacks is not 
> > > > elegant.
> > > >
> > > I don't want to comment for vdpa. But it is not part of the spec...
> > 
> > Neither is QEMU.  It's one of spec implementations. Yes, we care about not
> > adding blockers for features that, superficially, might make sense for them.
> > 
> > > > And I am all for building something better but we didn't build it yet.
> > >
> > > The proposal for 1.4 is literally very simple as below.
> > > 1. All existing fields of cfg space stays in cfg space 2. Any new
> > > capabilities to be queried, query using a vq (aq, cfgvq, whatevervq).
> > > 3. Optionally existing fields can be queries over vq of #2 Once this
> > > arrive, no need for new GET commands.
> > > Till that time, don't keep infinitely grow the cfg space.
> > > Any next addition to cfg space, should work on defining the cfgvq.
> > 
> > Simple, but short sighted. I know you guys don't support your hardware for 
> > 10-
> > 20 years but for software people do.
> > And so "All existing fields of cfg space stays in cfg space" is a bad idea 
> > simply
> > because this does not allow removing things from config space not in 10 not 
> > in
> > 20 years not ever.
> >
> #1 is for backward compat for existing drivers.
>  You missed about #3. Existing cfg space fields can be queries using the 
> cfgvq too.

Then #1 does not matter. We can give devices choice.

> > 
> > Instead we need to allow two ways to access config space.  Teach drivers 
> > about
> > both, actually mandate supporting both.  And then devices will make their 
> > own
> > cost/benefit decision about which features they want to support in MMIO.
> 
> If both method is mandated, I don't see benefit at all of two methods.

Mandated for driver.
Benefit is for devices, they will have the choice which drivers to
support. In 10-20 years all drivers support cfg command and then
people can start shipping devices without MMIO access to any
registers.

> VQ is generic part of the spec for slow and fast operation, so it is not at 
> all a cost for config reading.

This will depend. E.g. if there's a single command to get all of config
in one go, then it actually can be a speedup, reducing the # of VM
exits.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 1:28 PM

> > > 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. 
Only for vdpa type of solution.

For passthrough device no need to trap any parameters or capability. 

> 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.
> 
Group owner device gets to query the capabilities (and provision) using admin 
command.
(already part of transport vq proposal).

> > > > > 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?
>
By issuing admin command on the group owner device.

> > > > > 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.
> 
I didn't follow this suggestion.

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

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit


> From: virtio-dev@lists.oasis-open.org  On
> Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 22, 2023 1:38 PM
> 
> On Thu, Jun 22, 2023 at 05:15:50PM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin 
> > > Sent: Thursday, June 22, 2023 1:11 PM
> >
> > > > Provisioning driver usually do not attach to the member device directly.
> > > > This requires device reset, followed by reaching _DRIVER stage,
> > > > querying
> > > features etc and config area.
> > > > And unbinding it and second reset by member driver. Ugh.
> > > > Provisioning driver also needs to get the state or capabilities
> > > > even when
> > > member driver is already attached.
> > > > So config space is not much a gain either.
> > >
> > > Actually it's RO so you *can* read it without any issues:
> >
> > It is RO but not same across all devices.
> 
> If you provision VFs differently. I got it.
> 
> > > - block guest access to status
> > > - check DRIVER.
> > > If set:
> > >   - read features, config
> > > If not set:
> > >   - read features, config
> > >   - reset
> > >
> > This is what I explained.
> > It is more messy if you equate to GET command has mess.
> 
> At least it works.
>
And new GET command also works when CVQ is trapped.
And also works when AQ is querying capabilities of a member device using WIP 
cmd.
 
> > > I am not saying it is elegant but then all of vdpa pile of hacks is not 
> > > elegant.
> > >
> > I don't want to comment for vdpa. But it is not part of the spec...
> 
> Neither is QEMU.  It's one of spec implementations. Yes, we care about not
> adding blockers for features that, superficially, might make sense for them.
> 
> > > And I am all for building something better but we didn't build it yet.
> >
> > The proposal for 1.4 is literally very simple as below.
> > 1. All existing fields of cfg space stays in cfg space 2. Any new
> > capabilities to be queried, query using a vq (aq, cfgvq, whatevervq).
> > 3. Optionally existing fields can be queries over vq of #2 Once this
> > arrive, no need for new GET commands.
> > Till that time, don't keep infinitely grow the cfg space.
> > Any next addition to cfg space, should work on defining the cfgvq.
> 
> Simple, but short sighted. I know you guys don't support your hardware for 10-
> 20 years but for software people do.
> And so "All existing fields of cfg space stays in cfg space" is a bad idea 
> simply
> because this does not allow removing things from config space not in 10 not in
> 20 years not ever.
>
#1 is for backward compat for existing drivers.
 You missed about #3. Existing cfg space fields can be queries using the cfgvq 
too.

> 
> Instead we need to allow two ways to access config space.  Teach drivers about
> both, actually mandate supporting both.  And then devices will make their own
> cost/benefit decision about which features they want to support in MMIO.

If both method is mandated, I don't see benefit at all of two methods.
VQ is generic part of the spec for slow and fast operation, so it is not at all 
a cost for config reading.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 05:20:23PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Thursday, June 22, 2023 1:15 PM
> > 
> > On Thu, Jun 22, 2023 at 05:04:04PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: virtio-dev@lists.oasis-open.org
> > > >  On Behalf Of Michael S. Tsirkin
> > > > Sent: Thursday, June 22, 2023 12:54 PM
> > >
> > > > > Admin command as I recall are not accessible directly by the
> > > > > member driver to
> > > > the member device.
> > > > > So a cmdq or cfgq is needed.
> > > >
> > > > Possible, sure. Or we actually discussed a self group. I took it
> > > > away until it had a user.
> > > >
> > > The problematic part of AQ is that its index is placed in the yet another 
> > > onchip
> > die register that does not scale as each member device has different queue
> > count.
> > > When admin queue was discussed, it was only for group owner, (you
> > answered to Jiri).
> > > Hence the scale is relatively less, so it was acceptable.
> > >
> > > Now having unique numbers for VFs is not good.
> > > Max proposal was the last index after existing defined VQs of num_queues,
> > that saves the storage space on device.
> > 
> > Surely, you can just have a very large index and be done with it?
> >
> There is count of AQ too.

Make that same across VFs?

> For receive flow filters one may want to have multiple flowfilter_vqs as the 
> perf req is high for some vms.
> 
> And device to build non linear PCI steering on the driver notification for 
> this very high q count.
> It is optimal to have finite and linear q max value.

What does this have to do with AQ? These are data vqs.


> > > > > The single way for every device to query their capabilities is via
> > > > > a cfgvq for all
> > > > new fields without extending the existing config space.
> > > > > (and optionally old fields).
> > > >
> > > > Or adminq with self group. I like this somewhat better because we
> > > > need exactly same query from owner.
> > > >
> > > Yes. this is why I proposed to name is cmdvq that can carry admin commands
> > or other.
> > > But fine, we had to progress for group owner.
> > >
> > > > > > Why don't we focus on a work on a full solution? Just don't
> > > > > > implement this thing in your devices meanwhile until we do.
> > > > > >
> > > > > Then Heng needs to wait for cfgvq to be defined to be implemented 
> > > > > first.
> > > > > Doesn't look reasonable to me.
> > > >
> > > > And *everything* has to wait. No, not reasonable. We somehow managed
> > > > to release several spec versions and things did not ground to a halt 
> > > > without
> > cfgvq.
> > > > Don't see a reason to do it right now, what's special about now? I
> > > > feel we should add to config space and then solve it all.
> > > >
> > > Things didn't ground at cost of device keep increasing their memory 
> > > footprint.
> > > The latest addition I remember is the queue_reset register.
> > > It was bit but a purely control operation that got in there.
> > >
> > > > > Current GET is coherent with the new commands defined such as
> > > > > notification
> > > > coalescing.
> > > > >
> > > > > As community, we should work on defining the cfgvq, till that time
> > > > > have the
> > > > optimal way to get the config, i.e. using the cvq.
> > > >
> > > > cvq doesn't really work for capabilities though.
> > >
> > > For the device itself, it does which is what is being done here.
> > 
> > Yes but not for migration.
> 
> For migration a admin command to query capabilities is needed.
> This is present in the transport vq proposal already to be rebased on top of 
> admin cmd.

So 1.4 will maybe have new migration capabilities, and that is great.
But I do not like it that we are adding in 1.3 features that
can't be supported with current migration capabilities.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 05:15:50PM +, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin 
> > Sent: Thursday, June 22, 2023 1:11 PM
> 
> > > Provisioning driver usually do not attach to the member device directly.
> > > This requires device reset, followed by reaching _DRIVER stage, querying
> > features etc and config area.
> > > And unbinding it and second reset by member driver. Ugh.
> > > Provisioning driver also needs to get the state or capabilities even when
> > member driver is already attached.
> > > So config space is not much a gain either.
> > 
> > Actually it's RO so you *can* read it without any issues:
> 
> It is RO but not same across all devices.

If you provision VFs differently. I got it.

> > - block guest access to status
> > - check DRIVER.
> > If set:
> > - read features, config
> > If not set:
> > - read features, config
> > - reset
> > 
> This is what I explained.
> It is more messy if you equate to GET command has mess.

At least it works.

> > I am not saying it is elegant but then all of vdpa pile of hacks is not 
> > elegant.
> > 
> I don't want to comment for vdpa. But it is not part of the spec...

Neither is QEMU.  It's one of spec implementations. Yes, we care about
not adding blockers for features that, superficially, might make
sense for them.

> > And I am all for building something better but we didn't build it yet.
> 
> The proposal for 1.4 is literally very simple as below.
> 1. All existing fields of cfg space stays in cfg space
> 2. Any new capabilities to be queried, query using a vq (aq, cfgvq, 
> whatevervq).
> 3. Optionally existing fields can be queries over vq of #2
> Once this arrive, no need for new GET commands.
> Till that time, don't keep infinitely grow the cfg space.
> Any next addition to cfg space, should work on defining the cfgvq.

Simple, but short sighted. I know you guys don't support your
hardware for 10-20 years but for software people do.
And so "All existing fields of cfg space stays in cfg space" is a bad
idea simply because this does not allow removing things from config
space not in 10 not in 20 years not ever.


Instead we need to allow two ways to access config space.  Teach drivers
about both, actually mandate supporting both.  And then devices will
make their own cost/benefit decision about which features they want to
support in MMIO.


-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 05:11:24PM +, Parav Pandit wrote:
> 
> 
> > From: virtio-comm...@lists.oasis-open.org  > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 1:04 PM
> > To: Parav Pandit 
> > 
> > On Thu, Jun 22, 2023 at 04:54:48PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin 
> > > > 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


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit


> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 1:15 PM
> 
> On Thu, Jun 22, 2023 at 05:04:04PM +, Parav Pandit wrote:
> >
> >
> > > From: virtio-dev@lists.oasis-open.org
> > >  On Behalf Of Michael S. Tsirkin
> > > Sent: Thursday, June 22, 2023 12:54 PM
> >
> > > > Admin command as I recall are not accessible directly by the
> > > > member driver to
> > > the member device.
> > > > So a cmdq or cfgq is needed.
> > >
> > > Possible, sure. Or we actually discussed a self group. I took it
> > > away until it had a user.
> > >
> > The problematic part of AQ is that its index is placed in the yet another 
> > onchip
> die register that does not scale as each member device has different queue
> count.
> > When admin queue was discussed, it was only for group owner, (you
> answered to Jiri).
> > Hence the scale is relatively less, so it was acceptable.
> >
> > Now having unique numbers for VFs is not good.
> > Max proposal was the last index after existing defined VQs of num_queues,
> that saves the storage space on device.
> 
> Surely, you can just have a very large index and be done with it?
>
There is count of AQ too.
For receive flow filters one may want to have multiple flowfilter_vqs as the 
perf req is high for some vms.

And device to build non linear PCI steering on the driver notification for this 
very high q count.
It is optimal to have finite and linear q max value.

> > > > The single way for every device to query their capabilities is via
> > > > a cfgvq for all
> > > new fields without extending the existing config space.
> > > > (and optionally old fields).
> > >
> > > Or adminq with self group. I like this somewhat better because we
> > > need exactly same query from owner.
> > >
> > Yes. this is why I proposed to name is cmdvq that can carry admin commands
> or other.
> > But fine, we had to progress for group owner.
> >
> > > > > Why don't we focus on a work on a full solution? Just don't
> > > > > implement this thing in your devices meanwhile until we do.
> > > > >
> > > > Then Heng needs to wait for cfgvq to be defined to be implemented first.
> > > > Doesn't look reasonable to me.
> > >
> > > And *everything* has to wait. No, not reasonable. We somehow managed
> > > to release several spec versions and things did not ground to a halt 
> > > without
> cfgvq.
> > > Don't see a reason to do it right now, what's special about now? I
> > > feel we should add to config space and then solve it all.
> > >
> > Things didn't ground at cost of device keep increasing their memory 
> > footprint.
> > The latest addition I remember is the queue_reset register.
> > It was bit but a purely control operation that got in there.
> >
> > > > Current GET is coherent with the new commands defined such as
> > > > notification
> > > coalescing.
> > > >
> > > > As community, we should work on defining the cfgvq, till that time
> > > > have the
> > > optimal way to get the config, i.e. using the cvq.
> > >
> > > cvq doesn't really work for capabilities though.
> >
> > For the device itself, it does which is what is being done here.
> 
> Yes but not for migration.

For migration a admin command to query capabilities is needed.
This is present in the transport vq proposal already to be rebased on top of 
admin cmd.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 1:11 PM

> > Provisioning driver usually do not attach to the member device directly.
> > This requires device reset, followed by reaching _DRIVER stage, querying
> features etc and config area.
> > And unbinding it and second reset by member driver. Ugh.
> > Provisioning driver also needs to get the state or capabilities even when
> member driver is already attached.
> > So config space is not much a gain either.
> 
> Actually it's RO so you *can* read it without any issues:

It is RO but not same across all devices.

> - block guest access to status
> - check DRIVER.
> If set:
>   - read features, config
> If not set:
>   - read features, config
>   - reset
> 
This is what I explained.
It is more messy if you equate to GET command has mess.
> I am not saying it is elegant but then all of vdpa pile of hacks is not 
> elegant.
> 
I don't want to comment for vdpa. But it is not part of the spec...

> And I am all for building something better but we didn't build it yet.

The proposal for 1.4 is literally very simple as below.
1. All existing fields of cfg space stays in cfg space
2. Any new capabilities to be queried, query using a vq (aq, cfgvq, whatevervq).
3. Optionally existing fields can be queries over vq of #2

Once this arrive, no need for new GET commands.
Till that time, don't keep infinitely grow the cfg space.
Any next addition to cfg space, should work on defining the cfgvq.


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 05:04:04PM +, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org  On
> > Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 22, 2023 12:54 PM
> 
> > > Admin command as I recall are not accessible directly by the member 
> > > driver to
> > the member device.
> > > So a cmdq or cfgq is needed.
> > 
> > Possible, sure. Or we actually discussed a self group. I took it away until 
> > it had a
> > user.
> >
> The problematic part of AQ is that its index is placed in the yet another 
> onchip die register that does not scale as each member device has different 
> queue count.
> When admin queue was discussed, it was only for group owner, (you answered to 
> Jiri).
> Hence the scale is relatively less, so it was acceptable.
> 
> Now having unique numbers for VFs is not good.
> Max proposal was the last index after existing defined VQs of num_queues, 
> that saves the storage space on device.

Surely, you can just have a very large index and be done with it?

> > > The single way for every device to query their capabilities is via a 
> > > cfgvq for all
> > new fields without extending the existing config space.
> > > (and optionally old fields).
> > 
> > Or adminq with self group. I like this somewhat better because we need 
> > exactly
> > same query from owner.
> >
> Yes. this is why I proposed to name is cmdvq that can carry admin commands or 
> other.
> But fine, we had to progress for group owner.
>  
> > > > Why don't we focus on a work on a full solution? Just don't
> > > > implement this thing in your devices meanwhile until we do.
> > > >
> > > Then Heng needs to wait for cfgvq to be defined to be implemented first.
> > > Doesn't look reasonable to me.
> > 
> > And *everything* has to wait. No, not reasonable. We somehow managed to
> > release several spec versions and things did not ground to a halt without 
> > cfgvq.
> > Don't see a reason to do it right now, what's special about now? I feel we 
> > should
> > add to config space and then solve it all.
> >
> Things didn't ground at cost of device keep increasing their memory footprint.
> The latest addition I remember is the queue_reset register.
> It was bit but a purely control operation that got in there.
>  
> > > Current GET is coherent with the new commands defined such as notification
> > coalescing.
> > >
> > > As community, we should work on defining the cfgvq, till that time have 
> > > the
> > optimal way to get the config, i.e. using the cvq.
> > 
> > cvq doesn't really work for capabilities though.
> 
> For the device itself, it does which is what is being done here.

Yes but not for migration.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: virtio-comm...@lists.oasis-open.org  open.org> On Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 22, 2023 1:04 PM
> To: Parav Pandit 
> 
> On Thu, Jun 22, 2023 at 04:54:48PM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin 
> > > 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.

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

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

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

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 12:32:33PM +, Parav Pandit wrote:
> Provisioning driver usually do not attach to the member device directly.
> This requires device reset, followed by reaching _DRIVER stage, querying 
> features etc and config area.
> And unbinding it and second reset by member driver. Ugh.
> Provisioning driver also needs to get the state or capabilities even when 
> member driver is already attached.
> So config space is not much a gain either.

Actually it's RO so you *can* read it without any issues:
- block guest access to status
- check DRIVER.
If set:
- read features, config
If not set:
- read features, config
- reset

I am not saying it is elegant but then all of vdpa pile of
hacks is not elegant.

And I am all for building something better but we didn't
build it yet.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: virtio-dev@lists.oasis-open.org  On
> Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 22, 2023 12:54 PM

> > Admin command as I recall are not accessible directly by the member driver 
> > to
> the member device.
> > So a cmdq or cfgq is needed.
> 
> Possible, sure. Or we actually discussed a self group. I took it away until 
> it had a
> user.
>
The problematic part of AQ is that its index is placed in the yet another 
onchip die register that does not scale as each member device has different 
queue count.

When admin queue was discussed, it was only for group owner, (you answered to 
Jiri).
Hence the scale is relatively less, so it was acceptable.

Now having unique numbers for VFs is not good.
Max proposal was the last index after existing defined VQs of num_queues, that 
saves the storage space on device.

> > The single way for every device to query their capabilities is via a cfgvq 
> > for all
> new fields without extending the existing config space.
> > (and optionally old fields).
> 
> Or adminq with self group. I like this somewhat better because we need exactly
> same query from owner.
>
Yes. this is why I proposed to name is cmdvq that can carry admin commands or 
other.
But fine, we had to progress for group owner.
 
> > > Why don't we focus on a work on a full solution? Just don't
> > > implement this thing in your devices meanwhile until we do.
> > >
> > Then Heng needs to wait for cfgvq to be defined to be implemented first.
> > Doesn't look reasonable to me.
> 
> And *everything* has to wait. No, not reasonable. We somehow managed to
> release several spec versions and things did not ground to a halt without 
> cfgvq.
> Don't see a reason to do it right now, what's special about now? I feel we 
> should
> add to config space and then solve it all.
>
Things didn't ground at cost of device keep increasing their memory footprint.
The latest addition I remember is the queue_reset register.
It was bit but a purely control operation that got in there.
 
> > Current GET is coherent with the new commands defined such as notification
> coalescing.
> >
> > As community, we should work on defining the cfgvq, till that time have the
> optimal way to get the config, i.e. using the cvq.
> 
> cvq doesn't really work for capabilities though.

For the device itself, it does which is what is being done here.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 04:54:48PM +, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin 
> > 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.

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

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

A good implementation of virtio_cread can abstract that easily so we
don't need to change drivers.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit


> From: Michael S. Tsirkin 
> 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.
 
> 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.

> So what is the solution proposed for this?
> 
1. Query member device capabilities via admin command

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


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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 04:42:40PM +, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin 
> > Sent: Thursday, June 22, 2023 12:28 PM
> 
> > > > Not sure what's the implication?
> > > Implication is device needs to store this in always available on-chip 
> > > memory
> > which is not good.
> > 
> > Oh by devices you mean VFs. Now I get your motivation, at least. Thanks.
> >
> > > >
> > > > > > For example, for migration driver might want to validate that two
> > > > > > devices have same capability. doing it without dma is nicer.
> > > > > >
> > > > > A migration driver for real world scenario, will almost have to use 
> > > > > the dma
> > for
> > > > amount of data it needs to exchange.
> > > >
> > > > Not migration itself, provisioning.
> > > >
> > > Provisioning driver usually do not attach to the member device directly.
> > > This requires device reset, followed by reaching _DRIVER stage, querying
> > features etc and config area.
> > > And unbinding it and second reset by member driver. Ugh.
> > > Provisioning driver also needs to get the state or capabilities even when
> > member driver is already attached.
> > > So config space is not much a gain either.
> > >
> > 
> > Absolutely, that's why we have admin commands.  I was hoping for an
> > admin command that basically gets/sets RO fields of the config space.
> >
> Admin command as I recall are not accessible directly by the member driver to 
> the member device.
> So a cmdq or cfgq is needed.

Possible, sure. Or we actually discussed a self group. I took it away until it 
had a user.


> > > Instead of decision point being RO vs RW,
> > > any new fields via cmdvq and existing fields stays in cfg space, give 
> > > predictable
> > behavior to size the member devices in the system.
> > > Once the cmdvq is available, we can get rid of GET command used in this
> > version for new future features.
> > > Till that arrives, GET command is the efficient way.
> > 
> > I understand.  I just don't much like these patchwork solutions though.
> > And I don't like that we will pay by not having a single conherent
> > way to provision and query capabilities through config space,
> > instead just for this thing we will have a special thing.
> >
> The single way for every device to query their capabilities is via a cfgvq 
> for all new fields without extending the existing config space.
> (and optionally old fields).

Or adminq with self group. I like this somewhat better because we need
exactly same query from owner.

> > Why don't we focus on a work on a full solution? Just don't implement
> > this thing in your devices meanwhile until we do.
> > 
> Then Heng needs to wait for cfgvq to be defined to be implemented first.
> Doesn't look reasonable to me.

And *everything* has to wait. No, not reasonable. We somehow managed
to release several spec versions and things did not ground to
a halt without cfgvq. Don't see a reason to do it right now,
what's special about now? I feel we should add to config space
and then solve it all.

> Current GET is coherent with the new commands defined such as notification 
> coalescing.
> 
> As community, we should work on defining the cfgvq, till that time have the 
> optimal way to get the config, i.e. using the cvq.

cvq doesn't really work for capabilities though.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 02:27:31PM +, Parav Pandit wrote:
> 
> 
> > From: Heng Qi 
> > Sent: Thursday, June 22, 2023 9:43 AM
> > 
> > Yes, I think we also have to consider upcoming
> >      1. device counters (e.g. supported_device_counter),
> >      2. receive flow filters (e.g. supported_flow_types, 
> > supported_max_entries),
> >      3. header splits (e.g. supported_split_types) etc.
> > Continuous expansion of the configuration space needs to be careful.
> > 
> > >
> > > Instead of decision point being RO vs RW, any new fields via cmdvq and
> > > existing fields stays in cfg space, give predictable behavior to size the 
> > > member
> > devices in the system.
> > > Once the cmdvq is available, we can get rid of GET command used in this
> > version for new future features.
> > > Till that arrives, GET command is the efficient way.
> > 
> > Yes, I agree.
> > 
> > Thanks.
> 
> Right. So, Miachel main concern was two vs one struct.
> And given the GET and SET works on different fields, having two structure is 
> just fine.
> The code is fairly small.
> I don’t see any real issue here with v18.
> 

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.

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

But querying over cvq while VF is assigned clearly *doesn't* work.

So what is the solution proposed for this?

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.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 12:28 PM

> > > Not sure what's the implication?
> > Implication is device needs to store this in always available on-chip memory
> which is not good.
> 
> Oh by devices you mean VFs. Now I get your motivation, at least. Thanks.
>
> > >
> > > > > For example, for migration driver might want to validate that two
> > > > > devices have same capability. doing it without dma is nicer.
> > > > >
> > > > A migration driver for real world scenario, will almost have to use the 
> > > > dma
> for
> > > amount of data it needs to exchange.
> > >
> > > Not migration itself, provisioning.
> > >
> > Provisioning driver usually do not attach to the member device directly.
> > This requires device reset, followed by reaching _DRIVER stage, querying
> features etc and config area.
> > And unbinding it and second reset by member driver. Ugh.
> > Provisioning driver also needs to get the state or capabilities even when
> member driver is already attached.
> > So config space is not much a gain either.
> >
> 
> Absolutely, that's why we have admin commands.  I was hoping for an
> admin command that basically gets/sets RO fields of the config space.
>
Admin command as I recall are not accessible directly by the member driver to 
the member device.
So a cmdq or cfgq is needed.
 
> > Instead of decision point being RO vs RW,
> > any new fields via cmdvq and existing fields stays in cfg space, give 
> > predictable
> behavior to size the member devices in the system.
> > Once the cmdvq is available, we can get rid of GET command used in this
> version for new future features.
> > Till that arrives, GET command is the efficient way.
> 
> I understand.  I just don't much like these patchwork solutions though.
> And I don't like that we will pay by not having a single conherent
> way to provision and query capabilities through config space,
> instead just for this thing we will have a special thing.
>
The single way for every device to query their capabilities is via a cfgvq for 
all new fields without extending the existing config space.
(and optionally old fields).
 
> Why don't we focus on a work on a full solution? Just don't implement
> this thing in your devices meanwhile until we do.
> 
Then Heng needs to wait for cfgvq to be defined to be implemented first.
Doesn't look reasonable to me.
Current GET is coherent with the new commands defined such as notification 
coalescing.

As community, we should work on defining the cfgvq, till that time have the 
optimal way to get the config, i.e. using the cvq.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Michael S. Tsirkin
On Thu, Jun 22, 2023 at 12:32:33PM +, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin 
> > Sent: Thursday, June 22, 2023 2:23 AM
> > 
> > On Wed, Jun 21, 2023 at 08:52:04PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Wednesday, June 21, 2023 4:38 PM
> > >
> > > > > > And the field is RO so no memory cost to exposing it in all VFs.
> > > > > Two structures do not bring the asymmetry.
> > > > > Accessing current and enabled fields via two different mechanism
> > > > > is bringing
> > > > the asymmetry.
> > > >
> > > > I guess it's a matter of taste, but it is clearly more consistent
> > > > with other hash things, to which it's very similar.
> > > >
> > > This is consistent with new commands we define including notification
> > coalescing whose GET is not coming config space.
> > 
> > But there GET just reports the current state. Not the read only capability. 
> > So
> > there would be cost per VF to keep it in config space.
> > This one is RO no cost per VF. Let's make it convenient?
> >
> And each VF can have different value hence requires per VF storage in the 
> device.
>  
> > > >
> > > > Nah, config space is too convenient when we can live with its
> > > > limitations. I don't thin kwe prefer not to keep growing it.
> > > > For some things such as this one it's perfect.
> > > >
> > > Fields are different between different devices.
> > 
> > Not sure what's the implication?
> Implication is device needs to store this in always available on-chip memory 
> which is not good.

Oh by devices you mean VFs. Now I get your motivation, at least. Thanks.

> > 
> > > > For example, for migration driver might want to validate that two
> > > > devices have same capability. doing it without dma is nicer.
> > > >
> > > A migration driver for real world scenario, will almost have to use the 
> > > dma for
> > amount of data it needs to exchange.
> > 
> > Not migration itself, provisioning.
> >
> Provisioning driver usually do not attach to the member device directly.
> This requires device reset, followed by reaching _DRIVER stage, querying 
> features etc and config area.
> And unbinding it and second reset by member driver. Ugh.
> Provisioning driver also needs to get the state or capabilities even when 
> member driver is already attached.
> So config space is not much a gain either.
>

Absolutely, that's why we have admin commands.  I was hoping for an
admin command that basically gets/sets RO fields of the config space.

> > > > Another example, future admin transport will have ability to
> > > > provision devices by supplying their config space.
> > > > This will include this capability automatically, if instead we hide
> > > > it in a command we need to do extra custom work.
> > > >
> > > > > So we do not prefer to keep growing the config space anymore,
> > > > > hence GET is the right approach to me.
> > > >
> > > > Heh I know you hate config space. Let it go, stop wasting time
> > > > arguing about the same thing on every turn and instead help define
> > > > admin transport to solve it
> > >
> > > This was discussed many times, a driver to have a direct (non-intercepted 
> > > by
> > owner device) channel to device.
> > > If you mean this non-intercepted channel as admin transport, fine.
> > 
> > we can do that, sure.
> > 
> > > If you mean this is intercepted and it is going over admin cmd, then it 
> > > is of no
> > use for all future interfaces.
> > >
> > > We discussed this in thread with you and Jason.
> > > I provided concrete example with size and device provisioning math too and
> > other example of multi-physical address VQ.
> > > So transporting register by register over some admin transport is 
> > > sub-optimal.
> > >
> > 
> > Not register by register, we can send all of config space as long as it's 
> > RO. This
> > field is.
> >
> It is RO in context of one member device, but every VF can have different 
> value.
> The device will never know if one will use new cmdvq to access or some old 
> driver will use without it.
> And hence, it always needs to provision it on onchip memory for backward 
> compatibility.
> 
> Instead of decision point being RO vs RW, 
> any new fields via cmdvq and existing fields stays in cfg space, give 
> predictable behavior to size the member devices in the system.
> Once the cmdvq is available, we can get rid of GET command used in this 
> version for new future features.
> Till that arrives, GET command is the efficient way.

I understand.  I just don't much like these patchwork solutions though.
And I don't like that we will pay by not having a single conherent
way to provision and query capabilities through config space,
instead just for this thing we will have a special thing.

Why don't we focus on a work on a full solution? Just don't implement
this thing in your devices meanwhile until we do.

> > --
> > MST


-
To unsubscribe, e-mail: virtio-dev

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

2023-06-22 Thread Parav Pandit


> From: Heng Qi 
> Sent: Thursday, June 22, 2023 9:43 AM
> 
> Yes, I think we also have to consider upcoming
>      1. device counters (e.g. supported_device_counter),
>      2. receive flow filters (e.g. supported_flow_types, 
> supported_max_entries),
>      3. header splits (e.g. supported_split_types) etc.
> Continuous expansion of the configuration space needs to be careful.
> 
> >
> > Instead of decision point being RO vs RW, any new fields via cmdvq and
> > existing fields stays in cfg space, give predictable behavior to size the 
> > member
> devices in the system.
> > Once the cmdvq is available, we can get rid of GET command used in this
> version for new future features.
> > Till that arrives, GET command is the efficient way.
> 
> Yes, I agree.
> 
> Thanks.

Right. So, Miachel main concern was two vs one struct.
And given the GET and SET works on different fields, having two structure is 
just fine.
The code is fairly small.
I don’t see any real issue here with v18.



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

2023-06-22 Thread Heng Qi




在 2023/6/22 下午8:32, Parav Pandit 写道:



From: Michael S. Tsirkin 
Sent: Thursday, June 22, 2023 2:23 AM

On Wed, Jun 21, 2023 at 08:52:04PM +, Parav Pandit wrote:

From: Michael S. Tsirkin 
Sent: Wednesday, June 21, 2023 4:38 PM

And the field is RO so no memory cost to exposing it in all VFs.

Two structures do not bring the asymmetry.
Accessing current and enabled fields via two different mechanism
is bringing

the asymmetry.

I guess it's a matter of taste, but it is clearly more consistent
with other hash things, to which it's very similar.


This is consistent with new commands we define including notification

coalescing whose GET is not coming config space.

But there GET just reports the current state. Not the read only capability. So
there would be cost per VF to keep it in config space.
This one is RO no cost per VF. Let's make it convenient?


And each VF can have different value hence requires per VF storage in the 
device.
  

Nah, config space is too convenient when we can live with its
limitations. I don't thin kwe prefer not to keep growing it.
For some things such as this one it's perfect.


Fields are different between different devices.

Not sure what's the implication?

Implication is device needs to store this in always available on-chip memory 
which is not good.


For example, for migration driver might want to validate that two
devices have same capability. doing it without dma is nicer.


A migration driver for real world scenario, will almost have to use the dma for

amount of data it needs to exchange.

Not migration itself, provisioning.


Provisioning driver usually do not attach to the member device directly.
This requires device reset, followed by reaching _DRIVER stage, querying 
features etc and config area.
And unbinding it and second reset by member driver. Ugh.

Provisioning driver also needs to get the state or capabilities even when 
member driver is already attached.
So config space is not much a gain either.
  

Another example, future admin transport will have ability to
provision devices by supplying their config space.
This will include this capability automatically, if instead we hide
it in a command we need to do extra custom work.


So we do not prefer to keep growing the config space anymore,
hence GET is the right approach to me.

Heh I know you hate config space. Let it go, stop wasting time
arguing about the same thing on every turn and instead help define
admin transport to solve it

This was discussed many times, a driver to have a direct (non-intercepted by

owner device) channel to device.

If you mean this non-intercepted channel as admin transport, fine.

we can do that, sure.


If you mean this is intercepted and it is going over admin cmd, then it is of no

use for all future interfaces.

We discussed this in thread with you and Jason.
I provided concrete example with size and device provisioning math too and

other example of multi-physical address VQ.

So transporting register by register over some admin transport is sub-optimal.


Not register by register, we can send all of config space as long as it's RO. 
This
field is.


It is RO in context of one member device, but every VF can have different value.
The device will never know if one will use new cmdvq to access or some old 
driver will use without it.
And hence, it always needs to provision it on onchip memory for backward 
compatibility.


Yes, I think we also have to consider upcoming
    1. device counters (e.g. supported_device_counter),
    2. receive flow filters (e.g. supported_flow_types, 
supported_max_entries),

    3. header splits (e.g. supported_split_types) etc.
Continuous expansion of the configuration space needs to be careful.



Instead of decision point being RO vs RW,
any new fields via cmdvq and existing fields stays in cfg space, give 
predictable behavior to size the member devices in the system.
Once the cmdvq is available, we can get rid of GET command used in this version 
for new future features.
Till that arrives, GET command is the efficient way.


Yes, I agree.

Thanks.


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2023-06-22 Thread Parav Pandit



> From: Michael S. Tsirkin 
> Sent: Thursday, June 22, 2023 2:23 AM
> 
> On Wed, Jun 21, 2023 at 08:52:04PM +, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin 
> > > Sent: Wednesday, June 21, 2023 4:38 PM
> >
> > > > > And the field is RO so no memory cost to exposing it in all VFs.
> > > > Two structures do not bring the asymmetry.
> > > > Accessing current and enabled fields via two different mechanism
> > > > is bringing
> > > the asymmetry.
> > >
> > > I guess it's a matter of taste, but it is clearly more consistent
> > > with other hash things, to which it's very similar.
> > >
> > This is consistent with new commands we define including notification
> coalescing whose GET is not coming config space.
> 
> But there GET just reports the current state. Not the read only capability. So
> there would be cost per VF to keep it in config space.
> This one is RO no cost per VF. Let's make it convenient?
>
And each VF can have different value hence requires per VF storage in the 
device.
 
> > >
> > > Nah, config space is too convenient when we can live with its
> > > limitations. I don't thin kwe prefer not to keep growing it.
> > > For some things such as this one it's perfect.
> > >
> > Fields are different between different devices.
> 
> Not sure what's the implication?
Implication is device needs to store this in always available on-chip memory 
which is not good.

> 
> > > For example, for migration driver might want to validate that two
> > > devices have same capability. doing it without dma is nicer.
> > >
> > A migration driver for real world scenario, will almost have to use the dma 
> > for
> amount of data it needs to exchange.
> 
> Not migration itself, provisioning.
>
Provisioning driver usually do not attach to the member device directly.
This requires device reset, followed by reaching _DRIVER stage, querying 
features etc and config area.
And unbinding it and second reset by member driver. Ugh.

Provisioning driver also needs to get the state or capabilities even when 
member driver is already attached.
So config space is not much a gain either.
 
> > > Another example, future admin transport will have ability to
> > > provision devices by supplying their config space.
> > > This will include this capability automatically, if instead we hide
> > > it in a command we need to do extra custom work.
> > >
> > > > So we do not prefer to keep growing the config space anymore,
> > > > hence GET is the right approach to me.
> > >
> > > Heh I know you hate config space. Let it go, stop wasting time
> > > arguing about the same thing on every turn and instead help define
> > > admin transport to solve it
> >
> > This was discussed many times, a driver to have a direct (non-intercepted by
> owner device) channel to device.
> > If you mean this non-intercepted channel as admin transport, fine.
> 
> we can do that, sure.
> 
> > If you mean this is intercepted and it is going over admin cmd, then it is 
> > of no
> use for all future interfaces.
> >
> > We discussed this in thread with you and Jason.
> > I provided concrete example with size and device provisioning math too and
> other example of multi-physical address VQ.
> > So transporting register by register over some admin transport is 
> > sub-optimal.
> >
> 
> Not register by register, we can send all of config space as long as it's RO. 
> This
> field is.
>
It is RO in context of one member device, but every VF can have different value.
The device will never know if one will use new cmdvq to access or some old 
driver will use without it.
And hence, it always needs to provision it on onchip memory for backward 
compatibility.

Instead of decision point being RO vs RW, 
any new fields via cmdvq and existing fields stays in cfg space, give 
predictable behavior to size the member devices in the system.
Once the cmdvq is available, we can get rid of GET command used in this version 
for new future features.
Till that arrives, GET command is the efficient way.
 
> --
> MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org