RE: [EXT] [PATCH 6/7] virtio-net: Add flow filter requests

[Parav Pandit <parav@nvidia.com>]

> From: Satananda Burla <sburla@marvell.com>
> Sent: Sunday, October 1, 2023 3:13 PM


> > +struct virtio_net_ff_match_fields {
> > +        le32 num_entries;
> > +        struct virtio_net_ff_match_field match_entries[]; };
> It is better to fix the network protocol order here.(l2,l3,l4).

It is recommended but I am not sure if we should enforce it.

How about writing it as order SHOULD follow the packet field order..
This will cover across L2, L3 and within a given header type too.

> > +
> > +#define VIRTIO_NET_FF_DEST_RQ 0
> > +
> > +struct virtio_net_ff_dest {
> > +        u8 dest_type;
> > +        u8 reserved[3];
> > +        union {
> > +                le16 vq_index;
> > +                le32 reserved1;
> > +        };
> > +};
> > +
> > +struct virtio_net_ff_action {
> > +        u8 action;
> > +        u8 reserved[7];
> > +};
> > +
> > +#define VIRTIO_NET_FF_ACTION_DROP 0
> > +#define VIRTIO_NET_FF_ACTION_FORWARD 1
> > +
> > +struct virtio_net_ff_add {
> > +        u8 op;
> > +        u8 priority;	/* higher the value, higher priority */
> > +        u16 group_id;
> > +        le32 id;
> > +        struct virtio_net_ff_match_fields match;
> > +        struct virtio_net_ff_dest dest;
> > +        struct virtio_net_ff_action action;
> Isn't dest also an action ? for instance, if ACTION is drop, there is no point in
> adding destination, similarly if ACTION is forward, can we not add the vq_index
> to the action itself using the reserved bytes? based on action type, the
> additional bytes could be interpreted.
> > +
> > +        struct virtio_net_ff_match_fields mask;	/* optional */
> How is the presence of this field indicated if it is optional ?
> Does it need a field like has_mask?
Yes. I missed it.

> Can this be not added into struct virtio_net_ff_match_field ?
I think yes, the issue is not all use cases (like ARFS) needs mask, who always uses exact values.
So communicating and setting mask is redundant (also the struct is bit large).

But I like your suggestion that we can have mask defined as, just match_value.
Since value cannot live without its supporting type, finding it hard to have only value for the mask.

> The mask only applies to struct virtio_net_ff_match_value. So it may be better
> to just use struct virtio_net_ff_match_field {
> 	struct virtio_net_ff_match_type type;
> 	struct virtio_net_ff_match_value value;
> 	struct virtio_net_ff_match_value mask;
> };
> instead of repeating the type again.
> > +
How about encoding has_mask in the type, so that mask can be optional in the above struct you proposed?
With that we get both the benefits of size saving and computation too.