On Mon, Apr 14, 2025 at 10:52 AM Dumitru Ceara <dce...@redhat.com> wrote:
> The new action stores the 8 relevant bits of the OVS ct_state field into
> a register. This is implemented through an additional OpenFlow table,
> OFTABLE_CT_STATE_SAVE, that contains two (static) rules:
> - high-priority, set the target register to 0 if ct_state -trk is set
> - low-priority, copy the 8 bits ct_state to the target register
> otherwise
>
> From a logical flow perspective, the action is supposed to be used in
> the following way:
> R = ct_state_save();
>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> V2:
> - Addressed Ales' comment:
> - properly set the saved ct state register if ct_state includes +trk
> - Fixed up output of ovn-trace for ct_state_save() to include the actual
> value that gets copied to the destination register.
> ---
> controller/lflow.c | 1 +
> controller/lflow.h | 1 +
> controller/physical.c | 23 +++++++++++++++++++++++
> include/ovn/actions.h | 3 +++
> include/ovn/logical-fields.h | 2 ++
> lib/actions.c | 27 +++++++++++++++++++++++++++
> lib/ovn-util.c | 2 +-
> ovn-sb.xml | 8 ++++++++
> tests/ovn-macros.at | 1 +
> tests/ovn.at | 28 ++++++++++++++++++++++++++--
> tests/test-ovn.c | 1 +
> utilities/ovn-trace.c | 21 +++++++++++++++++++++
> 12 files changed, 115 insertions(+), 3 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 196f6a21ac..c24b2e2127 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -893,6 +893,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> .ct_ip6_dst_load_table = OFTABLE_CT_ORIG_IP6_DST_LOAD,
> .ct_tp_dst_load_table = OFTABLE_CT_ORIG_TP_DST_LOAD,
> .flood_remote_table = OFTABLE_FLOOD_REMOTE_CHASSIS,
> + .ct_state_save_table = OFTABLE_CT_STATE_SAVE,
> .ctrl_meter_id = ctrl_meter_id,
> .common_nat_ct_zone = get_common_nat_zone(ldp),
> };
> diff --git a/controller/lflow.h b/controller/lflow.h
> index fa99173e67..3fd7ca99f5 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -99,6 +99,7 @@ struct uuid;
> #define OFTABLE_CT_ORIG_IP6_DST_LOAD 82
> #define OFTABLE_CT_ORIG_TP_DST_LOAD 83
> #define OFTABLE_FLOOD_REMOTE_CHASSIS 84
> +#define OFTABLE_CT_STATE_SAVE 85
>
> /* Common defines shared between some controller components. */
> #define CHASSIS_FLOOD_INDEX_START 0x8000
> diff --git a/controller/physical.c b/controller/physical.c
> index 9e797b3b3c..b4f91b134d 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -3085,6 +3085,29 @@ physical_run(struct physical_ctx *p_ctx,
> ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_IP6_DST_LOAD, 100, 0,
> &match,
> &ofpacts, hc_uuid);
>
> + /* Implement the ct_state_save() logical action. */
> +
> + /* Add the flow:
> + * match = (1), action = (reg4[0..7] = ct_state[0..7])
> + * table = 85, priority = UINT16_MAX - 1
> + */
> + match_init_catchall(&match);
> + ofpbuf_clear(&ofpacts);
> + put_move(MFF_CT_STATE, 0, MFF_LOG_CT_SAVED_STATE, 0, 8, &ofpacts);
> + ofctrl_add_flow(flow_table, OFTABLE_CT_STATE_SAVE, UINT16_MAX - 1, 0,
> + &match, &ofpacts, hc_uuid);
> +
> + /* Add the flow:
> + * match = (!ct.trk), action = (reg4[0..7] = 0)
> + * table = 85, priority = UINT16_MAX
> + */
> + match_init_catchall(&match);
> + ofpbuf_clear(&ofpacts);
> + match_set_ct_state_masked(&match, 0, OVS_CS_F_TRACKED);
> + put_load(0, MFF_LOG_CT_SAVED_STATE, 0, 8, &ofpacts);
> + ofctrl_add_flow(flow_table, OFTABLE_CT_STATE_SAVE, UINT16_MAX, 0,
> + &match, &ofpacts, hc_uuid);
> +
> physical_eval_remote_chassis_flows(p_ctx, &ofpacts, flow_table);
>
> ofpbuf_uninit(&ofpacts);
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 0993ad0e5d..7fd7b26bfe 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -135,6 +135,7 @@ struct collector_set_ids;
> OVNACT(CT_ORIG_IP6_DST, ovnact_result) \
> OVNACT(CT_ORIG_TP_DST, ovnact_result) \
> OVNACT(FLOOD_REMOTE, ovnact_null) \
> + OVNACT(CT_STATE_SAVE, ovnact_result)
>
> /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
> enum OVS_PACKED_ENUM ovnact_type {
> @@ -964,6 +965,8 @@ struct ovnact_encode_params {
> * to resubmit. */
> uint32_t flood_remote_table; /* OpenFlow table for 'chassis_flood'
> * to resubmit. */
> + uint32_t ct_state_save_table; /* OpenFlow table for saving ct_state to
> + * register. */
> };
>
> void ovnacts_encode(const struct ovnact[], size_t ovnacts_len,
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index 7c6be6ea42..17154d38bd 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -63,6 +63,8 @@ enum ovn_controller_event {
> #define MFF_LOG_CT_ORIG_IP6_DST_ADDR MFF_XXREG1 /*
> REG_ORIG_DIP_IPV6 */
> #define MFF_LOG_CT_ORIG_TP_DST_PORT MFF_REG2 /*
> REG_ORIG_TP_DPORT
> * (bits 0..15). */
> +#define MFF_LOG_CT_SAVED_STATE MFF_REG4 /* REG_CT_STATE
> + * (bits 0..8). */
>
> /* Logical registers that are needed for backwards
> * compatibility with older northd versions.
> diff --git a/lib/actions.c b/lib/actions.c
> index b81061f5c1..f6f413be27 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -5552,6 +5552,29 @@ encode_FLOOD_REMOTE(const struct ovnact_null *null
> OVS_UNUSED,
> emit_resubmit(ofpacts, ep->flood_remote_table);
> }
>
> +static void
> +encode_CT_STATE_SAVE(const struct ovnact_result *res,
> + const struct ovnact_encode_params *ep OVS_UNUSED,
> + struct ofpbuf *ofpacts)
> +{
> + encode_result_action___(res, ep->ct_state_save_table,
> + MFF_LOG_CT_SAVED_STATE, 0, 8, ofpacts);
> +}
> +
> +static void
> +parse_CT_STATE_SAVE(struct action_context *ctx, const struct expr_field
> *dst,
> + struct ovnact_result *res)
> +{
> + parse_ovnact_result__(ctx, "ct_state_save", NULL, dst, res, 8);
> +}
> +
> +static void
> +format_CT_STATE_SAVE(const struct ovnact_result *res, struct ds *s)
> +{
> + expr_field_format(&res->dst, s);
> + ds_put_cstr(s, " = ct_state_save();");
> +}
> +
> /* Parses an assignment or exchange or put_dhcp_opts action. */
> static void
> parse_set_action(struct action_context *ctx)
> @@ -5658,6 +5681,10 @@ parse_set_action(struct action_context *ctx)
> lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
> parse_CT_ORIG_TP_DST(ctx, &lhs,
> ovnact_put_CT_ORIG_TP_DST(ctx->ovnacts));
> + } else if (!strcmp(ctx->lexer->token.s, "ct_state_save") &&
> + lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
> + parse_CT_STATE_SAVE(ctx, &lhs,
> + ovnact_put_CT_STATE_SAVE(ctx->ovnacts));
> } else {
> parse_assignment_action(ctx, false, &lhs);
> }
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index f406114db2..c825e0936f 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -898,7 +898,7 @@ ip_address_and_port_from_lb_key(const char *key, char
> **ip_address,
> *
> * This value is also used to handle some backward compatibility during
> * upgrading. It should never decrease or rewind. */
> -#define OVN_INTERNAL_MINOR_VER 7
> +#define OVN_INTERNAL_MINOR_VER 8
>
> /* Returns the OVN version. The caller must free the returned value. */
> char *
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 4bcf44d041..9777cc64ad 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2792,6 +2792,14 @@ tcp.flags = RST;
> </p>
> </dd>
>
> + <dt><code><var>R</var> = ct_state_save();</code></dt>
> + <dd>
> + <p>
> + This action checks if the packet is tracked and stores the
> + conntrack original state in the 8-bit register <var>R</var>.
> + </p>
> + </dd>
> +
> <dt><code>sample(probability=<var>packets</var>, ...)</code></dt>
> <dd>
> <p>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index fd1a6b197a..0c1c1cd992 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -1494,5 +1494,6 @@ m4_define([OFTABLE_CT_ORIG_NW_DST_LOAD], [81])
> m4_define([OFTABLE_CT_ORIG_IP6_DST_LOAD], [82])
> m4_define([OFTABLE_CT_ORIG_TP_DST_LOAD], [83])
> m4_define([OFTABLE_FLOOD_REMOTE_CHASSIS], [84])
> +m4_define([OFTABLE_CT_STATE_SAVE], [85])
>
> m4_define([OFTABLE_SAVE_INPORT_HEX], [m4_eval(OFTABLE_SAVE_INPORT, 16)])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bbaa653a82..938a0d0f0d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -2287,6 +2287,28 @@ flood_remote;
> flood_remote();
> Syntax error at `(' expecting `;'.
>
> +# ct_state_save()
> +reg1[[0..7]] = ct_state_save();
> + encodes as
> set_field:0/0xff->reg4,resubmit(,OFTABLE_CT_STATE_SAVE),move:NXM_NX_REG4[[0..7]]->NXM_NX_XXREG0[[64..71]]
> +
> +reg1[[2..9]] = ct_state_save();
> + encodes as
> set_field:0/0xff->reg4,resubmit(,OFTABLE_CT_STATE_SAVE),move:NXM_NX_REG4[[0..7]]->NXM_NX_XXREG0[[66..73]]
> +
> +reg1[[24..31]] = ct_state_save();
> + encodes as
> set_field:0/0xff->reg4,resubmit(,OFTABLE_CT_STATE_SAVE),move:NXM_NX_REG4[[0..7]]->NXM_NX_XXREG0[[88..95]]
> +
> +xreg1 = ct_state_save();
> + Cannot use 64-bit field xreg1[[0..63]] where 8-bit field is required.
> +
> +reg1[[0..30]] = ct_state_save();
> + Cannot use 31-bit field reg1[[0..30]] where 8-bit field is required.
> +
> +ct_state_save;
> + Syntax error at `ct_state_save' expecting action.
> +
> +ct_state_save();
> + Syntax error at `ct_state_save' expecting action.
> +
> # Miscellaneous negative tests.
> ;
> Syntax error at `;'.
> @@ -37003,8 +37025,10 @@ check_default_flows() {
> # Tables 81, 82 and 83 are part of ct_nw_dst(), ct_ip6_dst() and
> ct_tp_dst()
> # actions respectively and its OK for them to not have default
> flows.
> # Table 84 is part of flood_remote; action and its OK for
> - # it to not have default flows.
> - if test ${table} -eq 68 -o ${table} -eq 70 -o ${table} -eq 81 -o
> ${table} -eq 82 -o ${table} -eq 83 -o ${table} -eq 84; then
> + # it to not have default flows.
> + # Table 85 is the implementation of the ct_state_save action and
> + # doesn't require a default flow.
> + if test ${table} -eq 68 -o ${table} -eq 70 -o ${table} -eq 81 -o
> ${table} -eq 82 -o ${table} -eq 83 -o ${table} -eq 84 -o ${table} -eq 85;
> then
> continue;
> fi
> AT_CHECK([grep -qe "table=$table.*
> priority=0\(,metadata=0x\w*\)\? actions" oflows], [0], [ignore], [ignore],
> [echo "Table $table does not contain a default action"])
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index de0cb0b444..c4eb2a7dfb 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1388,6 +1388,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> .ct_ip6_dst_load_table = OFTABLE_CT_ORIG_IP6_DST_LOAD,
> .ct_tp_dst_load_table = OFTABLE_CT_ORIG_TP_DST_LOAD,
> .flood_remote_table = OFTABLE_FLOOD_REMOTE_CHASSIS,
> + .ct_state_save_table = OFTABLE_CT_STATE_SAVE,
> .lflow_uuid.parts =
> { 0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc, 0xdddddddd},
> .dp_key = 0xabcdef,
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index b8a9e0f0a6..ce55008a60 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -3121,6 +3121,23 @@ execute_check_out_port_sec(const struct
> ovnact_result *dl,
> mf_write_subfield_flow(&sf, &sv, uflow);
> }
>
> +static void
> +execute_ct_save_state(const struct ovnact_result *dl, struct flow *uflow,
> + struct ovs_list *super)
> +{
> + struct mf_subfield sf = expr_resolve_field(&dl->dst);
> + union mf_subvalue sv = {
> + .u8_val = uflow->ct_state & CS_TRACKED ? uflow->ct_state : 0,
> + };
> + mf_write_subfield_flow(&sf, &sv, uflow);
> +
> + struct ds s = DS_EMPTY_INITIALIZER;
> + expr_field_format(&dl->dst, &s);
> + ovntrace_node_append(super, OVNTRACE_NODE_ACTION, "/* %s = %"PRIu8";
> */",
> + ds_cstr(&s), sv.u8_val);
> + ds_destroy(&s);
> +}
> +
> static void
> trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> const struct ovntrace_datapath *dp, struct flow *uflow,
> @@ -3464,6 +3481,10 @@ trace_actions(const struct ovnact *ovnacts, size_t
> ovnacts_len,
> case OVNACT_FLOOD_REMOTE:
> ovntrace_node_append(super, OVNTRACE_NODE_OUTPUT,
> "/* Flood to all remote chassis */");
> + break;
> + case OVNACT_CT_STATE_SAVE:
> + execute_ct_save_state(ovnact_get_CT_STATE_SAVE(a), uflow,
> super);
> + break;
> }
> }
> ofpbuf_uninit(&stack);
> --
> 2.49.0
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <amu...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
