On 2025-04-18 3:24 a.m., Paul Menzel wrote:
Dear Ahmed,
Thank you for your patch.
Am 18.04.25 um 00:12 schrieb Ahmed Zaki:
Use the new virtchnl2 OP codes to communicate with the Control Plane to
add flow steering filters. We add the basic functionality for ADD/Delete
Minor thing: ADD/Delete are spelled differently.
with TCP/UDP IPv4 only. Support for other OP codes and protocols will be
added later.
Sure, will fix it.
Although it seems quite a lot of boilerplate, still for a diffstat of
more than 400 lines, a paragraph about the implementation would be nice
to have.
Standard 'ethtool -N|--config-ntuple' should be used, for example:
# ethtool -N ens801f0d1 flow-type tcp4 src-ip 10.0.0.1 action 6
to route all IPv4/TCP traffic from IP 10.0.0.1 to queue 6.
Is there a way to verify, that the traffic really goes to queue 6?
I usually use "ethtool -S <dev> " and grep on the rx queue number while
sending a lot of traffic that matches the filter info.
Reviewed-by: Sridhar Samudrala <[email protected]>
Signed-off-by: Ahmed Zaki <[email protected]>
---
drivers/net/ethernet/intel/idpf/idpf.h | 13 +
.../net/ethernet/intel/idpf/idpf_ethtool.c | 298 +++++++++++++++++-
drivers/net/ethernet/intel/idpf/idpf_lib.c | 5 +
.../net/ethernet/intel/idpf/idpf_virtchnl.c | 104 ++++++
.../net/ethernet/intel/idpf/idpf_virtchnl.h | 6 +
5 files changed, 421 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/
ethernet/intel/idpf/idpf.h
index c21903310354..1c791f5ca601 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -252,6 +252,12 @@ struct idpf_port_stats {
struct virtchnl2_vport_stats vport_stats;
};
+struct idpf_fsteer_fltr {
+ struct list_head list;
+ u32 loc;
+ unsigned int q_index;
+};
+
/**
* struct idpf_tx_tstamp_stats - Tx timestamp statistics
* @tx_hwtstamp_lock: Lock to protect Tx tstamp stats
@@ -406,6 +412,8 @@ struct idpf_rss_data {
* ethtool
* @user_flags: User toggled config flags
* @mac_filter_list: List of MAC filters
+ * @num_fsteer_fltrs: number of flow steering filters
+ * @flow_steer_list: list of flow steering filters
*
* Used to restore configuration after a reset as the vport will get
wiped.
*/
@@ -417,6 +425,8 @@ struct idpf_vport_user_config_data {
u32 num_req_rxq_desc;
DECLARE_BITMAP(user_flags, __IDPF_USER_FLAGS_NBITS);
struct list_head mac_filter_list;
+ u16 num_fsteer_fltrs;
Is there a reason to limit it to u16? `unsigned int` would use the
default “system size”, that is probably also 16.
Looking again at this, I think u16 might be too small. However, I will
change it to u32 and not unsigned int:
1 - it might be used to check the hardware limits, so I like constant
size more than "system size".
2 - this matches ethtool_rxnfc.rule_cnt.
+ struct list_head flow_steer_list;
};
/**
@@ -878,4 +888,7 @@ int idpf_sriov_configure(struct pci_dev *pdev, int
num_vfs);
u8 idpf_vport_get_hsplit(const struct idpf_vport *vport);
bool idpf_vport_set_hsplit(const struct idpf_vport *vport, u8 val);
+int idpf_add_del_fsteer_filters(struct idpf_adapter *adapter,
+ struct virtchnl2_flow_rule_add_del *rule,
+ bool add);
Is it possible to pass the op, so it’s clear what calling the function
does, and one does not have to look up the signature definition?
Sure, I can pass the op codes.
idpf_modify_fsteer_filters(… , virtchnl2_op op);
#endif /* !_IDPF_H_ */
diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/
net/ethernet/intel/idpf/idpf_ethtool.c
index 7a4793749bc5..35e1d5694212 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
@@ -3,6 +3,7 @@
#include "idpf.h"
#include "idpf_ptp.h"
+#include "idpf_virtchnl.h"
/**
* idpf_get_rxnfc - command to get RX flow classification rules
<...>
+/**
+ * idpf_add_flow_steer - add a Flow Steering filter
+ * @netdev: network interface device structure
+ * @cmd: command to add Flow Steering filter
+ *
+ * Return: 0 on success and negative values for failure
+ */
+static int idpf_add_flow_steer(struct net_device *netdev,
+ struct ethtool_rxnfc *cmd)
+{
+ struct idpf_fsteer_fltr *fltr, *parent = NULL, *f;
+ struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_vport_user_config_data *user_config;
+ struct ethtool_rx_flow_spec *fsp = &cmd->fs;
+ struct virtchnl2_flow_rule_add_del *rule;
+ struct idpf_vport_config *vport_config;
+ struct virtchnl2_rule_action_set *acts;
+ struct virtchnl2_flow_rule_info *info;
+ struct virtchnl2_proto_hdrs *hdrs;
+ struct idpf_vport *vport;
+ u32 flow_type, q_index;
+ u16 num_rxq;
+ int err;
+
+ vport = idpf_netdev_to_vport(netdev);
+ vport_config = vport->adapter->vport_config[np->vport_idx];
+ user_config = &vport_config->user_config;
+ num_rxq = user_config->num_req_rx_qs;
+
+ flow_type = fsp->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS);
+ if (flow_type != fsp->flow_type)
+ return -EINVAL;
+
+ if (!idpf_sideband_action_ena(vport, fsp) ||
+ !idpf_sideband_flow_type_ena(vport, flow_type))
+ return -EOPNOTSUPP;
+
+ if (user_config->num_fsteer_fltrs > idpf_fsteer_max_rules(vport))
+ return -ENOSPC;
+
+ q_index = fsp->ring_cookie;
+ if (q_index >= num_rxq)
+ return -EINVAL;
+
+ rule = kzalloc(struct_size(rule, rule_info, 1), GFP_KERNEL);
+ if (!rule)
+ return -ENOMEM;
+
+ rule->vport_id = cpu_to_le32(vport->vport_id);
+ rule->count = cpu_to_le32(1);
+ info = &rule->rule_info[0];
+ info->rule_id = cpu_to_le32(fsp->location);
+
+ hdrs = &info->rule_cfg.proto_hdrs;
+ hdrs->tunnel_level = 0;
+ hdrs->count = cpu_to_le32(2);
+
+ acts = &info->rule_cfg.action_set;
+ acts->count = cpu_to_le32(1);
+ acts->actions[0].action_type = cpu_to_le32(VIRTCHNL2_ACTION_QUEUE);
+ acts->actions[0].act_conf.q_id = cpu_to_le32(q_index);
+
+ switch (flow_type) {
+ case UDP_V4_FLOW:
+ idpf_fsteer_fill_ipv4(hdrs, fsp);
+ idpf_fsteer_fill_udp(hdrs, fsp, true);
+ break;
+ case TCP_V4_FLOW:
+ idpf_fsteer_fill_ipv4(hdrs, fsp);
+ idpf_fsteer_fill_tcp(hdrs, fsp, true);
+ break;
+ default:
+ err = -EINVAL;
+ goto out;
+ }
+
+ err = idpf_add_del_fsteer_filters(vport->adapter, rule, true);
+ if (err)
+ goto out;
+
+ if (info->status != cpu_to_le32(VIRTCHNL2_FLOW_RULE_SUCCESS)) {
+ err = -EIO;
+ goto out;
+ }
+
+ fltr = kzalloc(sizeof(*fltr), GFP_KERNEL);
+ if (!fltr) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ fltr->loc = fsp->location;
+ fltr->q_index = q_index;
fltr->q_index in the struct is unsigned int, and here it is u32, isn’t it?
Good catch, I changed fltr->q_index to be u32, this matches
fsp->ring_cookie.
+ list_for_each_entry(f, &user_config->flow_steer_list, list) {
+ if (f->loc >= fltr->loc)
+ break;
+ parent = f;
+ }
+
+ if (parent)
+ list_add(&fltr->list, &parent->list);
+ else
+ list_add(&fltr->list, &user_config->flow_steer_list);
You could use the ternary operator.
Sure, will do.
+
+ user_config->num_fsteer_fltrs++;
+
+out:
+ kfree(rule);
+ return err;
+}
+
<...>
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 06c33b638e60..0f827a184176 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -890,6 +890,37 @@ static int idpf_send_get_caps_msg(struct
idpf_adapter *adapter)
return 0;
}
+/**
+ * idpf_add_del_fsteer_filters - Send virtchnl add/del Flow Steering
message
+ * @adapter: adapter info struct
+ * @rule: Flow steering rule to add/delete
+ * @add: True to add filter, FALSE to delete
+ *
+ * Send ADD/DELETE flow steering virtchnl message and receive the
result.
+ *
+ * Return: 0 on success, negative on failure.
+ */
+int idpf_add_del_fsteer_filters(struct idpf_adapter *adapter,
+ struct virtchnl2_flow_rule_add_del *rule,
+ bool add)
+{
+ int rule_count = le32_to_cpu(rule->count);
+ struct idpf_vc_xn_params xn_params = {};
+ ssize_t reply_sz;
+
+ xn_params.vc_op = add ? VIRTCHNL2_OP_ADD_FLOW_RULE :
+ VIRTCHNL2_OP_DEL_FLOW_RULE;
+ xn_params.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
+ xn_params.async = false;
+ xn_params.send_buf.iov_base = rule;
+ xn_params.send_buf.iov_len = struct_size(rule, rule_info,
rule_count);
+ xn_params.recv_buf.iov_base = rule;
+ xn_params.recv_buf.iov_len = struct_size(rule, rule_info,
rule_count);
+
+ reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
+ return reply_sz < 0 ? reply_sz : 0;
+}
+
/**
* idpf_vport_alloc_max_qs - Allocate max queues for a vport
* @adapter: Driver specific private structure
@@ -3491,6 +3522,79 @@ bool idpf_is_capability_ena(struct idpf_adapter
*adapter, bool all,
return !!(*cap_field & flag);
}
+/**
+ * idpf_vport_is_cap_ena - Check if vport capability is enabled
+ * @vport: Private data struct
+ * @flag: flag(s) to check
+ *
+ * Return: true if the capability is supported, false otherwise
+ */
+bool idpf_vport_is_cap_ena(struct idpf_vport *vport, u16 flag)
Spell it out to enabled?
Some existing functions already use "ena", like idpf_is_cap_ena_all(),
idpf_is_cap_ena(). I'd rather follow that for now.
I will send v5 with the changes shortly.
Thanks for the review.
Ahmed
