[virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

The filtering behavior of the VLAN filter commands is not very clear as
discussed in thread [1].

Hence, add the command description and device requirements for it.

[1] https://lists.oasis-open.org/archives/virtio-dev/202301/msg00210.html

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/147
Suggested-by: Si-Wei Liu <si-wei.liu@oracle.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v9->v10:
- rebased to new directory name
v8->v9:
- rebased on top of [1]
- updated commit message to newer link
v7->v8:
- Fixed grammar
v6->v7:
- Moved VLAN filter table description from requirements to device
  descrption section
- Added MUST and SHOULD to device requirements
v5->v6:
- removed unwanted article
v4->v5:
- reworded 'vlan filtering table' to 'vlan filter table' to match
  to the existing description about vlan filtering
- remove confusing text around VLAN_DEL command description
- added missing article
- reword device match configuration to device configuration
- changed 'found' to 'present' and 'not found' to 'absent' to
  consider vlan filter table as config table rather
  than search table
v3->v4:
- added description for accepting vlan tagged packets when vlan
  filter is not negotiated
v2->v3:
- corrected grammar
- simplified description for untagged packets
v1->v2:
- adapt to new file path
v0->v1:
- added missing conformance section link
---
 device-types/net/description.tex        | 22 +++++++++++++++++++++-
 device-types/net/device-conformance.tex |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 7409f2a..970090c 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1194,7 +1194,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \paragraph{VLAN Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
 
 If the driver negotiates the VIRTIO_NET_F_CTRL_VLAN feature, it
-can control a VLAN filter table in the device.
+can control a VLAN filter table in the device. The VLAN filter
+table applies only to VLAN tagged packets.
+
+When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
+an empty VLAN filter table.
 
 \begin{note}
 Similar to the MAC address based filtering, the VLAN filtering
@@ -1210,6 +1214,22 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 Both the VIRTIO_NET_CTRL_VLAN_ADD and VIRTIO_NET_CTRL_VLAN_DEL
 command take a little-endian 16-bit VLAN id as the command-specific-data.
 
+VIRTIO_NET_CTRL_VLAN_ADD command adds the specified VLAN to the
+VLAN filter table.
+
+VIRTIO_NET_CTRL_VLAN_DEL command removes the specified VLAN from
+the VLAN filter table.
+
+\devicenormative{\subparagraph}{VLAN Filtering}{Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
+
+When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST
+accept all VLAN tagged packets as per the device configuration.
+
+When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST
+accept all VLAN tagged packets whose VLAN tag is present in
+the VLAN filter table and SHOULD drop all VLAN tagged packets
+whose VLAN tag is absent in the VLAN filter table.
+
 \subparagraph{Legacy Interface: VLAN Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering / Legacy Interface: VLAN Filtering}
 When using the legacy interface, transitional devices and drivers
 MUST format the VLAN id
diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
index c686377..54f6783 100644
--- a/device-types/net/device-conformance.tex
+++ b/device-types/net/device-conformance.tex
@@ -9,6 +9,7 @@
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Processing of Incoming Packets}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Setting MAC Address Filtering}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Gratuitous Packet Sending}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
-- 
2.26.2


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-comment] RE: [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Parav Pandit <parav@nvidia.com>
> Sent: Thursday, January 12, 2023 4:26 PM
> 
> The filtering behavior of the VLAN filter commands is not very clear as
> discussed in thread [1].
> 
> Hence, add the command description and device requirements for it.
> 
> [1] https://lists.oasis-open.org/archives/virtio-dev/202301/msg00210.html
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/147
> Suggested-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Can we please start the ballot for this documentation update patch?
The linked github issue is: https://github.com/oasis-tcs/virtio-spec/issues/147


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-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Halil Pasic]

On Thu, 12 Jan 2023 23:25:50 +0200
Parav Pandit <parav@nvidia.com> wrote:

> The filtering behavior of the VLAN filter commands is not very clear as
> discussed in thread [1].
> 
> Hence, add the command description and device requirements for it.
> 
> [1] https://lists.oasis-open.org/archives/virtio-dev/202301/msg00210.html

Reference is wrong. This links an unrelated thread.

For reference see:

https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09424.html

