[virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

If the VIRTIO_NET_F_HASH_REPORT feature is negotiated, the driver may
send VIRTIO_NET_CTRL_MQ_HASH_CONFIG commands, thus, the control VQ
feature should be negotiated.

---
v2: Use SHOULD instead of Feature bit requirement, version 1.2 is already
    out and doesn't include this depencency.

v3: Explain the dependency in a less confusing way.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>

 device-types/net/description.tex | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 700a1cb..1741c79 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -256,6 +256,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
 \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
 the \field{status}.
 
+The device SHOULD NOT offer VIRTIO_NET_F_HASH_REPORT if it
+does not offer VIRTIO_NET_F_CTRL_VQ.
+
 \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
 
 A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
@@ -289,6 +292,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
 SHOULD re-read \field{speed} and \field{duplex} after a
 configuration change notification.
 
+A driver SHOULD NOT negotiate VIRTIO_NET_F_HASH_REPORT if it
+does not negotiate VIRTIO_NET_F_CTRL_VQ.
+
 \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
 \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
 When using the legacy interface, transitional devices and drivers
-- 
2.34.1


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

[virtio-comment] RE: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Alvaro Karsz
> ---
> v2: Use SHOULD instead of Feature bit requirement, version 1.2 is already
>     out and doesn't include this depencency.
> 
> v3: Explain the dependency in a less confusing way.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> 
>  device-types/net/description.tex | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 700a1cb..1741c79 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -256,6 +256,9 @@ \subsection{Device configuration
> layout}\label{sec:Device Types / Network Device  \field{duplex} fields as long as
> VIRTIO_NET_S_LINK_UP is set in  the \field{status}.
> 
> +The device SHOULD NOT offer VIRTIO_NET_F_HASH_REPORT if it does not
> +offer VIRTIO_NET_F_CTRL_VQ.
> +
I believe this applies to all the configurations done through the control VQ.
So we should write a generic description instead of per feature statement like above.

>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types /
> Network Device / Device configuration layout}
> 
>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -289,6 +292,9 @@ \subsection{Device configuration
> layout}\label{sec:Device Types / Network Device  SHOULD re-read \field{speed}
> and \field{duplex} after a  configuration change notification.
> 
> +A driver SHOULD NOT negotiate VIRTIO_NET_F_HASH_REPORT if it does not
> +negotiate VIRTIO_NET_F_CTRL_VQ.
> +
Same for the driver too.
Like below.
The driver should not negotiate a feature that requires to use a control VQ when VIRTIO_NET_F_CTRL_VQ is not negotiated.

Or driver must not .. ..
Exception to that is VIRTIO_NET_F_HASH_REPORT, which driver should avoid negotiation ...

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

Hi Parav,

> > +A driver SHOULD NOT negotiate VIRTIO_NET_F_HASH_REPORT if it does not
> > +negotiate VIRTIO_NET_F_CTRL_VQ.
> > +
> Same for the driver too.
> Like below.
> The driver should not negotiate a feature that requires to use a control VQ when VIRTIO_NET_F_CTRL_VQ is not negotiated.

The dependency of VIRTIO_NET_F_HASH_REPORT on VIRTIO_NET_F_CTRL_VQ is
not mentioned in the spec (directly at least).
So I'm not so sure about this line.

All the dependencies are listed in the "Feature bit requirements" section.
VIRTIO_NET_F_HASH_REPORT is missing, but since 1.2 is out, we can't
add it now as a requirement [1] [2].

[1] https://github.com/oasis-tcs/virtio-spec/issues/158
[2] https://lists.oasis-open.org/archives/virtio-comment/202302/msg00027.html

> Or driver must not .. ..
> Exception to that is VIRTIO_NET_F_HASH_REPORT, which driver should avoid negotiation ...

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

Re: [virtio-comment] RE: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Michael S. Tsirkin]

On Mon, Feb 06, 2023 at 10:02:39PM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> > Behalf Of Alvaro Karsz
> > ---
> > v2: Use SHOULD instead of Feature bit requirement, version 1.2 is already
> >     out and doesn't include this depencency.
> > 
> > v3: Explain the dependency in a less confusing way.
> > 
> > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > 
> >  device-types/net/description.tex | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index 700a1cb..1741c79 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -256,6 +256,9 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device  \field{duplex} fields as long as
> > VIRTIO_NET_S_LINK_UP is set in  the \field{status}.
> > 
> > +The device SHOULD NOT offer VIRTIO_NET_F_HASH_REPORT if it does not
> > +offer VIRTIO_NET_F_CTRL_VQ.
> > +
> I believe this applies to all the configurations done through the control VQ.
> So we should write a generic description instead of per feature statement like above.


I think we mostly have dependencies listed already. Dependencies
are a MUST though. It's a pity we forgot this one but oh well.
*Maybe* we want a section of a kind of "weak dependencies" for
cases like this where it's a SHOULD not a MUST.


> >  \drivernormative{\subsubsection}{Device configuration layout}{Device Types /
> > Network Device / Device configuration layout}
> > 
> >  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > @@ -289,6 +292,9 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device  SHOULD re-read \field{speed}
> > and \field{duplex} after a  configuration change notification.
> > 
> > +A driver SHOULD NOT negotiate VIRTIO_NET_F_HASH_REPORT if it does not
> > +negotiate VIRTIO_NET_F_CTRL_VQ.
> > +
> Same for the driver too.
> Like below.
> The driver should not negotiate a feature that requires to use a control VQ when VIRTIO_NET_F_CTRL_VQ is not negotiated.
> Or driver must not .. ..
> Exception to that is VIRTIO_NET_F_HASH_REPORT, which driver should avoid negotiation ...

That's a vague enough not to be useful. VIRTIO_NET_F_HASH_REPORT
is not 100% through a queue, there's a list of supported hashes
that can be checked through config space.  Of limited usefulness
but still, e.g. you can know what's supported e.g. for management
purposes.



-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

Do you want a new version?
This one seems fine to me.

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

Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

Should we have a vote?

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/158

RE: [virtio-dev] Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Alvaro Karsz
> 
> Should we have a vote?
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/158 

I missed your previous response.
Sorry for the late response.

Why cannot device say as MUST requirement?

Let say there is a device out that exposes a F_HASH_REPORT without F_CTRL_VQ.
So driver tried to send a command and failed to issue cmd.
End result, hash config was not successful.
Did driver gain anything from seeing supplied hash in the config space?
No. it just confused driver that device offered feature bit, could see the supposed hash, but still couldn't configure it.

So, I think the right fix is that device MUST NOT set F_HASH_REPORT without F_CTRL_VQ.

And driver should .. is fine, because existing driver that followed 1.2 will negotiate, and device will also accept it if it offered without F_CTRL_VQ.

Any new device that follows 1.3 will not offer F_HAS_REPORT without C_VQ, hence it cannot be negotiated by driver without C_VQ.

The gain is : device doesn't need to continue offering F_HASH_REPORT without C_VQ. And I doubt if any device would have done it, as it was obvious.
Just the spec was missed out.

Is MUST/SHOULD big deal here for device? At least not to me, from practical standpoint to me.
Making must just makes the spec consistent with rest without breaking backward compat.

Re: [virtio-dev] Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

> I missed your previous response.
> Sorry for the late response.

No problem at all :)

