Re: [ovs-dev] [PATCH v3 2/3] OVN ACL: Allow ct_mark and ct_label values to be set from register as well

2019-04-17 Thread Ben Pfaff
On Tue, Apr 16, 2019 at 11:58:08PM +, Ankur Sharma wrote:
> OVN allows only an integer (or masked integer) to be assigned to
> ct_mark and ct_label.
> 
> This patch, enhances the parser code to allow ct_mark and ct_label
> to be assigned from 32 bit registers (MFF_REG0 - MFF_REG15) and  128
> bit registers (MFF_XXREG0 - MFF_XXREG3) respectively.
> 
> Signed-off-by: Ankur Sharma 

Thanks for the patch!

When we add new errors to the ovn action parser, we add tests to provoke
each of them.  I don't see any new test for some of the errors here,
such as the "invalid token type" error.

We try to make the OVN action parser error messages grammatically
correct English sentences or phrases.  For example:
Syntax error at `reg1' expecting integer
The new message
Syntax error at `xxreg1' input: xxreg1,  not a 32 bit register.
is not in this category, and it unnecessarily repeats the xxreg1 part.
Something like
Syntax error at `xxreg1' expecting a 32-bit register.
would be better.

This patch seems to add unnecessary restrictions.  Why restrict the
source fields to registers, for example?  It's also a bit of an
unnecessary restriction to force the entire ct_mark or ct_label field to
be set.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v3 2/3] OVN ACL: Allow ct_mark and ct_label values to be set from register as well

2019-04-16 Thread Ankur Sharma
OVN allows only an integer (or masked integer) to be assigned to
ct_mark and ct_label.

This patch, enhances the parser code to allow ct_mark and ct_label
to be assigned from 32 bit registers (MFF_REG0 - MFF_REG15) and  128
bit registers (MFF_XXREG0 - MFF_XXREG3) respectively.

Signed-off-by: Ankur Sharma 
---
 include/ovn/actions.h |  3 ++
 ovn/lib/actions.c | 77 +--
 ovn/ovn-sb.xml| 20 +++--
 tests/ovn.at  | 16 +++
 4 files changed, 99 insertions(+), 17 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 1c0c67c..58b96a1 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -24,6 +24,7 @@
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/uuid.h"
+#include "openvswitch/meta-flow.h"
 #include "util.h"
 
 struct expr;
@@ -196,8 +197,10 @@ struct ovnact_ct_next {
 /* OVNACT_CT_COMMIT. */
 struct ovnact_ct_commit {
 struct ovnact ovnact;
+bool is_ct_mark_reg, is_ct_label_reg; /* If the value is from a register */
 uint32_t ct_mark, ct_mark_mask;
 ovs_be128 ct_label, ct_label_mask;
+enum mf_field_id ct_mark_reg, ct_label_reg;
 };
 
 /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index eb7e5ba..d8b86dc 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -627,8 +627,28 @@ parse_ct_commit_arg(struct action_context *ctx,
 } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
 cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
 cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
+} else if (ctx->lexer->token.type == LEX_T_ID) {
+
+   cc->ct_mark_mask = UINT32_MAX;
+
+   const struct mf_field *mf = mf_from_name(ctx->lexer->token.s);
+   if (mf) {
+
+  if (mf->id >= MFF_REG0 && mf->id <= MFF_REG15) {
+ cc->is_ct_mark_reg = true;
+ cc->ct_mark_reg = mf->id;
+  } else {
+ lexer_syntax_error(ctx->lexer, "input: %s,  not a 32 bit "
+"register", mf->name);
+ return;
+  }
+   } else {
+  lexer_syntax_error(ctx->lexer, "invalid field name: %s",
+ ctx->lexer->token.s);
+  return;
+   }
 } else {
-lexer_syntax_error(ctx->lexer, "expecting integer");
+lexer_syntax_error(ctx->lexer, "invalid token type");
 return;
 }
 lexer_get(ctx->lexer);
@@ -642,9 +662,28 @@ parse_ct_commit_arg(struct action_context *ctx,
 } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
 cc->ct_label = ctx->lexer->token.value.be128_int;
 cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
+} else if (ctx->lexer->token.type == LEX_T_ID) {
+
+   cc->ct_label_mask = OVS_BE128_MAX;
+   const struct mf_field *mf = mf_from_name(ctx->lexer->token.s);
+   if (mf) {
+  if (mf->id >= MFF_XXREG0 && mf->id <= MFF_XXREG3) {
+ cc->is_ct_label_reg = true;
+ cc->ct_label_reg = mf->id;
+  } else {
+ lexer_syntax_error(ctx->lexer, "input: %s,  not a 128 bit "
+"register", mf->name);
+ return;
+  }
+   } else {
+  lexer_syntax_error(ctx->lexer, "invalid field name: %s",
+ ctx->lexer->token.s);
+  return;
+   }
+
 } else {
-lexer_syntax_error(ctx->lexer, "expecting integer");
-return;
+   lexer_syntax_error(ctx->lexer, "invalid token type");
+   return;
 }
 lexer_get(ctx->lexer);
 } else {
@@ -713,14 +752,36 @@ encode_CT_COMMIT(const struct ovnact_ct_commit *cc,
 ofpbuf_pull(ofpacts, set_field_offset);
 
 if (cc->ct_mark_mask) {
-const ovs_be32 value = htonl(cc->ct_mark);
-const ovs_be32 mask = htonl(cc->ct_mark_mask);
-ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);
+   if (cc->is_ct_mark_reg) {
+  struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
+
+  move->src.field = mf_from_id(cc->ct_mark_reg);
+  move->src.ofs = 0;
+  move->src.n_bits = 32;
+  move->dst.field = mf_from_id(MFF_CT_MARK);
+  move->dst.ofs = 0;
+  move->dst.n_bits = 32;
+   } else {
+  const ovs_be32 value = htonl(cc->ct_mark);
+  const ovs_be32 mask = htonl(cc->ct_mark_mask);
+  ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, 
&mask);
+   }
 }
 
 if (!ovs_be128_is_zero(cc->ct_label_mask)) {
-ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,
- &cc->ct_l