[..]
@ -1194,7 +1194,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device
Types / Network Device / Devi
>  \paragraph{VLAN Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
>  
>  If the driver negotiates the VIRTIO_NET_F_CTRL_VLAN feature, it
> -can control a VLAN filter table in the device.
> +can control a VLAN filter table in the device. The VLAN filter
> +table applies only to VLAN tagged packets.
> +
> +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
> +an empty VLAN filter table.
>  
>  \begin{note}
>  Similar to the MAC address based filtering, the VLAN filtering
> @@ -1210,6 +1214,22 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  Both the VIRTIO_NET_CTRL_VLAN_ADD and VIRTIO_NET_CTRL_VLAN_DEL
>  command take a little-endian 16-bit VLAN id as the command-specific-data.
>  
[..]
> +
> +\devicenormative{\subparagraph}{VLAN Filtering}{Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
> +
> +When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST
> +accept all VLAN tagged packets as per the device configuration.


The story so far (other thread):

> > I find "as per the device configuration" difficult to comprehend. Maybe we can
> > work on this with an editorial. What are you trying to express here? On one
> > hand you say "the device MUST accept all VLAN tagged packets" on the other
> > hand "per the device configuration" may explain, or may relativize and restrict
> > that statement.
> >   
> From VLAN filtering perspective, it should accept all packets, but this is subject to other device configuration such as MAC programming.
> The current one is fine that cover both the aspects to VLAN specific and other config.

IMHO you need to be explicit about this "From the VLAN filtering
perspective". A sub-case of "not negotiated" is "not offered because the
device does not support it" and in that case there is conceptually
no "VLAN filtering perspective".

> > Would
> > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, VLAN filtering is not
> > applied. I.e. VLAN tags are ignored by the device."
> > also work?
> >  
> No. its not about ignoring "VLAN tags". Its about dealing with "packets" that starts with the VLAN tag.

I'm not sure I understand the difference.

Would just 
"When VIRTIO_NET_F_CTRL_VLAN is not negotiated, no VLAN filtering is
done by the device."
work?

I don't like this "as per device configuration" formulation. If you
have multiple filtering mechanisms, like VLAN and MAC, and describe
all of these using this "as per device configuration", you would
end up with statements like:
"When feature F_X is not negotiated, the device MUST accept all
P_X packets as the per device configuration."
Where as per device configuration is supposed to cover all other
active filtering mechanisms except the one that is tied to F_X, but
exclude the mechanism that is tied to the feature F_X.

I'm also not convinced about the information content of "as per
device configuration". Kind of the complementer would be "contrary
to the device configuration" which does not make much sense. I would
also argue that we can add "as per device configuration" to most
of our  device normative statements. It would IMHO only decrease
clarity and increase the confusion. But if you ask yourself
is the behavior prescribed by an arbitrary device normative statemen "as
per device configuration" or not, I don't think one would ever answer
"no" to that question.

> +
> +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST
> +accept all VLAN tagged packets whose VLAN tag is present in
> +the VLAN filter table and SHOULD drop all VLAN tagged packets
> +whose VLAN tag is absent in the VLAN filter table.

We could also add "as per device configuration" here as well...

From the other thread

On Mon, 23 Jan 2023 12:41:16 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > BTW why SHOULD drop?  
> This is the offload feature to drop such packets so that OS driver doesn't need to do the filtering work. :)

Why not MUST drop? If the device is allowed to produce false negatives,
then I think the OS probably wants to check again to filter out those.
Can we change SHOULD to MUST, so the conforming device is guaranteed to
do the filtering, and the OS can rely on it?

Regards,
Halil 


> +
>  \subparagraph{Legacy Interface: VLAN Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering / Legacy Interface: VLAN Filtering}
>  When using the legacy interface, transitional devices and drivers
>  MUST format the VLAN id
> diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
> index c686377..54f6783 100644
> --- a/device-types/net/device-conformance.tex
> +++ b/device-types/net/device-conformance.tex
> @@ -9,6 +9,7 @@
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Processing of Incoming Packets}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Setting MAC Address Filtering}
> +\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / VLAN Filtering}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Gratuitous Packet Sending}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS processing}


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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Wednesday, January 25, 2023 1:05 PM
> 
> On Thu, 12 Jan 2023 23:25:50 +0200
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > The filtering behavior of the VLAN filter commands is not very clear
> > as discussed in thread [1].
> >
> > Hence, add the command description and device requirements for it.
> >
> > [1]
> > https://lists.oasis-open.org/archives/virtio-dev/202301/msg00210.html
> 
> Reference is wrong. This links an unrelated thread.
> 
> For reference see:
> 
> https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09424.html
>
In the commit log the right reference to the discussion should be,

https://www.mail-archive.com/qemu-devel@nongnu.org/msg912392.html

More below whether we need v11 or not.

> [..]
> @ -1194,7 +1194,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> Types / Network Device / Devi
> >  \paragraph{VLAN Filtering}\label{sec:Device Types / Network Device /
> > Device Operation / Control Virtqueue / VLAN Filtering}
> >
> >  If the driver negotiates the VIRTIO_NET_F_CTRL_VLAN feature, it -can
> > control a VLAN filter table in the device.
> > +can control a VLAN filter table in the device. The VLAN filter table
> > +applies only to VLAN tagged packets.
> > +
> > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with an
> > +empty VLAN filter table.
> >
> >  \begin{note}
> >  Similar to the MAC address based filtering, the VLAN filtering @@
> > -1210,6 +1214,22 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi  Both the VIRTIO_NET_CTRL_VLAN_ADD and
> > VIRTIO_NET_CTRL_VLAN_DEL  command take a little-endian 16-bit VLAN id as
> the command-specific-data.
> >
> [..]
> > +
> > +\devicenormative{\subparagraph}{VLAN Filtering}{Device Types /
> > +Network Device / Device Operation / Control Virtqueue / VLAN
> > +Filtering}
> > +
> > +When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept
> > +all VLAN tagged packets as per the device configuration.
> 
> 
> The story so far (other thread):
> 
> > > I find "as per the device configuration" difficult to comprehend.
> > > Maybe we can work on this with an editorial. What are you trying to
> > > express here? On one hand you say "the device MUST accept all VLAN
> > > tagged packets" on the other hand "per the device configuration" may
> > > explain, or may relativize and restrict that statement.
> > >
> > From VLAN filtering perspective, it should accept all packets, but this is
> subject to other device configuration such as MAC programming.
> > The current one is fine that cover both the aspects to VLAN specific and other
> config.
> 
> IMHO you need to be explicit about this "From the VLAN filtering perspective".
> A sub-case of "not negotiated" is "not offered because the device does not
> support it" and in that case there is conceptually no "VLAN filtering
> perspective".
> 
Not negotiated covers two conditions.
1. device offered, but driver choose to keep it disabled.
2. device didn't offer, so driver didn't enable it

This section talks about VLAN filtering, so context is VLAN filtering and no need to bring other context.
I tend to agree that we can drop the line "per device configuration".