> Why cannot device say as MUST requirement?
>
> Let say there is a device out that exposes a F_HASH_REPORT without F_CTRL_VQ.
> So driver tried to send a command and failed to issue cmd.
> End result, hash config was not successful.
> Did driver gain anything from seeing supplied hash in the config space?
> No. it just confused driver that device offered feature bit, could see the supposed hash, but still couldn't configure it.
>
> So, I think the right fix is that device MUST NOT set F_HASH_REPORT without F_CTRL_VQ.
>
> And driver should .. is fine, because existing driver that followed 1.2 will negotiate, and device will also accept it if it offered without F_CTRL_VQ.
>
> Any new device that follows 1.3 will not offer F_HAS_REPORT without C_VQ, hence it cannot be negotiated by driver without C_VQ.
>
> The gain is : device doesn't need to continue offering F_HASH_REPORT without C_VQ. And I doubt if any device would have done it, as it was obvious.
> Just the spec was missed out.
>
> Is MUST/SHOULD big deal here for device? At least not to me, from practical standpoint to me.
> Making must just makes the spec consistent with rest without breaking backward compat.

The first version of this patch [1] actually used a bit requirement
(which is equivalent to a "MUST" if I understand correctly).
Then Michael pointed out that we can't do it since a version was
published without this requirement.

