diff mbox series

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

Message ID 1547165793-14659-3-git-send-email-ankur.sharma@nutanix.com
State Changes Requested
Headers show
Series Associate identifier with OVN ACL connection tracking entry | expand

Commit Message

Ankur Sharma Jan. 11, 2019, 12:16 a.m. UTC 
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 <ankur.sharma@nutanix.com>
---
 include/ovn/actions.h |  3 +++
 ovn/lib/actions.c     | 73 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 70 insertions(+), 6 deletions(-)

Comments

Ben Pfaff Feb. 5, 2019, 9:30 p.m. UTC | #1 
On Fri, Jan 11, 2019 at 12:16:38AM +0000, 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 <ankur.sharma@nutanix.com>

Thanks for the patch!

Please capitalize "Signed-off-by" <-- just like that.

When we add new support for OVN actions, we also add tests to the
"ovn -- action parsing" test in ovn.at.  Please add a test for a
correctly formed action as well as at least one negative form that
exercises each of the new error messages.

Please also document the new form of the action in ovn-sb.xml.

The error messages don't seem all that user friendly.  Make sure that
they clearly point out the problem.  (That's probably easier when you
add the tests.)

The new forms break the level of abstraction that OVN fields are
supposed to provide, that is, please use OVN field names rather than
OpenFlow ones.

I don't know why only registers are allowed.  I don't believe that such
a restriction exists at the OpenFlow level.  I also don't know why only
full, aligned registers are allowed.  I don't think that restriction
exists at the OpenFlow level either.

It looks like the indentation is wrong here in encode_CT_COMMIT().

Thanks,

Ben.
Ankur Sharma Feb. 6, 2019, 9:36 p.m. UTC | #2 
Hi Ben,

Thanks for the review.
a. My bad on the Signed off by, will take care of it in V2.
b. Sure, I will add the test case as well.
c. I will update the ovn-sb.xml as well.
d. Yes, openflow does not restrict the usage of registers for connection tracker, I think probably because of lack of use case thus far, it was missing in OVN.


Thanks again for review, V2 will have all the comments addressed.

Regards,
Ankur

-----Original Message-----
From: Ben Pfaff <blp@ovn.org> 
Sent: Tuesday, February 5, 2019 1:30 PM
To: Ankur Sharma <ankur.sharma@nutanix.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 2/3] OVN ACL: Allow ct_mark and ct_label values to be set from register as well

On Fri, Jan 11, 2019 at 12:16:38AM +0000, 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 <ankur.sharma@nutanix.com>

Thanks for the patch!

Please capitalize "Signed-off-by" <-- just like that.

When we add new support for OVN actions, we also add tests to the "ovn -- action parsing" test in ovn.at.  Please add a test for a correctly formed action as well as at least one negative form that exercises each of the new error messages.

Please also document the new form of the action in ovn-sb.xml.

The error messages don't seem all that user friendly.  Make sure that they clearly point out the problem.  (That's probably easier when you add the tests.)

The new forms break the level of abstraction that OVN fields are supposed to provide, that is, please use OVN field names rather than OpenFlow ones.

I don't know why only registers are allowed.  I don't believe that such a restriction exists at the OpenFlow level.  I also don't know why only full, aligned registers are allowed.  I don't think that restriction exists at the OpenFlow level either.

It looks like the indentation is wrong here in encode_CT_COMMIT().

Thanks,

Ben.
diff mbox series

Patch

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 7b7a894..c0c8da7 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)) {
+       if (cc->is_ct_label_reg) {
+          struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
+
+          move->src.field = mf_from_id(cc->ct_label_reg);
+          move->src.ofs = 0;
+          move->src.n_bits = 128;
+          move->dst.field = mf_from_id(MFF_CT_LABEL);
+          move->dst.ofs = 0;
+          move->dst.n_bits = 128;
+       } else {
         ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,
                              &cc->ct_label_mask);
+       }
     }
 
     ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);