> > > Would
> > > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, VLAN filtering is
> > > not applied. I.e. VLAN tags are ignored by the device."
> > > also work?
> > >
> > No. its not about ignoring "VLAN tags". Its about dealing with "packets" that
> starts with the VLAN tag.
> 
> I'm not sure I understand the difference.
> 
> Would just
> "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, no VLAN filtering is done
> by the device."
> work?
>
This is also fine.
However, I prefer the current spec terminology used in mac, promiscuous etc area such as "receive/accept/drop".
It is lot clearer to understand in steering world.
And it also aligns to the steering tools such as tc [3] which defines the action as "drop/mirred/redirect" etc.

Hence, lets keep
"When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept all VLAN tagged packets"

This way spec language is same for negotiated/non-negotiated case of accept/drop etc.

> I don't like this "as per device configuration" formulation. If you have multiple
> filtering mechanisms, like VLAN and MAC, and describe all of these using this
> "as per device configuration", you would end up with statements like:
> "When feature F_X is not negotiated, the device MUST accept all P_X packets as
> the per device configuration."
> Where as per device configuration is supposed to cover all other active filtering
> mechanisms except the one that is tied to F_X, but exclude the mechanism that
> is tied to the feature F_X.
> 
> I'm also not convinced about the information content of "as per device
> configuration". Kind of the complementer would be "contrary to the device
> configuration" which does not make much sense. I would also argue that we
> can add "as per device configuration" to most of our  device normative
> statements. It would IMHO only decrease clarity and increase the confusion.
> But if you ask yourself is the behavior prescribed by an arbitrary device
> normative statemen "as per device configuration" or not, I don't think one
> would ever answer "no" to that question.
>
I am ok to drop the "per device configuration" given filtering is contextual to the section.

So far I see two comments in version v10 that needs to be addressed.
1. Correct the link the commit message to some past discussion.
2. Drop "as per device configuration"

Do you suggest V11?
Or it its ok, I prefer to write follow up "Fixes" patch to drop the per device configuration part.
Given that we are closed to voting deadline, and change is not breaking the spec.
Usually in other projects for minor things like above #1, maintainer applies the local change to commit log before applying the patch to avoid unnecessary churn of people's time.

Please suggest next step on resolving it.
 
> > +
> > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST accept all
> > +VLAN tagged packets whose VLAN tag is present in the VLAN filter
> > +table and SHOULD drop all VLAN tagged packets whose VLAN tag is
> > +absent in the VLAN filter table.
> 
> We could also add "as per device configuration" here as well...
> 
Yeah, lets keep it the way it is. It is easy to understand. :)

> From the other thread
> 
> On Mon, 23 Jan 2023 12:41:16 +0000
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > > BTW why SHOULD drop?
> > This is the offload feature to drop such packets so that OS driver
> > doesn't need to do the filtering work. :)
> 
> Why not MUST drop? If the device is allowed to produce false negatives, then I
> think the OS probably wants to check again to filter out those.
> Can we change SHOULD to MUST, so the conforming device is guaranteed to do
> the filtering, and the OS can rely on it?

In this patch we are not changing the spec.
It is done on a best effort basis based on existing implementation.
Hence, its is SHOULD.
Refer to the github issue [1], which you didn't vote in [2].

[1] https://github.com/oasis-tcs/virtio-spec/issues/47
[2] https://www.oasis-open.org/committees/ballot.php?id=3419
[3] https://man7.org/linux/man-pages/man8/tc-actions.8.html



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