It's clear that the control VQ is required to use this feature.
In the linux kernel implementation probe will fail if a device offers
F_HASH_REPORT without F_CTRL_VQ.

Ideally we'll add a "MUST", but since we can't, our options are to
have a "SHOULD", or not mention the dependency at all.

[1] https://lists.oasis-open.org/archives/virtio-comment/202302/msg00026.html

RE: [virtio-dev] Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Saturday, February 11, 2023 4:13 AM


> > Why cannot device say as MUST requirement?
> >
> > Let say there is a device out that exposes a F_HASH_REPORT without
> F_CTRL_VQ.
> > So driver tried to send a command and failed to issue cmd.
> > End result, hash config was not successful.
> > Did driver gain anything from seeing supplied hash in the config space?
> > No. it just confused driver that device offered feature bit, could see the
> supposed hash, but still couldn't configure it.
> >
> > So, I think the right fix is that device MUST NOT set F_HASH_REPORT without
> F_CTRL_VQ.
> >
> > And driver should .. is fine, because existing driver that followed 1.2 will
> negotiate, and device will also accept it if it offered without F_CTRL_VQ.
> >
> > Any new device that follows 1.3 will not offer F_HAS_REPORT without C_VQ,
> hence it cannot be negotiated by driver without C_VQ.
> >
> > The gain is : device doesn't need to continue offering F_HASH_REPORT
> without C_VQ. And I doubt if any device would have done it, as it was obvious.
> > Just the spec was missed out.
> >
> > Is MUST/SHOULD big deal here for device? At least not to me, from practical
> standpoint to me.
> > Making must just makes the spec consistent with rest without breaking
> backward compat.
> 
> The first version of this patch [1] actually used a bit requirement (which is
> equivalent to a "MUST" if I understand correctly).
> Then Michael pointed out that we can't do it since a version was published
> without this requirement.
> 
> It's clear that the control VQ is required to use this feature.
> In the linux kernel implementation probe will fail if a device offers
> F_HASH_REPORT without F_CTRL_VQ.
> 
This is even better.
> Ideally we'll add a "MUST", but since we can't, 
Lets hear Michael's view, why MUST cannot be done.
Based on our discussion here, I think MUST is possible and cleaner without breaking any existing sw or device.

> our options are to have a
> "SHOULD", or not mention the dependency at all.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202302/msg00026.html

Re: [virtio-dev] Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Michael S. Tsirkin]

