[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Sat, Jul 01, 2023 at 12:09:09AM +0800, Heng Qi wrote:
> On Fri, Jun 30, 2023 at 10:52:25AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
> > >
> > >
> > > 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > > > >
> > > > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > > > From: Heng Qi
> > > > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > > > >
> > > > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > > > > > From: Michael S. Tsirkin
> > > > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > > > +1.
> > > > > > > > > > > >
> > > > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine
> > > > > > > > > > > > to have its
> > > > > > > > > > > > own structure and commands.
> > > > > > > > > > > > There is no need to send additional RSS fields when we
> > > > > > > > > > > > configure
> > > > > > > > > > > > inner header hash.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks.
> > > > > > > > > > > Not RSS, hash calculations. It's not critical, but I note
> > > > > > > > > > > that
> > > > > > > > > > > practically you said you will enable this with symmetric
> > > > > > > > > > > hash so it
> > > > > > > > > > > makes sense to me to send this in the same command with
> > > > > > > > > > > the key.
> > > > > > > > > > >
> > > > > > > > > > In the v19, we have,
> > > > > > > > > >
> > > > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires
> > > > > > > > > > VIRTIO_NET_F_CTRL_VQ
> > > > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > > > So it is done along with rss, so in same struct as rss
> > > > > > > > > > config is fine.
> > > > > > > > > Do you mean having both virtio_net_rss_config and
> > > > > > > > > virtio_net_hash_config
> > > > > > > > > have enabled_hash_types?
> > > > > > > > > Like this:
> > > > > > > > >
> > > > > > > > > struct virtio_net_rss_config {
> > > > > > > > > le32 hash_types;
> > > > > > > > > le16 indirection_table_mask;
> > > > > > > > > struct rss_rq_id unclassified_queue;
> > > > > > > > > struct rss_rq_id
> > > > > > > > > indirection_table[indirection_table_length];
> > > > > > > > > le16 max_tx_vq;
> > > > > > > > > u8 hash_key_length;
> > > > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > > > +le32 enabled_tunnel_types;
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > struct virtio_net_hash_config {
> > > > > > > > > le32 hash_types;
> > > > > > > > > -le16 reserved[4];
> > > > > > > > > +le32 enabled_tunnel_types;
> > > > > > > > > +le16 reserved[2];
> > > > > > > > > u8 hash_key_length;
> > > > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > > > };
> > > > > > Oh, I forgot that rss and hash had identical structures.
> > > > > > And we want to keep that I think.
> > > > > >
> > > > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > > > apply to both hash calculation and rss?
> > > > > Yes. What I'm trying to say is that making the inner header hash and
> > > > > RSS/hash calculation clear delimitation will make everything easier.
> > > > > The inner header hash just configures enabled_tunnel_types.
> > > > > The RSS/hash calculation is configured as before without modification.
> > > > >
> > > > > > I note we normally have separate configs for hash and rss.
> > > > > > So to keep it consistent what should we do? two set commands?
> > > > > As I explained above, like outer udp port hash/symmetric toeplitz
> > > > > hash,
> > > > > these fall under the umbrella of RSS/hash calculation.
> > > > Yes but note that symmetric toeplitz hash has to be configured
> > > > separately for RSS and for hashing.
> > >
> > > Yes, this requires a field like \field{mode}, with different values
> > > corresponding to different hashing algorithms, such as toeplitz or
> > > symmetric
> > > toeplitz.
> > > The outer port hash belongs to RSS/hash calculation.
> >
> > So there will be need for more fields.
>
> Yes, the mode field is outside of RSS/hash, as it should be at a higher
> level than RSS/hash.
>
> > To me this implies extending with struct virtio_net_rss_tunnel_config
> > is a better idea since we then have some reserved space to
> > put "mode" down the road (in the reserved[6] space below).
> >
> > No?
> >
> > > >
> > > > > Let's keep the inner header hash simple.
> > > > >
> > > > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > > > Certainly. See:
> > > > >
> > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > >
Re: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 11:38:49AM +, Parav Pandit wrote: > > > > From: virtio-dev@lists.oasis-open.org On > > Behalf Of Michael S. Tsirkin > > Sent: Thursday, June 29, 2023 3:07 AM > > > > > Well SHOULD also does not mean "ok to just ignore". > > > > > > > > This word, or the adjective "RECOMMENDED", mean that there > > > >may exist valid reasons in particular circumstances to > > > > ignore a > > > >particular item, but the full implications must be > > > > understood and > > > >carefully weighed before choosing a different course. > > > > > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is > > not good. > > > > You said this already. Let's not repeat ourselves please. > > > > It's a single word. We need to work on a patch, we can argue about small > > detail > > once it's posted. It's not in your list of plans so I am guessing we need > > someone > > on Red Hat side to work on this? > > Yes, I prefer to work together and before writing the patch. > As just DMA for config space is only point fix problem. > So lets discuss in new thread and improve this for config space and also > other limitations (like support multi-address VQ). > Interesting. What is multi-address VQ? -- 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: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
>
>
> 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > >
> > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > From: Heng Qi
> > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > >
> > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > > > From: Michael S. Tsirkin
> > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > +1.
> > > > > > > > > >
> > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to
> > > > > > > > > > have its
> > > > > > > > > > own structure and commands.
> > > > > > > > > > There is no need to send additional RSS fields when we
> > > > > > > > > > configure
> > > > > > > > > > inner header hash.
> > > > > > > > > >
> > > > > > > > > > Thanks.
> > > > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > > > practically you said you will enable this with symmetric hash
> > > > > > > > > so it
> > > > > > > > > makes sense to me to send this in the same command with the
> > > > > > > > > key.
> > > > > > > > >
> > > > > > > > In the v19, we have,
> > > > > > > >
> > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > So it is done along with rss, so in same struct as rss config
> > > > > > > > is fine.
> > > > > > > Do you mean having both virtio_net_rss_config and
> > > > > > > virtio_net_hash_config
> > > > > > > have enabled_hash_types?
> > > > > > > Like this:
> > > > > > >
> > > > > > > struct virtio_net_rss_config {
> > > > > > > le32 hash_types;
> > > > > > > le16 indirection_table_mask;
> > > > > > > struct rss_rq_id unclassified_queue;
> > > > > > > struct rss_rq_id
> > > > > > > indirection_table[indirection_table_length];
> > > > > > > le16 max_tx_vq;
> > > > > > > u8 hash_key_length;
> > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > +le32 enabled_tunnel_types;
> > > > > > > };
> > > > > > >
> > > > > > > struct virtio_net_hash_config {
> > > > > > > le32 hash_types;
> > > > > > > -le16 reserved[4];
> > > > > > > +le32 enabled_tunnel_types;
> > > > > > > +le16 reserved[2];
> > > > > > > u8 hash_key_length;
> > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > };
> > > > Oh, I forgot that rss and hash had identical structures.
> > > > And we want to keep that I think.
> > > >
> > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > apply to both hash calculation and rss?
> > > Yes. What I'm trying to say is that making the inner header hash and
> > > RSS/hash calculation clear delimitation will make everything easier.
> > > The inner header hash just configures enabled_tunnel_types.
> > > The RSS/hash calculation is configured as before without modification.
> > >
> > > > I note we normally have separate configs for hash and rss.
> > > > So to keep it consistent what should we do? two set commands?
> > > As I explained above, like outer udp port hash/symmetric toeplitz hash,
> > > these fall under the umbrella of RSS/hash calculation.
> > Yes but note that symmetric toeplitz hash has to be configured
> > separately for RSS and for hashing.
>
> Yes, this requires a field like \field{mode}, with different values
> corresponding to different hashing algorithms, such as toeplitz or symmetric
> toeplitz.
> The outer port hash belongs to RSS/hash calculation.
So there will be need for more fields.
To me this implies extending with struct virtio_net_rss_tunnel_config
is a better idea since we then have some reserved space to
put "mode" down the road (in the reserved[6] space below).
No?
> >
> > > Let's keep the inner header hash simple.
> > >
> > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > Certainly. See:
> > >
> > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> > > with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> >
> > It does not really say that.
> Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
> applied to hash and RSS. Yes.
> When one wants to configure inner header hash separately, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
> separately.
> When one wants to configure both inner header hash and RSS/hash, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> The inner header hash is decoupled from RSS/hash, and no extra fields will
> be sent every configuration.
But why is tunnel so different? Rest of things
RE: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
> From: virtio-dev@lists.oasis-open.org On > Behalf Of Michael S. Tsirkin > Sent: Thursday, June 29, 2023 3:07 AM > > > Well SHOULD also does not mean "ok to just ignore". > > > > > > This word, or the adjective "RECOMMENDED", mean that there > > > may exist valid reasons in particular circumstances to ignore a > > > particular item, but the full implications must be understood and > > > carefully weighed before choosing a different course. > > > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is > not good. > > You said this already. Let's not repeat ourselves please. > > It's a single word. We need to work on a patch, we can argue about small > detail > once it's posted. It's not in your list of plans so I am guessing we need > someone > on Red Hat side to work on this? Yes, I prefer to work together and before writing the patch. As just DMA for config space is only point fix problem. So lets discuss in new thread and improve this for config space and also other limitations (like support multi-address VQ). - 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: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
>
>
> 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > >
> > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > From: Heng Qi
> > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > >
> > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > From: Michael S. Tsirkin
> > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > +1.
> > > > > > > >
> > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have
> > > > > > > > its
> > > > > > > > own structure and commands.
> > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > inner header hash.
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > practically you said you will enable this with symmetric hash so
> > > > > > > it
> > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > >
> > > > > > In the v19, we have,
> > > > > >
> > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > So it is done along with rss, so in same struct as rss config is
> > > > > > fine.
> > > > > Do you mean having both virtio_net_rss_config and
> > > > > virtio_net_hash_config
> > > > > have enabled_hash_types?
> > > > > Like this:
> > > > >
> > > > > struct virtio_net_rss_config {
> > > > >le32 hash_types;
> > > > >le16 indirection_table_mask;
> > > > >struct rss_rq_id unclassified_queue;
> > > > >struct rss_rq_id indirection_table[indirection_table_length];
> > > > >le16 max_tx_vq;
> > > > >u8 hash_key_length;
> > > > >u8 hash_key_data[hash_key_length];
> > > > > +le32 enabled_tunnel_types;
> > > > > };
> > > > >
> > > > > struct virtio_net_hash_config {
> > > > >le32 hash_types;
> > > > > -le16 reserved[4];
> > > > > +le32 enabled_tunnel_types;
> > > > > +le16 reserved[2];
> > > > >u8 hash_key_length;
> > > > >u8 hash_key_data[hash_key_length];
> > > > > };
> > Oh, I forgot that rss and hash had identical structures.
> > And we want to keep that I think.
> >
> > But now it's not clear to me: does the same enabled_tunnel_types
> > apply to both hash calculation and rss?
>
> Yes. What I'm trying to say is that making the inner header hash and
> RSS/hash calculation clear delimitation will make everything easier.
> The inner header hash just configures enabled_tunnel_types.
> The RSS/hash calculation is configured as before without modification.
>
> > I note we normally have separate configs for hash and rss.
> > So to keep it consistent what should we do? two set commands?
>
> As I explained above, like outer udp port hash/symmetric toeplitz hash,
> these fall under the umbrella of RSS/hash calculation.
Yes but note that symmetric toeplitz hash has to be configured
separately for RSS and for hashing.
> Let's keep the inner header hash simple.
>
> > Or does enabled_tunnel_types apply to both rss and hash?
>
> Certainly. See:
>
> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
It does not really say that.
> >
> > We should have reserved more space. We can still do it if you like:
> >
> > struct virtio_net_rss_tunnel_config {
> >le32 enabled_tunnel_types;
> >le16 reserved[6];
> >struct virtio_net_rss_config hash;
> > };
> >
> > struct virtio_net_hash_tunnel_config {
> >le32 enabled_tunnel_types;
> >le16 reserved[6];
> >struct virtio_net_hash_config hash;
> > };
> >
> > ?
> >
> >
> >
> >
> >
> > > > >
> > > > > If yes, this should have been discussed in v10 [1] before,
> > > > > enabled_tunnel_types
> > > > > in virtio_net_rss_config will follow the variable length field and
> > > > > cause
> > > > > misalignment.
> > > > >
> > > > > If we let the inner header hash reuse the virtio_net_hash_config
> > > > > structure, it
> > > > > can work, but the only disadvantage is that the configuration of the
> > > > > inner
> > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > Just imagine:
> > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > then if we only want to configure the inner header hash (such as
> > > > > enabled_tunnel_types), it is good for us to send
> > > > > virtio_net_hash_config alone;
> > > > > 2. but then if we want to configure the inner header hash and RSS
> > > > > (such as
> > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > virtio_net_hash_config
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
>
>
> 在 2023/6/30 上午9:36, Parav Pandit 写道:
> >
> > > From: Heng Qi
> > > Sent: Thursday, June 29, 2023 8:54 PM
> > >
> > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin
> > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > >
> > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > +1.
> > > > > >
> > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > own structure and commands.
> > > > > > There is no need to send additional RSS fields when we configure
> > > > > > inner header hash.
> > > > > >
> > > > > > Thanks.
> > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > practically you said you will enable this with symmetric hash so it
> > > > > makes sense to me to send this in the same command with the key.
> > > > >
> > > > In the v19, we have,
> > > >
> > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > So it is done along with rss, so in same struct as rss config is fine.
> > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > have enabled_hash_types?
> > > Like this:
> > >
> > > struct virtio_net_rss_config {
> > > le32 hash_types;
> > > le16 indirection_table_mask;
> > > struct rss_rq_id unclassified_queue;
> > > struct rss_rq_id indirection_table[indirection_table_length];
> > > le16 max_tx_vq;
> > > u8 hash_key_length;
> > > u8 hash_key_data[hash_key_length];
> > > +le32 enabled_tunnel_types;
> > > };
> > >
> > > struct virtio_net_hash_config {
> > > le32 hash_types;
> > > -le16 reserved[4];
> > > +le32 enabled_tunnel_types;
> > > +le16 reserved[2];
> > > u8 hash_key_length;
> > > u8 hash_key_data[hash_key_length];
> > > };
Oh, I forgot that rss and hash had identical structures.
And we want to keep that I think.
But now it's not clear to me: does the same enabled_tunnel_types
apply to both hash calculation and rss?
I note we normally have separate configs for hash and rss.
So to keep it consistent what should we do? two set commands?
Or does enabled_tunnel_types apply to both rss and hash?
We should have reserved more space. We can still do it if you like:
struct virtio_net_rss_tunnel_config {
le32 enabled_tunnel_types;
le16 reserved[6];
struct virtio_net_rss_config hash;
};
struct virtio_net_hash_tunnel_config {
le32 enabled_tunnel_types;
le16 reserved[6];
struct virtio_net_hash_config hash;
};
?
> > >
> > >
> > > If yes, this should have been discussed in v10 [1] before,
> > > enabled_tunnel_types
> > > in virtio_net_rss_config will follow the variable length field and cause
> > > misalignment.
> > >
> > > If we let the inner header hash reuse the virtio_net_hash_config
> > > structure, it
> > > can work, but the only disadvantage is that the configuration of the inner
> > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > Just imagine:
> > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > then if we only want to configure the inner header hash (such as
> > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config
> > > alone;
> > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > indirection table), we need to send all virtio_net_rss_config and
> > > virtio_net_hash_config once, because virtio_net_rss_config now does not
> > > carry
> > > enabled_tunnel_types due to misalignment.
> > >
> > > So, I think the following structure will make it clearer to configure
> > > inner header
> > > hash and RSS/hash calculation.
> > > But in any case, if we still propose to reuse virtio_net_hash_config
> > > proposal, I
> > > am ok, no objection:
> > >
> > > 1. The supported_tunnel_types are placed in the device config space;
> > >
> > Yes. I forgot the variable length part.
> > The second disadvantage I remember now is one need to resupply all the rss
> > hash config parameter and device needs to compare and modify for this one
> > field.
Or it could be an advantage since one normally wants to configure a
symmetric key with this. Further device can just use the new config
with no need to check what the old one was. I'd call it a wash.
> > Given these two disadvantages, I also prefer independent SET command the
> > way you have it.
>
> OK, let's wait for Michael's input again.
>
> Thanks.
This part is not critical to me, but now I understand we need two sets of SET
commands.
> > > 2.
> > > Reserve the following structure:
> > >
> > >struct virtnet_hash_tunnel {
> > > le32 enabled_tunnel_types;
> > >};
> > >
> > > 3. Reserve the SET command for
RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
> From: Heng Qi
> Sent: Thursday, June 29, 2023 8:54 PM
>
> On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin
> > > Sent: Thursday, June 29, 2023 7:48 AM
> >
> >
> > > > > struct virtio_net_hash_config reserved is fine.
> > > >
> > > > +1.
> > > >
> > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > own structure and commands.
> > > > There is no need to send additional RSS fields when we configure
> > > > inner header hash.
> > > >
> > > > Thanks.
> > >
> > > Not RSS, hash calculations. It's not critical, but I note that
> > > practically you said you will enable this with symmetric hash so it
> > > makes sense to me to send this in the same command with the key.
> > >
> >
> > In the v19, we have,
> >
> > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> >
> > So it is done along with rss, so in same struct as rss config is fine.
>
> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> have enabled_hash_types?
> Like this:
>
> struct virtio_net_rss_config {
> le32 hash_types;
> le16 indirection_table_mask;
> struct rss_rq_id unclassified_queue;
> struct rss_rq_id indirection_table[indirection_table_length];
> le16 max_tx_vq;
> u8 hash_key_length;
> u8 hash_key_data[hash_key_length];
> +le32 enabled_tunnel_types;
> };
>
> struct virtio_net_hash_config {
> le32 hash_types;
> -le16 reserved[4];
> +le32 enabled_tunnel_types;
> +le16 reserved[2];
> u8 hash_key_length;
> u8 hash_key_data[hash_key_length];
> };
>
>
> If yes, this should have been discussed in v10 [1] before,
> enabled_tunnel_types
> in virtio_net_rss_config will follow the variable length field and cause
> misalignment.
>
> If we let the inner header hash reuse the virtio_net_hash_config structure, it
> can work, but the only disadvantage is that the configuration of the inner
> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> Just imagine:
> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> then if we only want to configure the inner header hash (such as
> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> 2. but then if we want to configure the inner header hash and RSS (such as
> indirection table), we need to send all virtio_net_rss_config and
> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> enabled_tunnel_types due to misalignment.
>
> So, I think the following structure will make it clearer to configure inner
> header
> hash and RSS/hash calculation.
> But in any case, if we still propose to reuse virtio_net_hash_config
> proposal, I
> am ok, no objection:
>
> 1. The supported_tunnel_types are placed in the device config space;
>
Yes. I forgot the variable length part.
The second disadvantage I remember now is one need to resupply all the rss hash
config parameter and device needs to compare and modify for this one field.
Given these two disadvantages, I also prefer independent SET command the way
you have it.
> 2.
> Reserve the following structure:
>
> struct virtnet_hash_tunnel {
> le32 enabled_tunnel_types;
> };
>
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> command for enabled_tunnel_types.
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
>
> Thanks a lot!
-
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 v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin
> > Sent: Thursday, June 29, 2023 7:48 AM
>
>
> > > > struct virtio_net_hash_config reserved is fine.
> > >
> > > +1.
> > >
> > > Inner header hash is orthogonal to RSS, and it's fine to have its own
> > > structure and commands.
> > > There is no need to send additional RSS fields when we configure inner
> > > header hash.
> > >
> > > Thanks.
> >
> > Not RSS, hash calculations. It's not critical, but I note that practically
> > you said
> > you will enable this with symmetric hash so it makes sense to me to send
> > this in
> > the same command with the key.
> >
>
> In the v19, we have,
>
> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with
> VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>
> So it is done along with rss, so in same struct as rss config is fine.
Do you mean having both virtio_net_rss_config and virtio_net_hash_config
have enabled_hash_types?
Like this:
struct virtio_net_rss_config {
le32 hash_types;
le16 indirection_table_mask;
struct rss_rq_id unclassified_queue;
struct rss_rq_id indirection_table[indirection_table_length];
le16 max_tx_vq;
u8 hash_key_length;
u8 hash_key_data[hash_key_length];
+le32 enabled_tunnel_types;
};
struct virtio_net_hash_config {
le32 hash_types;
-le16 reserved[4];
+le32 enabled_tunnel_types;
+le16 reserved[2];
u8 hash_key_length;
u8 hash_key_data[hash_key_length];
};
If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
in virtio_net_rss_config
will follow the variable length field and cause misalignment.
If we let the inner header hash reuse the virtio_net_hash_config structure, it
can work, but the only disadvantage
is that the configuration of the inner header hash and *RSS*(not hash
calculations) becomes somewhat coupled.
Just imagine:
If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT,
1. then if we only want to configure the inner header hash (such as
enabled_tunnel_types), it is good for us to send
virtio_net_hash_config alone;
2. but then if we want to configure the inner header hash and RSS (such as
indirection table), we need to send all
virtio_net_rss_config and virtio_net_hash_config once, because
virtio_net_rss_config now does not carry enabled_tunnel_types
due to misalignment.
So, I think the following structure will make it clearer to configure inner
header hash and RSS/hash calculation.
But in any case, if we still propose to reuse virtio_net_hash_config proposal,
I am ok, no objection:
1. The supported_tunnel_types are placed in the device config space;
2.
Reserve the following structure:
struct virtnet_hash_tunnel {
le32 enabled_tunnel_types;
};
3. Reserve the SET command for enabled_tunnel_types and remove the GET
command for enabled_tunnel_types.
[1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
Thanks a lot!
-
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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Thursday, June 29, 2023 7:48 AM > > > struct virtio_net_hash_config reserved is fine. > > > > +1. > > > > Inner header hash is orthogonal to RSS, and it's fine to have its own > > structure and commands. > > There is no need to send additional RSS fields when we configure inner > > header hash. > > > > Thanks. > > Not RSS, hash calculations. It's not critical, but I note that practically > you said > you will enable this with symmetric hash so it makes sense to me to send this > in > the same command with the key. > In the v19, we have, +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT. So it is done along with rss, so in same struct as rss config is fine.
Re: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
在 2023/6/29 下午7:48, Michael S. Tsirkin 写道: On Thu, Jun 29, 2023 at 10:05:09AM +0800, Heng Qi wrote: 在 2023/6/29 上午9:56, Parav Pandit 写道: From: Michael S. Tsirkin Sent: Wednesday, June 28, 2023 3:45 PM Maybe I get it. You want to use the new features as a carrot to force drivers to implement DMA? You suspect they will ignore the spec requirement just because things seem to work? Right because it is not a must normative. Well SHOULD also does not mean "ok to just ignore". This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. So rather a good design is device tells the starting offset for the extended config space. And extended config space MUST be accessed using a DMA. With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from. This also gives the ability to maintain current config as MMIO for backward compatibility. There's some logic here, for sure. you just might be right. However, surely we can discuss this small tweak in 1.4 timeframe? Sure, if we prefer the DMA approach I don't have a problem in adding temporary one field to config space. I propose to add a line to the spec " Device Configuration Space" section, something like, Note: Any new device configuration space fields additional MUST consider accessing such fields via a DMA interface. And this will guide the new patches of what to do instead of last moment rush. Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to MMIO might be an option for qemu helping us debug DMA issues. There are too many queues whose debugging is needed and MMIO likely not the way to debug. The time to discuss this detail would be around when proposal for the DMA access to config space is on list though: I feel this SHOULD vs MUST is a small enough detail. From implementation POV it is certainly critical and good step forward to optimize virtio interface. Going back to inner hash. If we move supported_tunnels back to config space, do you feel we still need GET or just drop it? I note we do not have GET for either hash or rss config. For hash and rss config, debugging is missing. :) Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config. Great! Glad to hear this! And if we no longer have GET is there still a reason for a separate command as opposed to a field in virtio_net_hash_config? I know this was done in v11 but there it was misaligned. We went with a command because we needed it for supported_tunnels but now that is no longer the case and there are reserved words in virtio_net_hash_config ... Let me know how you feel it about that, not critical for me. struct virtio_net_hash_config reserved is fine. +1. Inner header hash is orthogonal to RSS, and it's fine to have its own structure and commands. There is no need to send additional RSS fields when we configure inner header hash. Thanks. Not RSS, hash calculations. It's not critical, but I note that practically you said you will enable this with symmetric hash so it makes sense to me to send this in the same command This works for me. Thanks. with the key. Not critical though if there's opposition. - 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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Thursday, June 29, 2023 7:46 AM
> Just a second, when I talked about virtio_net_hash_config I meant this:
>
>
> struct virtio_net_hash_config {
> le32 hash_types;
> - le16 reserved[4];
> + le32 enabled_tunnel_types;
> + le16 reserved[2];
> u8 hash_key_length;
> u8 hash_key_data[hash_key_length];
> };
>
> and then we do not add a new command we modify the hash config command.
>
> Parav still ok with you?
yes, looks good.
-
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 v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 07:54:29AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 29, 2023 at 11:31:58AM +0800, Jason Wang wrote:
> >
> > 在 2023/6/28 18:10, Michael S. Tsirkin 写道:
> > > On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> > > > On Wed, Jun 28, 2023 at 12:35 AM Heng Qi
> > > > wrote:
> > > > > 1. Currently, a received encapsulated packet has an outer and an
> > > > > inner header, but
> > > > > the virtio device is unable to calculate the hash for the inner
> > > > > header. The same
> > > > > flow can traverse through different tunnels, resulting in the
> > > > > encapsulated
> > > > > packets being spread across multiple receive queues (refer to the
> > > > > figure below).
> > > > > However, in certain scenarios, we may need to direct these
> > > > > encapsulated packets of
> > > > > the same flow to a single receive queue. This facilitates the
> > > > > processing
> > > > > of the flow by the same CPU to improve performance (warm caches, less
> > > > > locking, etc.).
> > > > >
> > > > > client1client2
> > > > >|+---+ |
> > > > >+--->|tunnels|<+
> > > > > +---+
> > > > >| |
> > > > >v v
> > > > >+-+
> > > > >| monitoring host |
> > > > >+-+
> > > > >
> > > > > To achieve this, the device can calculate a symmetric hash based on
> > > > > the inner headers
> > > > > of the same flow.
> > > > >
> > > > > 2. For legacy systems, they may lack entropy fields which modern
> > > > > protocols have in
> > > > > the outer header, resulting in multiple flows with the same outer
> > > > > header but
> > > > > different inner headers being directed to the same receive queue.
> > > > > This results in
> > > > > poor receive performance.
> > > > >
> > > > > To address this limitation, inner header hash can be used to enable
> > > > > the device to advertise
> > > > > the capability to calculate the hash for the inner packet, regaining
> > > > > better receive performance.
> > > > >
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > > > > Signed-off-by: Heng Qi
> > > > > Reviewed-by: Xuan Zhuo
> > > > > Reviewed-by: Parav Pandit
> > > > > ---
> > > > > v18->v19:
> > > > > 1. Have a single structure instead of two. @Michael S .
> > > > > Tsirkin
> > > > > 2. Some small rewrites. @Michael S . Tsirkin
> > > > > 3. Rebase to master.
> > > > >
> > > > > v17->v18:
> > > > > 1. Some rewording suggestions from Michael (Thanks!).
> > > > > 2. Use 0 to disable inner header hash and remove
> > > > > VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > > > > v16->v17:
> > > > > 1. Some small rewrites. @Parav Pandit
> > > > > 2. Add Parav's Reviewed-by tag (Thanks!).
> > > > >
> > > > > v15->v16:
> > > > > 1. Remove the hash_option. In order to delimit the inner
> > > > > header hash and RSS
> > > > > configuration, the ability to configure the outer src udp
> > > > > port hash is given
> > > > > to RSS. This is orthogonal to inner header hash, which
> > > > > will be done in the
> > > > > RSS capability extension topic (considered as an RSS
> > > > > extension together
> > > > > with the symmetric toeplitz hash algorithm, etc.). @Parav
> > > > > Pandit @Michael S . Tsirkin
> > > > > 2. Fix a 'field' typo. @Parav Pandit
> > > > >
> > > > > v14->v15:
> > > > > 1. Add tunnel hash option suggested by @Michael S . Tsirkin
> > > > > 2. Adjust some descriptions.
> > > > >
> > > > > v13->v14:
> > > > > 1. Move supported_hash_tunnel_types from config space into
> > > > > cvq command. @Parav Pandit
> > > > I may miss some discussions, but this complicates the provisioning a
> > > > lot.
> > > >
> > > > Having it in the config space, then a type agnostic provisioning
> > > > through config space + feature bits just works fine.
> > > >
> > > > If we move it only via cvq, we need device specific provisioning
> > > > interface.
> > > >
> > > > Thanks
> > > Yea that's what I said too. Debugging too. I think we should build a
> > > consistent solution that allows accessing config space through DMA,
> > > separately from this effort. Parav do you think you can live with this
> > > approach so this specific proposal can move forward?
> >
> >
> > We can probably go another way, invent a new device configuration space
> > capability which fixed size like PCI configuration access capability?
> >
> > struct virtio_pci_cfg_cap {
> > struct virtio_pci_cap cap;
> > u8 dev_cfg_data[4]; /* Data for device configuration space access.
> > */
> > };
> >
> > So it won't grow as the size of device
[virtio-dev] Re: [virtio-comment] Re: [PATCH v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 11:31:58AM +0800, Jason Wang wrote:
>
> 在 2023/6/28 18:10, Michael S. Tsirkin 写道:
> > On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> > > On Wed, Jun 28, 2023 at 12:35 AM Heng Qi wrote:
> > > > 1. Currently, a received encapsulated packet has an outer and an inner
> > > > header, but
> > > > the virtio device is unable to calculate the hash for the inner header.
> > > > The same
> > > > flow can traverse through different tunnels, resulting in the
> > > > encapsulated
> > > > packets being spread across multiple receive queues (refer to the
> > > > figure below).
> > > > However, in certain scenarios, we may need to direct these encapsulated
> > > > packets of
> > > > the same flow to a single receive queue. This facilitates the processing
> > > > of the flow by the same CPU to improve performance (warm caches, less
> > > > locking, etc.).
> > > >
> > > > client1client2
> > > >|+---+ |
> > > >+--->|tunnels|<+
> > > > +---+
> > > >| |
> > > >v v
> > > >+-+
> > > >| monitoring host |
> > > >+-+
> > > >
> > > > To achieve this, the device can calculate a symmetric hash based on the
> > > > inner headers
> > > > of the same flow.
> > > >
> > > > 2. For legacy systems, they may lack entropy fields which modern
> > > > protocols have in
> > > > the outer header, resulting in multiple flows with the same outer
> > > > header but
> > > > different inner headers being directed to the same receive queue. This
> > > > results in
> > > > poor receive performance.
> > > >
> > > > To address this limitation, inner header hash can be used to enable the
> > > > device to advertise
> > > > the capability to calculate the hash for the inner packet, regaining
> > > > better receive performance.
> > > >
> > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > > > Signed-off-by: Heng Qi
> > > > Reviewed-by: Xuan Zhuo
> > > > Reviewed-by: Parav Pandit
> > > > ---
> > > > v18->v19:
> > > > 1. Have a single structure instead of two. @Michael S . Tsirkin
> > > > 2. Some small rewrites. @Michael S . Tsirkin
> > > > 3. Rebase to master.
> > > >
> > > > v17->v18:
> > > > 1. Some rewording suggestions from Michael (Thanks!).
> > > > 2. Use 0 to disable inner header hash and remove
> > > > VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > > > v16->v17:
> > > > 1. Some small rewrites. @Parav Pandit
> > > > 2. Add Parav's Reviewed-by tag (Thanks!).
> > > >
> > > > v15->v16:
> > > > 1. Remove the hash_option. In order to delimit the inner
> > > > header hash and RSS
> > > > configuration, the ability to configure the outer src udp
> > > > port hash is given
> > > > to RSS. This is orthogonal to inner header hash, which will
> > > > be done in the
> > > > RSS capability extension topic (considered as an RSS
> > > > extension together
> > > > with the symmetric toeplitz hash algorithm, etc.). @Parav
> > > > Pandit @Michael S . Tsirkin
> > > > 2. Fix a 'field' typo. @Parav Pandit
> > > >
> > > > v14->v15:
> > > > 1. Add tunnel hash option suggested by @Michael S . Tsirkin
> > > > 2. Adjust some descriptions.
> > > >
> > > > v13->v14:
> > > > 1. Move supported_hash_tunnel_types from config space into cvq
> > > > command. @Parav Pandit
> > > I may miss some discussions, but this complicates the provisioning a lot.
> > >
> > > Having it in the config space, then a type agnostic provisioning
> > > through config space + feature bits just works fine.
> > >
> > > If we move it only via cvq, we need device specific provisioning
> > > interface.
> > >
> > > Thanks
> > Yea that's what I said too. Debugging too. I think we should build a
> > consistent solution that allows accessing config space through DMA,
> > separately from this effort. Parav do you think you can live with this
> > approach so this specific proposal can move forward?
>
>
> We can probably go another way, invent a new device configuration space
> capability which fixed size like PCI configuration access capability?
>
> struct virtio_pci_cfg_cap {
> struct virtio_pci_cap cap;
> u8 dev_cfg_data[4]; /* Data for device configuration space access.
> */
> };
>
> So it won't grow as the size of device configuration space grows.
>
> Thanks
It is true, it does not have to be DMA strictly speaking.
The basic issue is with synchronous access.
We can change the capability in some way to allow asynch then
that works. E.g. make device change length to 0 and driver
must poll before considering the operation done.
Having said
[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 10:05:09AM +0800, Heng Qi wrote: > > > 在 2023/6/29 上午9:56, Parav Pandit 写道: > > > > > From: Michael S. Tsirkin > > > Sent: Wednesday, June 28, 2023 3:45 PM > > > > > Maybe I get it. You want to use the new features as a carrot to > > > > > force drivers to implement DMA? You suspect they will ignore the > > > > > spec requirement just because things seem to work? > > > > > > > > > Right because it is not a must normative. > > > Well SHOULD also does not mean "ok to just ignore". > > > > > > This word, or the adjective "RECOMMENDED", mean that there > > > may exist valid reasons in particular circumstances to ignore a > > > particular item, but the full implications must be understood and > > > carefully weighed before choosing a different course. > > > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. > > So rather a good design is device tells the starting offset for the > > extended config space. > > And extended config space MUST be accessed using a DMA. > > With this sw can have infinite size MMIO and hw device forces DMA based on > > its implementation of where to start DMA from. > > This also gives the ability to maintain current config as MMIO for backward > > compatibility. > > > > > > > > There's some logic here, for sure. you just might be right. > > > > > > > > > > However, surely we can discuss this small tweak in 1.4 timeframe? > > > > Sure, if we prefer the DMA approach I don't have a problem in adding > > > temporary one field to config space. > > > > I propose to add a line to the spec " Device Configuration Space" > > > > section, something like, > > > > > > > > Note: Any new device configuration space fields additional MUST consider > > > accessing such fields via a DMA interface. > > > > And this will guide the new patches of what to do instead of last moment > > > rush. > > > > > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how > > > switching to > > > MMIO might be an option for qemu helping us debug DMA issues. > > > > > There are too many queues whose debugging is needed and MMIO likely not the > > way to debug. > > > The time to discuss this detail would be around when proposal for the DMA > > > access to config space is on list though: I feel this SHOULD vs MUST is a > > > small > > > enough detail. > > > > > From implementation POV it is certainly critical and good step forward to > > optimize virtio interface. > > > Going back to inner hash. If we move supported_tunnels back to config > > > space, > > > do you feel we still need GET or just drop it? I note we do not have GET > > > for > > > either hash or rss config. > > > > > For hash and rss config, debugging is missing. :) > > Yes, we can drop the GET after switching supported_tunnels to struct > > virtio_net_hash_config. > > Great! Glad to hear this! > > > > And if we no longer have GET is there still a reason for a separate > > > command as > > > opposed to a field in virtio_net_hash_config? > > > I know this was done in v11 but there it was misaligned. > > > We went with a command because we needed it for supported_tunnels but > > > now that is no longer the case and there are reserved words in > > > virtio_net_hash_config ... > > > > > > Let me know how you feel it about that, not critical for me. > > struct virtio_net_hash_config reserved is fine. > > +1. > > Inner header hash is orthogonal to RSS, and it's fine to have its own > structure and commands. > There is no need to send additional RSS fields when we configure inner > header hash. > > Thanks. Not RSS, hash calculations. It's not critical, but I note that practically you said you will enable this with symmetric hash so it makes sense to me to send this in the same command with the key. Not critical though if there's opposition. -- 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 v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 02:40:42PM +0800, Heng Qi wrote:
> On Thu, Jun 29, 2023 at 01:56:34AM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin
> > > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > > Maybe I get it. You want to use the new features as a carrot to
> > > > > force drivers to implement DMA? You suspect they will ignore the
> > > > > spec requirement just because things seem to work?
> > > > >
> > > > Right because it is not a must normative.
> > >
> > > Well SHOULD also does not mean "ok to just ignore".
> > >
> > > This word, or the adjective "RECOMMENDED", mean that there
> > > may exist valid reasons in particular circumstances to ignore a
> > > particular item, but the full implications must be understood and
> > > carefully weighed before choosing a different course.
> > >
> > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> > So rather a good design is device tells the starting offset for the
> > extended config space.
> > And extended config space MUST be accessed using a DMA.
> > With this sw can have infinite size MMIO and hw device forces DMA based on
> > its implementation of where to start DMA from.
> > This also gives the ability to maintain current config as MMIO for backward
> > compatibility.
> >
> > >
> > >
> > > > > There's some logic here, for sure. you just might be right.
> > > > >
> > > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > > >
> > > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > > temporary one field to config space.
> > > >
> > > > I propose to add a line to the spec " Device Configuration Space"
> > > > section, something like,
> > > >
> > > > Note: Any new device configuration space fields additional MUST consider
> > > accessing such fields via a DMA interface.
> > > >
> > > > And this will guide the new patches of what to do instead of last moment
> > > rush.
> > >
> > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how
> > > switching to
> > > MMIO might be an option for qemu helping us debug DMA issues.
> > >
> > There are too many queues whose debugging is needed and MMIO likely not the
> > way to debug.
> >
> > > The time to discuss this detail would be around when proposal for the DMA
> > > access to config space is on list though: I feel this SHOULD vs MUST is a
> > > small
> > > enough detail.
> > >
> > From implementation POV it is certainly critical and good step forward to
> > optimize virtio interface.
> >
> > > Going back to inner hash. If we move supported_tunnels back to config
> > > space,
> > > do you feel we still need GET or just drop it? I note we do not have GET
> > > for
> > > either hash or rss config.
> > >
> > For hash and rss config, debugging is missing. :)
> > Yes, we can drop the GET after switching supported_tunnels to struct
> > virtio_net_hash_config.
> >
>
> I would like to make sure if we're aligned. The new version should contain
> the following:
> 1. The supported_tunnel_types are placed in the device config space;
> 2. Reserve the following structure:
>
> struct virtnet_hash_tunnel {
> le32 enabled_tunnel_types;
> };
>
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> command for enabled_tunnel_types.
>
> If there is no problem, I will modify it accordingly.
>
> Thanks!
> > > And if we no longer have GET is there still a reason for a separate
> > > command as
> > > opposed to a field in virtio_net_hash_config?
> > > I know this was done in v11 but there it was misaligned.
> > > We went with a command because we needed it for supported_tunnels but
> > > now that is no longer the case and there are reserved words in
> > > virtio_net_hash_config ...
> > >
> > > Let me know how you feel it about that, not critical for me.
> >
> > struct virtio_net_hash_config reserved is fine.
Just a second, when I talked about virtio_net_hash_config I meant this:
struct virtio_net_hash_config {
le32 hash_types;
- le16 reserved[4];
+ le32 enabled_tunnel_types;
+ le16 reserved[2];
u8 hash_key_length;
u8 hash_key_data[hash_key_length];
};
and then we do not add a new command we modify the hash config command.
Parav still ok with you?
--
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 v19] virtio-net: support inner header hash
> From: Heng Qi
> Sent: Thursday, June 29, 2023 2:41 AM
> I would like to make sure if we're aligned. The new version should contain the
> following:
> 1. The supported_tunnel_types are placed in the device config space; 2.
> Reserve the following structure:
>
> struct virtnet_hash_tunnel {
> le32 enabled_tunnel_types;
> };
>
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> command for enabled_tunnel_types.
>
> If there is no problem, I will modify it accordingly.
Ack.
-
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 v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 01:56:34AM +, Parav Pandit wrote: > > > > From: Michael S. Tsirkin > > Sent: Wednesday, June 28, 2023 3:45 PM > > > > Maybe I get it. You want to use the new features as a carrot to > > > > force drivers to implement DMA? You suspect they will ignore the > > > > spec requirement just because things seem to work? > > > > > > > Right because it is not a must normative. > > > > Well SHOULD also does not mean "ok to just ignore". > > > > This word, or the adjective "RECOMMENDED", mean that there > >may exist valid reasons in particular circumstances to ignore a > >particular item, but the full implications must be understood and > >carefully weighed before choosing a different course. > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. You said this already. Let's not repeat ourselves please. It's a single word. We need to work on a patch, we can argue about small detail once it's posted. It's not in your list of plans so I am guessing we need someone on Red Hat side to work 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
[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 01:56:34AM +, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin
> > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > Maybe I get it. You want to use the new features as a carrot to
> > > > force drivers to implement DMA? You suspect they will ignore the
> > > > spec requirement just because things seem to work?
> > > >
> > > Right because it is not a must normative.
> >
> > Well SHOULD also does not mean "ok to just ignore".
> >
> > This word, or the adjective "RECOMMENDED", mean that there
> >may exist valid reasons in particular circumstances to ignore a
> >particular item, but the full implications must be understood and
> >carefully weighed before choosing a different course.
> >
> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> So rather a good design is device tells the starting offset for the extended
> config space.
> And extended config space MUST be accessed using a DMA.
> With this sw can have infinite size MMIO and hw device forces DMA based on
> its implementation of where to start DMA from.
> This also gives the ability to maintain current config as MMIO for backward
> compatibility.
>
> >
> >
> > > > There's some logic here, for sure. you just might be right.
> > > >
> > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > >
> > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > temporary one field to config space.
> > >
> > > I propose to add a line to the spec " Device Configuration Space"
> > > section, something like,
> > >
> > > Note: Any new device configuration space fields additional MUST consider
> > accessing such fields via a DMA interface.
> > >
> > > And this will guide the new patches of what to do instead of last moment
> > rush.
> >
> > Yea, except again I'd probably make it a SHOULD: e.g. I can see how
> > switching to
> > MMIO might be an option for qemu helping us debug DMA issues.
> >
> There are too many queues whose debugging is needed and MMIO likely not the
> way to debug.
>
> > The time to discuss this detail would be around when proposal for the DMA
> > access to config space is on list though: I feel this SHOULD vs MUST is a
> > small
> > enough detail.
> >
> From implementation POV it is certainly critical and good step forward to
> optimize virtio interface.
>
> > Going back to inner hash. If we move supported_tunnels back to config space,
> > do you feel we still need GET or just drop it? I note we do not have GET for
> > either hash or rss config.
> >
> For hash and rss config, debugging is missing. :)
> Yes, we can drop the GET after switching supported_tunnels to struct
> virtio_net_hash_config.
>
I would like to make sure if we're aligned. The new version should contain the
following:
1. The supported_tunnel_types are placed in the device config space;
2. Reserve the following structure:
struct virtnet_hash_tunnel {
le32 enabled_tunnel_types;
};
3. Reserve the SET command for enabled_tunnel_types and remove the GET command
for enabled_tunnel_types.
If there is no problem, I will modify it accordingly.
Thanks!
> > And if we no longer have GET is there still a reason for a separate command
> > as
> > opposed to a field in virtio_net_hash_config?
> > I know this was done in v11 but there it was misaligned.
> > We went with a command because we needed it for supported_tunnels but
> > now that is no longer the case and there are reserved words in
> > virtio_net_hash_config ...
> >
> > Let me know how you feel it about that, not critical for me.
>
> struct virtio_net_hash_config reserved is fine.
-
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 v19] virtio-net: support inner header hash
On Thu, Jun 29, 2023 at 01:56:34AM +, Parav Pandit wrote: > > > > From: Michael S. Tsirkin > > Sent: Wednesday, June 28, 2023 3:45 PM > > > > Maybe I get it. You want to use the new features as a carrot to > > > > force drivers to implement DMA? You suspect they will ignore the > > > > spec requirement just because things seem to work? > > > > > > > Right because it is not a must normative. > > > > Well SHOULD also does not mean "ok to just ignore". > > > > This word, or the adjective "RECOMMENDED", mean that there > >may exist valid reasons in particular circumstances to ignore a > >particular item, but the full implications must be understood and > >carefully weighed before choosing a different course. > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. > So rather a good design is device tells the starting offset for the extended > config space. > And extended config space MUST be accessed using a DMA. > With this sw can have infinite size MMIO and hw device forces DMA based on > its implementation of where to start DMA from. > This also gives the ability to maintain current config as MMIO for backward > compatibility. Really we do not need offsets. We have feature bits, and offsets are implied. But let's start a separate thread for discussions of this? > > > > > > > > There's some logic here, for sure. you just might be right. > > > > > > > > However, surely we can discuss this small tweak in 1.4 timeframe? > > > > > > Sure, if we prefer the DMA approach I don't have a problem in adding > > temporary one field to config space. > > > > > > I propose to add a line to the spec " Device Configuration Space" > > > section, something like, > > > > > > Note: Any new device configuration space fields additional MUST consider > > accessing such fields via a DMA interface. > > > > > > And this will guide the new patches of what to do instead of last moment > > rush. > > > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how > > switching to > > MMIO might be an option for qemu helping us debug DMA issues. > > > There are too many queues whose debugging is needed and MMIO likely not the > way to debug. > > > The time to discuss this detail would be around when proposal for the DMA > > access to config space is on list though: I feel this SHOULD vs MUST is a > > small > > enough detail. > > > >From implementation POV it is certainly critical and good step forward to > >optimize virtio interface. > > > Going back to inner hash. If we move supported_tunnels back to config space, > > do you feel we still need GET or just drop it? I note we do not have GET for > > either hash or rss config. > > > For hash and rss config, debugging is missing. :) > Yes, we can drop the GET after switching supported_tunnels to struct > virtio_net_hash_config. > > > And if we no longer have GET is there still a reason for a separate command > > as > > opposed to a field in virtio_net_hash_config? > > I know this was done in v11 but there it was misaligned. > > We went with a command because we needed it for supported_tunnels but > > now that is no longer the case and there are reserved words in > > virtio_net_hash_config ... > > > > Let me know how you feel it about that, not critical for me. > > struct virtio_net_hash_config reserved is fine. - 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 v19] virtio-net: support inner header hash
在 2023/6/29 上午9:56, Parav Pandit 写道: From: Michael S. Tsirkin Sent: Wednesday, June 28, 2023 3:45 PM Maybe I get it. You want to use the new features as a carrot to force drivers to implement DMA? You suspect they will ignore the spec requirement just because things seem to work? Right because it is not a must normative. Well SHOULD also does not mean "ok to just ignore". This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. So rather a good design is device tells the starting offset for the extended config space. And extended config space MUST be accessed using a DMA. With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from. This also gives the ability to maintain current config as MMIO for backward compatibility. There's some logic here, for sure. you just might be right. However, surely we can discuss this small tweak in 1.4 timeframe? Sure, if we prefer the DMA approach I don't have a problem in adding temporary one field to config space. I propose to add a line to the spec " Device Configuration Space" section, something like, Note: Any new device configuration space fields additional MUST consider accessing such fields via a DMA interface. And this will guide the new patches of what to do instead of last moment rush. Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to MMIO might be an option for qemu helping us debug DMA issues. There are too many queues whose debugging is needed and MMIO likely not the way to debug. The time to discuss this detail would be around when proposal for the DMA access to config space is on list though: I feel this SHOULD vs MUST is a small enough detail. From implementation POV it is certainly critical and good step forward to optimize virtio interface. Going back to inner hash. If we move supported_tunnels back to config space, do you feel we still need GET or just drop it? I note we do not have GET for either hash or rss config. For hash and rss config, debugging is missing. :) Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config. Great! Glad to hear this! And if we no longer have GET is there still a reason for a separate command as opposed to a field in virtio_net_hash_config? I know this was done in v11 but there it was misaligned. We went with a command because we needed it for supported_tunnels but now that is no longer the case and there are reserved words in virtio_net_hash_config ... Let me know how you feel it about that, not critical for me. struct virtio_net_hash_config reserved is fine. +1. Inner header hash is orthogonal to RSS, and it's fine to have its own structure and commands. There is no need to send additional RSS fields when we configure inner header hash. 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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, June 28, 2023 3:45 PM > > > Maybe I get it. You want to use the new features as a carrot to > > > force drivers to implement DMA? You suspect they will ignore the > > > spec requirement just because things seem to work? > > > > > Right because it is not a must normative. > > Well SHOULD also does not mean "ok to just ignore". > > This word, or the adjective "RECOMMENDED", mean that there > may exist valid reasons in particular circumstances to ignore a > particular item, but the full implications must be understood and > carefully weighed before choosing a different course. > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good. So rather a good design is device tells the starting offset for the extended config space. And extended config space MUST be accessed using a DMA. With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from. This also gives the ability to maintain current config as MMIO for backward compatibility. > > > > > There's some logic here, for sure. you just might be right. > > > > > > However, surely we can discuss this small tweak in 1.4 timeframe? > > > > Sure, if we prefer the DMA approach I don't have a problem in adding > temporary one field to config space. > > > > I propose to add a line to the spec " Device Configuration Space" > > section, something like, > > > > Note: Any new device configuration space fields additional MUST consider > accessing such fields via a DMA interface. > > > > And this will guide the new patches of what to do instead of last moment > rush. > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching > to > MMIO might be an option for qemu helping us debug DMA issues. > There are too many queues whose debugging is needed and MMIO likely not the way to debug. > The time to discuss this detail would be around when proposal for the DMA > access to config space is on list though: I feel this SHOULD vs MUST is a > small > enough detail. > >From implementation POV it is certainly critical and good step forward to >optimize virtio interface. > Going back to inner hash. If we move supported_tunnels back to config space, > do you feel we still need GET or just drop it? I note we do not have GET for > either hash or rss config. > For hash and rss config, debugging is missing. :) Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config. > And if we no longer have GET is there still a reason for a separate command as > opposed to a field in virtio_net_hash_config? > I know this was done in v11 but there it was misaligned. > We went with a command because we needed it for supported_tunnels but > now that is no longer the case and there are reserved words in > virtio_net_hash_config ... > > Let me know how you feel it about that, not critical for me. struct virtio_net_hash_config reserved is fine. - 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 v19] virtio-net: support inner header hash
On Wed, Jun 28, 2023 at 05:38:29PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, June 28, 2023 1:24 PM > > > > > Because when device has two ways to access config space, it always have to > > account and build the interface that, hey some driver will not use DMA. > > > Hence have it always in the MMIO accessible area. > > > > Maybe I get it. You want to use the new features as a carrot to force > > drivers to > > implement DMA? You suspect they will ignore the spec requirement just > > because things seem to work? > > > Right because it is not a must normative. Well SHOULD also does not mean "ok to just ignore". This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. > > There's some logic here, for sure. you just might be right. > > > > However, surely we can discuss this small tweak in 1.4 timeframe? > > Sure, if we prefer the DMA approach I don't have a problem in adding > temporary one field to config space. > > I propose to add a line to the spec " Device Configuration Space" section, > something like, > > Note: Any new device configuration space fields additional MUST consider > accessing such fields via a DMA interface. > > And this will guide the new patches of what to do instead of last moment rush. Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to MMIO might be an option for qemu helping us debug DMA issues. The time to discuss this detail would be around when proposal for the DMA access to config space is on list though: I feel this SHOULD vs MUST is a small enough detail. Going back to inner hash. If we move supported_tunnels back to config space, do you feel we still need GET or just drop it? I note we do not have GET for either hash or rss config. And if we no longer have GET is there still a reason for a separate command as opposed to a field in virtio_net_hash_config? I know this was done in v11 but there it was misaligned. We went with a command because we needed it for supported_tunnels but now that is no longer the case and there are reserved words in virtio_net_hash_config ... Let me know how you feel it about that, not critical for me. -- 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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, June 28, 2023 1:24 PM > > Because when device has two ways to access config space, it always have to > account and build the interface that, hey some driver will not use DMA. > > Hence have it always in the MMIO accessible area. > > Maybe I get it. You want to use the new features as a carrot to force drivers > to > implement DMA? You suspect they will ignore the spec requirement just > because things seem to work? > Right because it is not a must normative. > There's some logic here, for sure. you just might be right. > > However, surely we can discuss this small tweak in 1.4 timeframe? Sure, if we prefer the DMA approach I don't have a problem in adding temporary one field to config space. I propose to add a line to the spec " Device Configuration Space" section, something like, Note: Any new device configuration space fields additional MUST consider accessing such fields via a DMA interface. And this will guide the new patches of what to do instead of last moment rush. - 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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, June 28, 2023 1:16 PM > > 1. Because than device is 100% sure that it does not fulfill MMIO > > needs 2. Single interface to access extended config space, which is what you > asked for. > > 3. Single driver code because there is single way to get it. > > > > Right. But I think there's a better way. > Just say that any feature in MMIO MUST be also in DMA. > So any modern driver will have no reason at all to use MMIO - DMA is a > superset. > How does that relax the need of MMIO for the device? > If we say that drivers should use DMA, they will. If we additionally explain > that > some features might not be in MMIO no sane driver will use MMIO if it does not > have to. > Or if it does then it has violated the spec and will get less features? > So than why to have it in MMIO anyway? Why not say driver must use dma? > > This demands two ways of access and that conflicts with your point of desire > to do single way. > > I proposed single way, extended config space via DMA interface, that is > > really > as simple as that. > > > My desire is to have a single way that works for everything. > We have legacy so we will have legacy ways too, these might not work for > everything. Here is what can work well, A device tells the offset of the extended config space to the driver. Any field starting from that offset must be accessed via a DMA interface. This way, driver must support DMA and all new features come from there. If device chose to use MMIO, say sw, it can still do it using MMIO. - 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 v19] virtio-net: support inner header hash
On Wed, Jun 28, 2023 at 05:06:40PM +, Parav Pandit wrote: > > > > > > > > Please let people just focus on fixing config space instead of > > > > temporary cvq hacks. > > > > > > The ask is to have predictable sizing for existing defined config space > > > field and > > not keep infinitely growing by two interfaces. > > > > I don't know what "predictable sizing" is or why does it matter. > > > Because when device has two ways to access config space, it always have to > account and build the interface that, hey some driver will not use DMA. > Hence have it always in the MMIO accessible area. Maybe I get it. You want to use the new features as a carrot to force drivers to implement DMA? You suspect they will ignore the spec requirement just because things seem to work? There's some logic here, for sure. you just might be right. However, surely we can discuss this small tweak in 1.4 timeframe? -- 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 v19] virtio-net: support inner header hash
On Wed, Jun 28, 2023 at 05:06:40PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, June 28, 2023 12:46 PM > > > Actually, my preblem is exactly if there's no one single interface that can > > do > > everything. > > > Extended config space reading via single DMA interface is just enough. > > > > > Two interfaces that do the same might seem redundant but there could be good > > reasons for this, in this case legacy. > > > But legacy should not force it to newer interface. More below. > > > See the difference? > > > > > > I didn't propose two interfaces. Let me explain again. > > > > > > I proposed, > > > a. all new fields to go via vq (single interface) b. all existing > > > fields to stay in cfg space (single interface) c. if one wants to > > > optimize optionally existing fields can utilize (same flexibility how you > > > asked > > for PF and VF notification). > > > > > > > > > > We already need config space with provisioning. > > > Provisioning does provision features, config space and control parameters, > > msix vectors and more. > > > Provisioning does _not_ need config space. > > > > I mean it needs to provision it. > > > No matter there is config space of dma, a device to be provisioned. > > > > > In fact e.g. on Linux we want drivers to keep doing virtio_cread() > > > > and all the DMA tricks should be hidden behind the scenes. You don't > > > > like it that config space is on-chip? Build an interface for it not > > > > to be on chip. There is simply no downside to that. > > > > > > > This is the ask to build software interface between driver and device to > > > not > > demand on-chip config space more importantly _predictably_. > > > > Absolutely. So we would say that driver SHOULD use the DMA interface if it > > is > > present. > > > I am asking for driver MUST use the DMA interface for _extended_ config space. > Why? > For two reasons. > > 1. Because than device is 100% sure that it does not fulfill MMIO needs > 2. Single interface to access extended config space, which is what you asked > for. > 3. Single driver code because there is single way to get it. > Right. But I think there's a better way. Just say that any feature in MMIO MUST be also in DMA. So any modern driver will have no reason at all to use MMIO - DMA is a superset. > > > > I came back after a small break and I still see this discussion that > > > > keeps playing out as a big distraction. > > > > In particular where are you doing to store the things that are for DMA? > > > > In system RAM? We don't *have* anywhere in system RAM to store this, > > > > we will need an interface to allocate system RAM and pass it to > > > > device for this idea to work. So in 1.3 where we won't have this, > > > > there is much less of an excuse to use a vq. > > > To store what? > > > CVQ is already there to do GET and SET operation. > > > > To store whatever value CVQ is requesting. Where is it going to come from? > > > It is going to come from the device memory that does not have hard > requirements of MMIO accessibility. > For example slow flash memory. > > > Asynchronous is just harder for software. Look at the hoops virtio net has > > to > > jump through to do control path over cvq and weep. Compare to single liner > > virtio_cread. > > > > I happen to maintain Linux drivers too, so I care. > > > It is harder for first time if no such construct exists; and it is clearly > not the case. > This patch is already adding CVQ get and set command. I expect get to mostly go unused. > Async behavior already built into the virtio spec. > Extending it for extended config space just make device more cheaper to build. > Software cost is paid only once at device init time just like CVQ, AQ and > more. > > > > > > > > > Please let people just focus on fixing config space instead of > > > > temporary cvq hacks. > > > > > > The ask is to have predictable sizing for existing defined config space > > > field and > > not keep infinitely growing by two interfaces. > > > > I don't know what "predictable sizing" is or why does it matter. > > > Because when device has two ways to access config space, it always have to > account and build the interface that, hey some driver will not use DMA. > Hence have it always in the MMIO accessible area. If we say that drivers should use DMA, they will. If we additionally explain that some features might not be in MMIO no sane driver will use MMIO if it does not have to. Or if it does then it has violated the spec and will get less features? > > I get the value of reduced MMIO register map. > > I get that the only way to get that appears to be to pass values around > > using > > DMA. > > > Ok. great. > > > I don't think we need to throw away config space - just use DMA to access > > it. > > > > And "existing defined" here seems to be at a completely random point in > > time. > > If someone wants to have fields in MMIO, I see no reason not to. > >
[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, June 28, 2023 12:46 PM > Actually, my preblem is exactly if there's no one single interface that can do > everything. > Extended config space reading via single DMA interface is just enough. > > Two interfaces that do the same might seem redundant but there could be good > reasons for this, in this case legacy. > But legacy should not force it to newer interface. More below. > See the difference? > > > I didn't propose two interfaces. Let me explain again. > > > > I proposed, > > a. all new fields to go via vq (single interface) b. all existing > > fields to stay in cfg space (single interface) c. if one wants to > > optimize optionally existing fields can utilize (same flexibility how you > > asked > for PF and VF notification). > > > > > > > We already need config space with provisioning. > > Provisioning does provision features, config space and control parameters, > msix vectors and more. > > Provisioning does _not_ need config space. > > I mean it needs to provision it. > No matter there is config space of dma, a device to be provisioned. > > > In fact e.g. on Linux we want drivers to keep doing virtio_cread() > > > and all the DMA tricks should be hidden behind the scenes. You don't > > > like it that config space is on-chip? Build an interface for it not > > > to be on chip. There is simply no downside to that. > > > > > This is the ask to build software interface between driver and device to not > demand on-chip config space more importantly _predictably_. > > Absolutely. So we would say that driver SHOULD use the DMA interface if it is > present. > I am asking for driver MUST use the DMA interface for _extended_ config space. Why? For two reasons. 1. Because than device is 100% sure that it does not fulfill MMIO needs 2. Single interface to access extended config space, which is what you asked for. 3. Single driver code because there is single way to get it. > > > I came back after a small break and I still see this discussion that > > > keeps playing out as a big distraction. > > > In particular where are you doing to store the things that are for DMA? > > > In system RAM? We don't *have* anywhere in system RAM to store this, > > > we will need an interface to allocate system RAM and pass it to > > > device for this idea to work. So in 1.3 where we won't have this, > > > there is much less of an excuse to use a vq. > > To store what? > > CVQ is already there to do GET and SET operation. > > To store whatever value CVQ is requesting. Where is it going to come from? > It is going to come from the device memory that does not have hard requirements of MMIO accessibility. For example slow flash memory. > Asynchronous is just harder for software. Look at the hoops virtio net has to > jump through to do control path over cvq and weep. Compare to single liner > virtio_cread. > > I happen to maintain Linux drivers too, so I care. > It is harder for first time if no such construct exists; and it is clearly not the case. This patch is already adding CVQ get and set command. Async behavior already built into the virtio spec. Extending it for extended config space just make device more cheaper to build. Software cost is paid only once at device init time just like CVQ, AQ and more. > > > > > > Please let people just focus on fixing config space instead of > > > temporary cvq hacks. > > > > The ask is to have predictable sizing for existing defined config space > > field and > not keep infinitely growing by two interfaces. > > I don't know what "predictable sizing" is or why does it matter. > Because when device has two ways to access config space, it always have to account and build the interface that, hey some driver will not use DMA. Hence have it always in the MMIO accessible area. > I get the value of reduced MMIO register map. > I get that the only way to get that appears to be to pass values around using > DMA. > Ok. great. > I don't think we need to throw away config space - just use DMA to access it. > > And "existing defined" here seems to be at a completely random point in time. > If someone wants to have fields in MMIO, I see no reason not to. This demands two ways of access and that conflicts with your point of desire to do single way. I proposed single way, extended config space via DMA interface, that is really as simple as that. - 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 v19] virtio-net: support inner header hash
On Wed, Jun 28, 2023 at 04:18:02PM +, Parav Pandit wrote: > > > > From: Michael S. Tsirkin > > Sent: Wednesday, June 28, 2023 6:27 AM > > > > On Wed, Jun 28, 2023 at 04:23:09AM +, Parav Pandit wrote: > > > > > > > From: Jason Wang > > > > Sent: Tuesday, June 27, 2023 11:46 PM > > > > > > > > Having it in the config space, then a type agnostic provisioning > > > > through config space + feature bits just works fine. > > > > > > > Provisioning is far simpler thing to do in device specific way than > > > asking device > > to store this value in onchip area which is rarely accessed. > > > > I thought a lot about this over the weekend. I just do not see this working > > out, > > sorry. Two things is never easier than one. > > You proposed two things and making it mandatory. > I am glad that you realized to do using one interface. > > In different context, you proposed two interface for driver notification on > PF and/or VF BAR in separate thread for flexibility. I asked for one. > Somehow you promoted flexibility (and also complexity that no vendor wanted), > and here you lean towards single interface... > > So more explanation of the rationale will help to understand. OK I will try. Actually, my preblem is exactly if there's no one single interface that can do everything. Two interfaces that do the same might seem redundant but there could be good reasons for this, in this case legacy. See the difference? > I didn't propose two interfaces. Let me explain again. > > I proposed, > a. all new fields to go via vq (single interface) > b. all existing fields to stay in cfg space (single interface) > c. if one wants to optimize optionally existing fields can utilize (same > flexibility how you asked for PF and VF notification). > > > > We already need config space with provisioning. > Provisioning does provision features, config space and control parameters, > msix vectors and more. > Provisioning does _not_ need config space. I mean it needs to provision it. > > In fact e.g. on Linux we want drivers to keep doing virtio_cread() > > and all the DMA tricks should be hidden behind the scenes. You don't like > > it that > > config space is on-chip? Build an interface for it not to be on chip. There > > is > > simply no downside to that. > > > This is the ask to build software interface between driver and device to not > demand on-chip config space more importantly _predictably_. Absolutely. So we would say that driver SHOULD use the DMA interface if it is present. > > I came back after a small break and I still see this discussion that keeps > > playing > > out as a big distraction. > > In particular where are you doing to store the things that are for DMA? > > In system RAM? We don't *have* anywhere in system RAM to store this, we will > > need an interface to allocate system RAM and pass it to device for this > > idea to > > work. So in 1.3 where we won't have this, there is much less of an excuse > > to use > > a vq. > To store what? > CVQ is already there to do GET and SET operation. To store whatever value CVQ is requesting. Where is it going to come from? > > > > > > > > > > > There are many fields of many features for 1.4 are discussed including > > > this > > one. All of these capabilities just cannot be stored in config space. > > > > config space is synchronous interface, vq is asynchronous. Setup is just > > too > > messy to do asynchronously. So yes, config space. > > > Huh, I disagree. The whole virtio spec has got the VQs all over and you say > that VQ is messy. > Doesn't make any sense at all. Asynchronous is just harder for software. Look at the hoops virtio net has to jump through to do control path over cvq and weep. Compare to single liner virtio_cread. I happen to maintain Linux drivers too, so I care. > > > > Please let people just focus on fixing config space instead of temporary cvq > > hacks. > > The ask is to have predictable sizing for existing defined config space field > and not keep infinitely growing by two interfaces. I don't know what "predictable sizing" is or why does it matter. I get the value of reduced MMIO register map. I get that the only way to get that appears to be to pass values around using DMA. I don't think we need to throw away config space - just use DMA to access it. And "existing defined" here seems to be at a completely random point in time. If someone wants to have fields in MMIO, I see no reason not to. -- 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 v19] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, June 28, 2023 6:27 AM > > On Wed, Jun 28, 2023 at 04:23:09AM +, Parav Pandit wrote: > > > > > From: Jason Wang > > > Sent: Tuesday, June 27, 2023 11:46 PM > > > > > > Having it in the config space, then a type agnostic provisioning > > > through config space + feature bits just works fine. > > > > > Provisioning is far simpler thing to do in device specific way than asking > > device > to store this value in onchip area which is rarely accessed. > > I thought a lot about this over the weekend. I just do not see this working > out, > sorry. Two things is never easier than one. You proposed two things and making it mandatory. I am glad that you realized to do using one interface. In different context, you proposed two interface for driver notification on PF and/or VF BAR in separate thread for flexibility. I asked for one. Somehow you promoted flexibility (and also complexity that no vendor wanted), and here you lean towards single interface... So more explanation of the rationale will help to understand. I didn't propose two interfaces. Let me explain again. I proposed, a. all new fields to go via vq (single interface) b. all existing fields to stay in cfg space (single interface) c. if one wants to optimize optionally existing fields can utilize (same flexibility how you asked for PF and VF notification). > We already need config space with provisioning. Provisioning does provision features, config space and control parameters, msix vectors and more. Provisioning does _not_ need config space. > In fact e.g. on Linux we want drivers to keep doing virtio_cread() > and all the DMA tricks should be hidden behind the scenes. You don't like it > that > config space is on-chip? Build an interface for it not to be on chip. There is > simply no downside to that. > This is the ask to build software interface between driver and device to not demand on-chip config space more importantly _predictably_. > I came back after a small break and I still see this discussion that keeps > playing > out as a big distraction. > In particular where are you doing to store the things that are for DMA? > In system RAM? We don't *have* anywhere in system RAM to store this, we will > need an interface to allocate system RAM and pass it to device for this idea > to > work. So in 1.3 where we won't have this, there is much less of an excuse to > use > a vq. To store what? CVQ is already there to do GET and SET operation. > > > > > > There are many fields of many features for 1.4 are discussed including this > one. All of these capabilities just cannot be stored in config space. > > config space is synchronous interface, vq is asynchronous. Setup is just too > messy to do asynchronously. So yes, config space. > Huh, I disagree. The whole virtio spec has got the VQs all over and you say that VQ is messy. Doesn't make any sense at all. > > Please let people just focus on fixing config space instead of temporary cvq > hacks. The ask is to have predictable sizing for existing defined config space field and not keep infinitely growing by two interfaces. - 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 v19] virtio-net: support inner header hash
On Wed, Jun 28, 2023 at 04:23:09AM +, Parav Pandit wrote: > > > From: Jason Wang > > Sent: Tuesday, June 27, 2023 11:46 PM > > > > Having it in the config space, then a type agnostic provisioning through > > config > > space + feature bits just works fine. > > > Provisioning is far simpler thing to do in device specific way than asking > device to store this value in onchip area which is rarely accessed. I thought a lot about this over the weekend. I just do not see this working out, sorry. Two things is never easier than one. We already need config space with provisioning. In fact e.g. on Linux we want drivers to keep doing virtio_cread() and all the DMA tricks should be hidden behind the scenes. You don't like it that config space is on-chip? Build an interface for it not to be on chip. There is simply no downside to that. I came back after a small break and I still see this discussion that keeps playing out as a big distraction. In particular where are you doing to store the things that are for DMA? In system RAM? We don't *have* anywhere in system RAM to store this, we will need an interface to allocate system RAM and pass it to device for this idea to work. So in 1.3 where we won't have this, there is much less of an excuse to use a vq. > There are many fields of many features for 1.4 are discussed including this > one. All of these capabilities just cannot be stored in config space. config space is synchronous interface, vq is asynchronous. Setup is just too messy to do asynchronously. So yes, config space. Please let people just focus on fixing config space instead of temporary cvq hacks. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