[virtio-dev] Re: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Wed, Jan 25, 2023 at 07:09:28PM +0000, Parav Pandit wrote:
> 
> > From: Halil Pasic <pasic@linux.ibm.com>
> > Sent: Wednesday, January 25, 2023 1:05 PM
> > 
> > On Thu, 12 Jan 2023 23:25:50 +0200
> > Parav Pandit <parav@nvidia.com> wrote:
> > 
> > > The filtering behavior of the VLAN filter commands is not very clear
> > > as discussed in thread [1].
> > >
> > > Hence, add the command description and device requirements for it.
> > >
> > > [1]
> > > https://lists.oasis-open.org/archives/virtio-dev/202301/msg00210.html
> > 
> > Reference is wrong. This links an unrelated thread.
> > 
> > For reference see:
> > 
> > https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09424.html
> >
> In the commit log the right reference to the discussion should be,
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg912392.html
> 
> More below whether we need v11 or not.
> 
> > [..]
> > @ -1194,7 +1194,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi
> > >  \paragraph{VLAN Filtering}\label{sec:Device Types / Network Device /
> > > Device Operation / Control Virtqueue / VLAN Filtering}
> > >
> > >  If the driver negotiates the VIRTIO_NET_F_CTRL_VLAN feature, it -can
> > > control a VLAN filter table in the device.
> > > +can control a VLAN filter table in the device. The VLAN filter table
> > > +applies only to VLAN tagged packets.
> > > +
> > > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with an
> > > +empty VLAN filter table.
> > >
> > >  \begin{note}
> > >  Similar to the MAC address based filtering, the VLAN filtering @@
> > > -1210,6 +1214,22 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > Types / Network Device / Devi  Both the VIRTIO_NET_CTRL_VLAN_ADD and
> > > VIRTIO_NET_CTRL_VLAN_DEL  command take a little-endian 16-bit VLAN id as
> > the command-specific-data.
> > >
> > [..]
> > > +
> > > +\devicenormative{\subparagraph}{VLAN Filtering}{Device Types /
> > > +Network Device / Device Operation / Control Virtqueue / VLAN
> > > +Filtering}
> > > +
> > > +When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept
> > > +all VLAN tagged packets as per the device configuration.
> > 
> > 
> > The story so far (other thread):
> > 
> > > > I find "as per the device configuration" difficult to comprehend.
> > > > Maybe we can work on this with an editorial. What are you trying to
> > > > express here? On one hand you say "the device MUST accept all VLAN
> > > > tagged packets" on the other hand "per the device configuration" may
> > > > explain, or may relativize and restrict that statement.
> > > >
> > > From VLAN filtering perspective, it should accept all packets, but this is
> > subject to other device configuration such as MAC programming.
> > > The current one is fine that cover both the aspects to VLAN specific and other
> > config.
> > 
> > IMHO you need to be explicit about this "From the VLAN filtering perspective".
> > A sub-case of "not negotiated" is "not offered because the device does not
> > support it" and in that case there is conceptually no "VLAN filtering
> > perspective".
> > 
> Not negotiated covers two conditions.
> 1. device offered, but driver choose to keep it disabled.
> 2. device didn't offer, so driver didn't enable it
> 
> This section talks about VLAN filtering, so context is VLAN filtering and no need to bring other context.
> I tend to agree that we can drop the line "per device configuration".
> 
> > > > Would
> > > > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, VLAN filtering is
> > > > not applied. I.e. VLAN tags are ignored by the device."
> > > > also work?
> > > >
> > > No. its not about ignoring "VLAN tags". Its about dealing with "packets" that
> > starts with the VLAN tag.
> > 
> > I'm not sure I understand the difference.
> > 
> > Would just
> > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, no VLAN filtering is done
> > by the device."
> > work?
> >
> This is also fine.
> However, I prefer the current spec terminology used in mac, promiscuous etc area such as "receive/accept/drop".
> It is lot clearer to understand in steering world.
> And it also aligns to the steering tools such as tc [3] which defines the action as "drop/mirred/redirect" etc.
> 
> Hence, lets keep
> "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept all VLAN tagged packets"
> 
> This way spec language is same for negotiated/non-negotiated case of accept/drop etc.
> 
> > I don't like this "as per device configuration" formulation. If you have multiple
> > filtering mechanisms, like VLAN and MAC, and describe all of these using this
> > "as per device configuration", you would end up with statements like:
> > "When feature F_X is not negotiated, the device MUST accept all P_X packets as
> > the per device configuration."
> > Where as per device configuration is supposed to cover all other active filtering
> > mechanisms except the one that is tied to F_X, but exclude the mechanism that
> > is tied to the feature F_X.
> > 
> > I'm also not convinced about the information content of "as per device
> > configuration". Kind of the complementer would be "contrary to the device
> > configuration" which does not make much sense. I would also argue that we
> > can add "as per device configuration" to most of our  device normative
> > statements. It would IMHO only decrease clarity and increase the confusion.
> > But if you ask yourself is the behavior prescribed by an arbitrary device
> > normative statemen "as per device configuration" or not, I don't think one
> > would ever answer "no" to that question.
> >
> I am ok to drop the "per device configuration" given filtering is contextual to the section.
> 
> So far I see two comments in version v10 that needs to be addressed.
> 1. Correct the link the commit message to some past discussion.
> 2. Drop "as per device configuration"
> 
> Do you suggest V11?
> Or it its ok, I prefer to write follow up "Fixes" patch to drop the per device configuration part.
> Given that we are closed to voting deadline, and change is not breaking the spec.
> Usually in other projects for minor things like above #1, maintainer applies the local change to commit log before applying the patch to avoid unnecessary churn of people's time.
> 
> Please suggest next step on resolving it.

Yea #1 does not matter much.

This ballot
	https://lists.oasis-open.org/archives/virtio-dev/202301/msg00231.html
is likely set to approve v10.

If you really want to withdraw it you can request that.

Alternatively post a patch on top, if it's a minor cleanup it can be applied
without a vote. I'd say removing "as per device configuration" is
probably a minor cleanup.


> > > +
> > > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST accept all
> > > +VLAN tagged packets whose VLAN tag is present in the VLAN filter
> > > +table and SHOULD drop all VLAN tagged packets whose VLAN tag is
> > > +absent in the VLAN filter table.
> > 
> > We could also add "as per device configuration" here as well...
> > 
> Yeah, lets keep it the way it is. It is easy to understand. :)
> 
> > From the other thread
> > 
> > On Mon, 23 Jan 2023 12:41:16 +0000
> > Parav Pandit <parav@nvidia.com> wrote:
> > 
> > > > BTW why SHOULD drop?
> > > This is the offload feature to drop such packets so that OS driver
> > > doesn't need to do the filtering work. :)
> > 
> > Why not MUST drop? If the device is allowed to produce false negatives, then I
> > think the OS probably wants to check again to filter out those.
> > Can we change SHOULD to MUST, so the conforming device is guaranteed to do
> > the filtering, and the OS can rely on it?
> 
> In this patch we are not changing the spec.

Well yes we do :) I think you mean we are not adding new features.

> It is done on a best effort basis based on existing implementation.
> Hence, its is SHOULD.
> Refer to the github issue [1], which you didn't vote in [2].
> 
> [1] https://github.com/oasis-tcs/virtio-spec/issues/47
> [2] https://www.oasis-open.org/committees/ballot.php?id=3419
> [3] https://man7.org/linux/man-pages/man8/tc-actions.8.html
> 


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

[virtio-dev] RE: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, January 25, 2023 3:32 PM
> 