On Sat, Feb 11, 2023 at 02:03:01PM +0000, Parav Pandit wrote:
> 
> 
> > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Sent: Saturday, February 11, 2023 4:13 AM
> 
> 
> > > Why cannot device say as MUST requirement?
> > >
> > > Let say there is a device out that exposes a F_HASH_REPORT without
> > F_CTRL_VQ.
> > > So driver tried to send a command and failed to issue cmd.
> > > End result, hash config was not successful.
> > > Did driver gain anything from seeing supplied hash in the config space?
> > > No. it just confused driver that device offered feature bit, could see the
> > supposed hash, but still couldn't configure it.
> > >
> > > So, I think the right fix is that device MUST NOT set F_HASH_REPORT without
> > F_CTRL_VQ.
> > >
> > > And driver should .. is fine, because existing driver that followed 1.2 will
> > negotiate, and device will also accept it if it offered without F_CTRL_VQ.
> > >
> > > Any new device that follows 1.3 will not offer F_HAS_REPORT without C_VQ,
> > hence it cannot be negotiated by driver without C_VQ.
> > >
> > > The gain is : device doesn't need to continue offering F_HASH_REPORT
> > without C_VQ. And I doubt if any device would have done it, as it was obvious.
> > > Just the spec was missed out.
> > >
> > > Is MUST/SHOULD big deal here for device? At least not to me, from practical
> > standpoint to me.
> > > Making must just makes the spec consistent with rest without breaking
> > backward compat.
> > 
> > The first version of this patch [1] actually used a bit requirement (which is
> > equivalent to a "MUST" if I understand correctly).
> > Then Michael pointed out that we can't do it since a version was published
> > without this requirement.
> > 
> > It's clear that the control VQ is required to use this feature.
> > In the linux kernel implementation probe will fail if a device offers
> > F_HASH_REPORT without F_CTRL_VQ.
> > 
> This is even better.
> > Ideally we'll add a "MUST", but since we can't, 
> Lets hear Michael's view, why MUST cannot be done.
> Based on our discussion here, I think MUST is possible and cleaner without breaking any existing sw or device.

1.2 is out without this requirement. Making this a MUST at this
point would declare such previously conformant devices non-conformant.
So I'm afraid our hands are tied.

It might be a good idea to start building out a charter documenting
all kind of compat hacks like this such that new devices are
not tempted to do the wrong thing. I am not sure how this
will look exactly though.


> > our options are to have a
> > "SHOULD", or not mention the dependency at all.
> > 
> > [1] https://lists.oasis-open.org/archives/virtio-comment/202302/msg00026.html

[virtio-comment] RE: [virtio-dev] Re: [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, February 12, 2023 4:39 AM

[..]
> > This is even better.
> > > Ideally we'll add a "MUST", but since we can't,
> > Lets hear Michael's view, why MUST cannot be done.
> > Based on our discussion here, I think MUST is possible and cleaner without
> breaking any existing sw or device.
> 
> 1.2 is out without this requirement. Making this a MUST at this point would
> declare such previously conformant devices non-conformant.
> So I'm afraid our hands are tied.
> 
Technically yes, I agree it make non conformant.
The device that offered HASH_REPORT without offering CVQ, is extremely rare/narrow case.
I am not sure if anyone would have ever built such a thing just because such description was missing from the spec.
So, I am inclined towards a practical part than purely technical.

But I can live with SHOULD here if you want to stick to strict compliance here.

> It might be a good idea to start building out a charter documenting all kind of
> compat hacks like this such that new devices are not tempted to do the wrong
> thing. I am not sure how this will look exactly though.
> 
We should write a line along with device requirements something like below.

Even though CVQ is not mandatory for HASH_REPORT, it is strong advised the device to NOT report this feature when CTRL_VQ is not advertised.

Once there is better section, more generic table etc can be created.
Since you created the ballot already, I will supply the short patch to add above description later.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Cornelia Huck]

On Mon, Feb 06 2023, Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:

> If the VIRTIO_NET_F_HASH_REPORT feature is negotiated, the driver may
> send VIRTIO_NET_CTRL_MQ_HASH_CONFIG commands, thus, the control VQ
> feature should be negotiated.
>
> ---
> v2: Use SHOULD instead of Feature bit requirement, version 1.2 is already
>     out and doesn't include this depencency.
>
> v3: Explain the dependency in a less confusing way.
>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>

Minor nit (for the next time, was trivial to fix up): Putting your s-o-b
beneath the "---" instructs git am to cut it off; the changelog should
go below the s-o-b.

>
>  device-types/net/description.tex | 6 ++++++
>  1 file changed, 6 insertions(+)


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

Re: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ

[Alvaro Karsz]

> Minor nit (for the next time, was trivial to fix up): Putting your s-o-b
> beneath the "---" instructs git am to cut it off; the changelog should
> go below the s-o-b.

You're right, sorry, I missed it.