> > So far I see two comments in version v10 that needs to be addressed.
> > 1. Correct the link the commit message to some past discussion.
> > 2. Drop "as per device configuration"
> >
> > Do you suggest V11?
> > Or it its ok, I prefer to write follow up "Fixes" patch to drop the per device
> configuration part.
> > Given that we are closed to voting deadline, and change is not breaking the
> spec.
> > Usually in other projects for minor things like above #1, maintainer applies
> the local change to commit log before applying the patch to avoid unnecessary
> churn of people's time.
> >
> > Please suggest next step on resolving it.
> 
> Yea #1 does not matter much.
> 
> This ballot
> 	https://lists.oasis-open.org/archives/virtio-
> dev/202301/msg00231.html
> is likely set to approve v10.
> 
> If you really want to withdraw it you can request that.
> 
> Alternatively post a patch on top, if it's a minor cleanup it can be applied
> without a vote. I'd say removing "as per device configuration" is probably a
> minor cleanup.
>
I am in middle of writing this fixes patch.
I will post a patch of this cleanup later today.

[..]
> >
> > In this patch we are not changing the spec.
> 
> Well yes we do :) I think you mean we are not adding new features.
> 
You are right. :)
We are not changing device or driver behavior in this patch...
We are improving/clarifying things in crisp details for betterment.

---------------------------------------------------------------------
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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Wed, Jan 25, 2023 at 07:04:54PM +0100, Halil Pasic wrote:
> > +
> > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST
> > +accept all VLAN tagged packets whose VLAN tag is present in
> > +the VLAN filter table and SHOULD drop all VLAN tagged packets
> > +whose VLAN tag is absent in the VLAN filter table.
> 
> We could also add "as per device configuration" here as well...
> 
> >From the other thread
> 
> On Mon, 23 Jan 2023 12:41:16 +0000
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > > BTW why SHOULD drop?  
> > This is the offload feature to drop such packets so that OS driver doesn't need to do the filtering work. :)
> 
> Why not MUST drop? If the device is allowed to produce false negatives,
> then I think the OS probably wants to check again to filter out those.
> Can we change SHOULD to MUST, so the conforming device is guaranteed to
> do the filtering, and the OS can rely on it?

Well the spec says:
	Similar to the MAC address based filtering, the VLAN filtering
	is also best-effort: unwanted packets could still arrive.

 am not sure why - IIRC MAC filtering is best effort because
a. we don't expose table size to guests so the table can always overflow.
b. we like to rely on bridge to filter for performance and *that* is
   best effort.

but vlans are different aren't they? Anyway changing that will need
a new feature bit.

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

RE: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, January 25, 2023 3:56 PM
> 
> On Wed, Jan 25, 2023 at 07:04:54PM +0100, Halil Pasic wrote:
> > > +
> > > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST accept
> > > +all VLAN tagged packets whose VLAN tag is present in the VLAN
> > > +filter table and SHOULD drop all VLAN tagged packets whose VLAN tag
> > > +is absent in the VLAN filter table.
> >
> > We could also add "as per device configuration" here as well...
> >
> > >From the other thread
> >
> > On Mon, 23 Jan 2023 12:41:16 +0000
> > Parav Pandit <parav@nvidia.com> wrote:
> >
> > > > BTW why SHOULD drop?
> > > This is the offload feature to drop such packets so that OS driver
> > > doesn't need to do the filtering work. :)
> >
> > Why not MUST drop? If the device is allowed to produce false
> > negatives, then I think the OS probably wants to check again to filter out
> those.
> > Can we change SHOULD to MUST, so the conforming device is guaranteed
> > to do the filtering, and the OS can rely on it?
> 
> Well the spec says:
> 	Similar to the MAC address based filtering, the VLAN filtering
> 	is also best-effort: unwanted packets could still arrive.
> 
>  am not sure why - IIRC MAC filtering is best effort because a. we don't expose
> table size to guests so the table can always overflow.
> b. we like to rely on bridge to filter for performance and *that* is
>    best effort.
> 
> but vlans are different aren't they? Anyway changing that will need a new
> feature bit.
I do not the history for about "Note" on why it was made best effort.
To best of my knowledge, it should not be best effort.
Device should not overflow the table. It should fail the ADD call.

If the above claim of "Note" is not strong enough, we can probably run a different issue to make it "MUST".
I am not sure we really need feature bit it we fix the note.

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-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Parav Pandit
> Sent: Wednesday, January 25, 2023 6:31 PM
> To: Michael S. Tsirkin <mst@redhat.com>; Halil Pasic <pasic@linux.ibm.com>
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, January 25, 2023 3:56 PM
> >
> > On Wed, Jan 25, 2023 at 07:04:54PM +0100, Halil Pasic wrote:
> > > > +
> > > > +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST accept
> > > > +all VLAN tagged packets whose VLAN tag is present in the VLAN
> > > > +filter table and SHOULD drop all VLAN tagged packets whose VLAN
> > > > +tag is absent in the VLAN filter table.
> > >
> > > We could also add "as per device configuration" here as well...
> > >
> > > >From the other thread
> > >
> > > On Mon, 23 Jan 2023 12:41:16 +0000
> > > Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > > > BTW why SHOULD drop?
> > > > This is the offload feature to drop such packets so that OS driver
> > > > doesn't need to do the filtering work. :)
> > >
> > > Why not MUST drop? If the device is allowed to produce false
> > > negatives, then I think the OS probably wants to check again to
> > > filter out
> > those.
> > > Can we change SHOULD to MUST, so the conforming device is guaranteed
> > > to do the filtering, and the OS can rely on it?
> >
> > Well the spec says:
> > 	Similar to the MAC address based filtering, the VLAN filtering
> > 	is also best-effort: unwanted packets could still arrive.
> >
> >  am not sure why - IIRC MAC filtering is best effort because a. we
> > don't expose table size to guests so the table can always overflow.
> > b. we like to rely on bridge to filter for performance and *that* is
> >    best effort.
> >
> > but vlans are different aren't they? Anyway changing that will need a
> > new feature bit.
> I do not the history for about "Note" on why it was made best effort.
> To best of my knowledge, it should not be best effort.
> Device should not overflow the table. It should fail the ADD call.
> 
> If the above claim of "Note" is not strong enough, we can probably run a
> different issue to make it "MUST".
> I am not sure we really need feature bit it we fix the note.

I clicked too early while typing.
Since DEL operation doesn't synchronize with the data path of device and driver, at some point a packet in pipe can be still received with removed VLAN.
So may be the note was added.
But for sure ADD should not be best effort basis.

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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Si-Wei Liu]

On 1/25/2023 3:46 PM, Parav Pandit wrote:
>> From: Parav Pandit
>> Sent: Wednesday, January 25, 2023 6:31 PM
>> To: Michael S. Tsirkin <mst@redhat.com>; Halil Pasic <pasic@linux.ibm.com>
>>
>>
>>> From: Michael S. Tsirkin <mst@redhat.com>
>>> Sent: Wednesday, January 25, 2023 3:56 PM
>>>
>>> On Wed, Jan 25, 2023 at 07:04:54PM +0100, Halil Pasic wrote:
>>>>> +
>>>>> +When VIRTIO_NET_F_CTRL_VLAN is negotiated, the device MUST accept
>>>>> +all VLAN tagged packets whose VLAN tag is present in the VLAN
>>>>> +filter table and SHOULD drop all VLAN tagged packets whose VLAN
>>>>> +tag is absent in the VLAN filter table.
>>>> We could also add "as per device configuration" here as well...
>>>>
>>>> >From the other thread
>>>>
>>>> On Mon, 23 Jan 2023 12:41:16 +0000
>>>> Parav Pandit <parav@nvidia.com> wrote:
>>>>
>>>>>> BTW why SHOULD drop?
>>>>> This is the offload feature to drop such packets so that OS driver
>>>>> doesn't need to do the filtering work. :)
>>>> Why not MUST drop? If the device is allowed to produce false
>>>> negatives, then I think the OS probably wants to check again to
>>>> filter out
>>> those.
>>>> Can we change SHOULD to MUST, so the conforming device is guaranteed
>>>> to do the filtering, and the OS can rely on it?
>>> Well the spec says:
>>> 	Similar to the MAC address based filtering, the VLAN filtering
>>> 	is also best-effort: unwanted packets could still arrive.
>>>
>>>   am not sure why - IIRC MAC filtering is best effort because a. we
>>> don't expose table size to guests so the table can always overflow.
>>> b. we like to rely on bridge to filter for performance and *that* is
>>>     best effort.
>>>
>>> but vlans are different aren't they? Anyway changing that will need a
>>> new feature bit.
>> I do not the history for about "Note" on why it was made best effort.
>> To best of my knowledge, it should not be best effort.
>> Device should not overflow the table. It should fail the ADD call.
>>
>> If the above claim of "Note" is not strong enough, we can probably run a
>> different issue to make it "MUST".
>> I am not sure we really need feature bit it we fix the note.
> I clicked too early while typing.
> Since DEL operation doesn't synchronize with the data path of device and driver, at some point a packet in pipe can be still received with removed VLAN.
> So may be the note was added.
> But for sure ADD should not be best effort basis.
If I recall the software backend implementation correctly, it's even 
unsynchronized for ADD. That's where the best effort came from (existing 
artifact in some software implementation). While I don't think it's 
worth adding another feature representing the difference. That said, I 
agree we SHOULD try to synchronize packet reception with ADD and DEL if 
at all possible.

-Siwei



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

[virtio-dev] Re: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> I clicked too early while typing.
> Since DEL operation doesn't synchronize with the data path of device and driver, at some point a packet in pipe can be still received with removed VLAN.
> So may be the note was added.
> But for sure ADD should not be best effort basis.

why are you sure? dropping a packet or two while reconfiguring might not be a big deal either.

-- 
MST


---------------------------------------------------------------------
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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> > > but vlans are different aren't they? Anyway changing that will need a
> > > new feature bit.
> > I do not the history for about "Note" on why it was made best effort.
> > To best of my knowledge, it should not be best effort.
> > Device should not overflow the table. It should fail the ADD call.

For the MAC table?  Failing commands generally is
a bad way to communicate to drivers. If we do there needs to also
be a way to report our limits. In this case it's enough to
look at qemu to see an implementation that just sets a "table
overflow" flag and stops filtering.

Similarly a populat vhost+bridge+tap combo does not keep
the copy of the mac table at all and instead realies on
guest sending announcement packets and bridge learning from these.


> > If the above claim of "Note" is not strong enough, we can probably run a
> > different issue to make it "MUST".
> > I am not sure we really need feature bit it we fix the note.
> 
> I clicked too early while typing.
> Since DEL operation doesn't synchronize with the data path of device and driver, at some point a packet in pipe can be still received with removed VLAN.
> So may be the note was added.
> But for sure ADD should not be best effort basis.

So for VLANs this was added by Tiwei. I guess it looked sane then
but now I don't remember the motivation. Tiwei CC'd.

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

Re: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Halil Pasic]

On Wed, 25 Jan 2023 19:09:28 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > Would just
> > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, no VLAN filtering is done
> > by the device."
> > work?
> >  
> This is also fine.
> However, I prefer the current spec terminology used in mac, promiscuous etc area such as "receive/accept/drop".
> It is lot clearer to understand in steering world.
> And it also aligns to the steering tools such as tc [3] which defines the action as "drop/mirred/redirect" etc.

My problem is not the word accept itself. Let me try to explain it once
again, now that we have gotten rid of the confusing "as per device
configuration". Now our sentence looks like this.

When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept
all VLAN tagged packets.

Let us analyze it. 
Predicate: "MUST accept"
Object: "all VLAN tagged packets"
Subject: "the device"

Compare that to man 8 tc. There they make it clear that there may
the things like the next filter, and that those are comprised of
actions. 

I think, in a system with multiple filters, it is a very different
piece of cake stating that a:
* A certain filter accepts an input.
* The whole system accepts an input.

If in our sentence the subject where not the "the device" but
something like "the VLAN filtering facility" or you name it,
I would be fine with that.

But when without the corresponding domain knowledge, the device
MUST accept sounds like the driver of an conforming device will see that
packet. I.e. the device dropping that very same packet for a different
reason is not possible without violating that normative.

Of course with enough domain knowledge, one would know that we didn't
really mean that the "the packet must be accepted by the device" but
rather that the "VLAN filtering mechanism is not allowed to drop the
give packet". Because the former makes no sense.

In the end, I believe the target audience of this document does have
the domain knowledge to deal with the situation. But I prefer normative
statement that don't lend themselves to misinterpretation.

Regards,
Halil



> 
> Hence, lets keep
> "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept all VLAN tagged packets"
> 
> This way spec language is same for negotiated/non-negotiated case of accept/drop etc.


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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Thursday, January 26, 2023 6:49 AM
> 
> On Wed, 25 Jan 2023 19:09:28 +0000
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > > Would just
> > > "When VIRTIO_NET_F_CTRL_VLAN is not negotiated, no VLAN filtering is
> > > done by the device."
> > > work?
> > >
> > This is also fine.
> > However, I prefer the current spec terminology used in mac, promiscuous etc
> area such as "receive/accept/drop".
> > It is lot clearer to understand in steering world.
> > And it also aligns to the steering tools such as tc [3] which defines the action
> as "drop/mirred/redirect" etc.
> 
> My problem is not the word accept itself. Let me try to explain it once again,
> now that we have gotten rid of the confusing "as per device configuration".
> Now our sentence looks like this.
> 
> When VIRTIO_NET_F_CTRL_VLAN is not negotiated, the device MUST accept all
> VLAN tagged packets.
> 
> Let us analyze it.
> Predicate: "MUST accept"
> Object: "all VLAN tagged packets"
> Subject: "the device"
> 
> Compare that to man 8 tc. There they make it clear that there may the things
> like the next filter, and that those are comprised of actions.
> 
> I think, in a system with multiple filters, it is a very different piece of cake stating
> that a:
> * A certain filter accepts an input.
> * The whole system accepts an input.
> 
> If in our sentence the subject where not the "the device" but something like
> "the VLAN filtering facility" or you name it, I would be fine with that.
> 
> But when without the corresponding domain knowledge, the device MUST
> accept sounds like the driver of an conforming device will see that packet. I.e.
> the device dropping that very same packet for a different reason is not possible
> without violating that normative.
> 
> Of course with enough domain knowledge, one would know that we didn't
> really mean that the "the packet must be accepted by the device" but rather
> that the "VLAN filtering mechanism is not allowed to drop the give packet".
> Because the former makes no sense.
> 
> In the end, I believe the target audience of this document does have the
> domain knowledge to deal with the situation. But I prefer normative statement
> that don't lend themselves to misinterpretation.
> 
Yes. exactly. I did clarify in one of the previous response s(don't have the link handy) that steering section overall for mac, promiscuous, vlan etc flitering needs some rewrite for cross dependencies.
I would possibly drive it in a separate issue word it. Because the scope of it beyond just this issue.


---------------------------------------------------------------------
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] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Thu, Jan 26, 2023 at 04:17:30PM +0000, Parav Pandit wrote:
> Because the scope of it beyond just this issue.

You keep using that word. I don't think it means what you think it
means.


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-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, January 26, 2023 12:08 AM
> 
> On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> > > > but vlans are different aren't they? Anyway changing that will
> > > > need a new feature bit.
> > > I do not the history for about "Note" on why it was made best effort.
> > > To best of my knowledge, it should not be best effort.
> > > Device should not overflow the table. It should fail the ADD call.
> 
> For the MAC table?  Failing commands generally is a bad way to communicate
> to drivers. 
Failing the command when device cannot do the job is right way to tell the driver to follow the contract in general terms.
A command could be to disable a vq, delete entry, add entry, encrypt data, LSO packets and more.
Again in general terms. :)
Device telling driver that it did the job and not doing it in device only brings inferior user experience.

May be QEMU has some history...

> If we do there needs to also be a way to report our limits. 
Yes, I agree. Reporting limits is good approach we should follow.

> In this case
> it's enough to look at qemu to see an implementation that just sets a "table
> overflow" flag and stops filtering.
> 
> Similarly a populat vhost+bridge+tap combo does not keep the copy of the mac
> table at all and instead realies on guest sending announcement packets and
> bridge learning from these.
>
How does this work in a sane way?
Driver set the mac filter table and it doesn't expect random packets to come in?
If the device doesn't maintain the filter table, it is "SHOULD" requirement as stated today on best effort basis.
 
> 
> > > If the above claim of "Note" is not strong enough, we can probably
> > > run a different issue to make it "MUST".
> > > I am not sure we really need feature bit it we fix the note.
> >
> > I clicked too early while typing.
> > Since DEL operation doesn't synchronize with the data path of device and
> driver, at some point a packet in pipe can be still received with removed VLAN.
> > So may be the note was added.
> > But for sure ADD should not be best effort basis.
> 
> So for VLANs this was added by Tiwei. I guess it looked sane then but now I
> don't remember the motivation. Tiwei CC'd.
> --
> 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/

Re: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Thu, Jan 26, 2023 at 04:27:47PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, January 26, 2023 12:08 AM
> > 
> > On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> > > > > but vlans are different aren't they? Anyway changing that will
> > > > > need a new feature bit.
> > > > I do not the history for about "Note" on why it was made best effort.
> > > > To best of my knowledge, it should not be best effort.
> > > > Device should not overflow the table. It should fail the ADD call.
> > 
> > For the MAC table?  Failing commands generally is a bad way to communicate
> > to drivers. 
> Failing the command when device cannot do the job is right way to tell the driver to follow the contract in general terms.

There's no contract though. My point is that table size is not
in the spec. As long as that is the case failing because it's too big
is a bad idea.

> A command could be to disable a vq, delete entry, add entry, encrypt data, LSO packets and more.
> Again in general terms. :)
> Device telling driver that it did the job and not doing it in device only brings inferior user experience.
> 
> May be QEMU has some history...
> 
> > If we do there needs to also be a way to report our limits. 
> Yes, I agree. Reporting limits is good approach we should follow.
> 
> > In this case
> > it's enough to look at qemu to see an implementation that just sets a "table
> > overflow" flag and stops filtering.
> > 
> > Similarly a populat vhost+bridge+tap combo does not keep the copy of the mac
> > table at all and instead realies on guest sending announcement packets and
> > bridge learning from these.
> >
> How does this work in a sane way?
> Driver set the mac filter table and it doesn't expect random packets to come in?
> If the device doesn't maintain the filter table, it is "SHOULD" requirement as stated today on best effort basis.

exaactly, that is why it's SHOULD.

> > 
> > > > If the above claim of "Note" is not strong enough, we can probably
> > > > run a different issue to make it "MUST".
> > > > I am not sure we really need feature bit it we fix the note.
> > >
> > > I clicked too early while typing.
> > > Since DEL operation doesn't synchronize with the data path of device and
> > driver, at some point a packet in pipe can be still received with removed VLAN.
> > > So may be the note was added.
> > > But for sure ADD should not be best effort basis.
> > 
> > So for VLANs this was added by Tiwei. I guess it looked sane then but now I
> > don't remember the motivation. Tiwei CC'd.
> > --
> > 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/

RE: [virtio-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, January 26, 2023 11:31 AM
> 
> On Thu, Jan 26, 2023 at 04:27:47PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Thursday, January 26, 2023 12:08 AM
> > >
> > > On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> > > > > > but vlans are different aren't they? Anyway changing that will
> > > > > > need a new feature bit.
> > > > > I do not the history for about "Note" on why it was made best effort.
> > > > > To best of my knowledge, it should not be best effort.
> > > > > Device should not overflow the table. It should fail the ADD call.
> > >
> > > For the MAC table?  Failing commands generally is a bad way to
> > > communicate to drivers.
> > Failing the command when device cannot do the job is right way to tell the
> driver to follow the contract in general terms.
> 
> There's no contract though. My point is that table size is not in the spec. As long
> as that is the case failing because it's too big is a bad idea.
> 
I agree that table size should be in the spec.
But I am not convinced that a device should return a fake success and do something wonky behavior.
For some legacy reason it may have been done, but spec guideline should not be to return some fake success..

> > > So for VLANs this was added by Tiwei. I guess it looked sane then
> > > but now I don't remember the motivation. Tiwei CC'd.
> > > --
> > > MST
I get undeliverable reply for Tiwei's email.
I don't see vlan table being done on best effort in QEMU, nor in mlx5 vdpa device.

Si-Wei,
Do you know any implementation that attempts to do vlan on best effort?
Otherwise we should clean this up.

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-comment] [PATCH v10] virtio-net: Clarify VLAN filter table configuration

[Michael S. Tsirkin]

On Thu, Jan 26, 2023 at 04:42:29PM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, January 26, 2023 11:31 AM
> > 
> > On Thu, Jan 26, 2023 at 04:27:47PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, January 26, 2023 12:08 AM
> > > >
> > > > On Wed, Jan 25, 2023 at 11:46:58PM +0000, Parav Pandit wrote:
> > > > > > > but vlans are different aren't they? Anyway changing that will
> > > > > > > need a new feature bit.
> > > > > > I do not the history for about "Note" on why it was made best effort.
> > > > > > To best of my knowledge, it should not be best effort.
> > > > > > Device should not overflow the table. It should fail the ADD call.
> > > >
> > > > For the MAC table?  Failing commands generally is a bad way to
> > > > communicate to drivers.
> > > Failing the command when device cannot do the job is right way to tell the
> > driver to follow the contract in general terms.
> > 
> > There's no contract though. My point is that table size is not in the spec. As long
> > as that is the case failing because it's too big is a bad idea.
> > 
> I agree that table size should be in the spec.
> But I am not convinced that a device should return a fake success and do something wonky behavior.
> For some legacy reason it may have been done, but spec guideline should not be to return some fake success..
> 
> > > > So for VLANs this was added by Tiwei. I guess it looked sane then
> > > > but now I don't remember the motivation. Tiwei CC'd.
> > > > --
> > > > MST
> I get undeliverable reply for Tiwei's email.
> I don't see vlan table being done on best effort in QEMU, nor in mlx5 vdpa device.

No but MAC table is. And with bridge, we rely on management to read vlan
table and update the bridge vlan config. Which happens asynchronously
so again - best effort.


> Si-Wei,
> Do you know any implementation that attempts to do vlan on best effort?
> Otherwise we should clean this up.

Probably too late without a feature bit.

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