diff mbox series

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

Message ID 1555459154-103091-3-git-send-email-ankur.sharma@nutanix.com
State Superseded
Headers show
Series Associate identifier with OVN ACL connection tracking entry | expand

Commit Message

Ankur Sharma April 16, 2019, 11:58 p.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     | 77 +++++++++++++++++++++++++++++++++++++++++++++------
 ovn/ovn-sb.xml        | 20 +++++++------
 tests/ovn.at          | 16 +++++++++++
 4 files changed, 99 insertions(+), 17 deletions(-)

Comments

0-day Robot April 17, 2019, 2:17 a.m. UTC | #1
Bleep bloop.  Greetings Ankur Sharma, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#128 FILE: ovn/lib/actions.c:767:
          ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);

WARNING: Line is 80 characters long (recommended limit is 79)
#145 FILE: ovn/lib/actions.c:782:
          ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,

WARNING: Line is 94 characters long (recommended limit is 79)
#162 FILE: ovn/ovn-sb.xml:1183:
        <dt><code>ct_commit(ct_mark=(<var>value[/mask]</var> OR <var>regX</var>));</code></dt>

WARNING: Line is 97 characters long (recommended limit is 79)
#163 FILE: ovn/ovn-sb.xml:1184:
        <dt><code>ct_commit(ct_label=(<var>value[/mask]</var> OR <var>xxregX</var>));</code></dt>

WARNING: Line is 151 characters long (recommended limit is 79)
#164 FILE: ovn/ovn-sb.xml:1185:
        <dt><code>ct_commit(ct_mark=(<var>value[/mask]</var> OR <var>regX</var>), ct_label=(<var>value[/mask]</var> OR <var>xxregX</var>));</code></dt>

WARNING: Line is 84 characters long (recommended limit is 79)
#172 FILE: ovn/ovn-sb.xml:1190:
            <code>ct_mark=<var>value[/mask]</var> OR <var>xxregX</var></code> and/or

WARNING: Line is 92 characters long (recommended limit is 79)
#173 FILE: ovn/ovn-sb.xml:1191:
            <code>ct_label=<var>value[/mask]</var> OR <var>xxregX</var></code> are supplied,

WARNING: Line is 83 characters long (recommended limit is 79)
#178 FILE: ovn/ovn-sb.xml:1193:
            values indicated by <var>value[/mask]</var> or 32 bit/128 bit registers

WARNING: Line is 84 characters long (recommended limit is 79)
#179 FILE: ovn/ovn-sb.xml:1194:
            on the connection tracking entry. <code>ct_mark</code> is a 32-bit field

WARNING: Line is 80 characters long (recommended limit is 79)
#180 FILE: ovn/ovn-sb.xml:1195:
            and hence will read value only from a 32 bit register (reg0 - reg9).

WARNING: Line is 83 characters long (recommended limit is 79)
#181 FILE: ovn/ovn-sb.xml:1196:
            <code>ct_label</code> is a 128-bit field and hence will read value only

WARNING: Line is 82 characters long (recommended limit is 79)
#182 FILE: ovn/ovn-sb.xml:1197:
            from a 128 bit register (xxreg0 - xxreg1). The <var>value[/mask]</var>

Lines checked: 215, Warnings: 12, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Ben Pfaff April 17, 2019, 5:44 p.m. UTC | #2
On Tue, Apr 16, 2019 at 11:58:08PM +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!

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.
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 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_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);
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 5c4a852..35719c1 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1180,19 +1180,21 @@ 
         </dd>
 
         <dt><code>ct_commit;</code></dt>
-        <dt><code>ct_commit(ct_mark=<var>value[/mask]</var>);</code></dt>
-        <dt><code>ct_commit(ct_label=<var>value[/mask]</var>);</code></dt>
-        <dt><code>ct_commit(ct_mark=<var>value[/mask]</var>, ct_label=<var>value[/mask]</var>);</code></dt>
+        <dt><code>ct_commit(ct_mark=(<var>value[/mask]</var> OR <var>regX</var>));</code></dt>
+        <dt><code>ct_commit(ct_label=(<var>value[/mask]</var> OR <var>xxregX</var>));</code></dt>
+        <dt><code>ct_commit(ct_mark=(<var>value[/mask]</var> OR <var>regX</var>), ct_label=(<var>value[/mask]</var> OR <var>xxregX</var>));</code></dt>
         <dd>
           <p>
             Commit the flow to the connection tracking entry associated with it
-            by a previous call to <code>ct_next</code>.  When
-            <code>ct_mark=<var>value[/mask]</var></code> and/or
-            <code>ct_label=<var>value[/mask]</var></code> are supplied,
+            by a previous call to <code>ct_next</code>. When
+            <code>ct_mark=<var>value[/mask]</var> OR <var>xxregX</var></code> and/or
+            <code>ct_label=<var>value[/mask]</var> OR <var>xxregX</var></code> are supplied,
             <code>ct_mark</code> and/or <code>ct_label</code> will be set to the
-            values indicated by <var>value[/mask]</var> on the connection
-            tracking entry. <code>ct_mark</code> is a 32-bit field.
-            <code>ct_label</code> is a 128-bit field. The <var>value[/mask]</var>
+            values indicated by <var>value[/mask]</var> or 32 bit/128 bit registers
+            on the connection tracking entry. <code>ct_mark</code> is a 32-bit field
+            and hence will read value only from a 32 bit register (reg0 - reg9).
+            <code>ct_label</code> is a 128-bit field and hence will read value only
+            from a 128 bit register (xxreg0 - xxreg1). The <var>value[/mask]</var>
             should be specified in hex string if more than 64bits are to be used.
           </p>
 
diff --git a/tests/ovn.at b/tests/ovn.at
index b546e9a..f4e3650 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1021,6 +1021,22 @@  ct_commit(ct_label=18446744073709551615);
 ct_commit(ct_label=18446744073709551616);
     Decimal constants must be less than 2**64.
 
+ct_commit(ct_label=xxreg1);
+    formats as ct_commit(ct_label=0);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(move:NXM_NX_XXREG1[]->NXM_NX_CT_LABEL[]))
+    has prereqs ip
+
+ct_commit(ct_mark=reg1);
+    formats as ct_commit(ct_mark=0);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(move:NXM_NX_REG1[]->NXM_NX_CT_MARK[]))
+    has prereqs ip
+
+ct_commit(ct_label=reg1);
+    Syntax error at `reg1' input: reg1,  not a 128 bit register.
+
+ct_commit(ct_mark=xxreg1);
+    Syntax error at `xxreg1' input: xxreg1,  not a 32 bit register.
+
 # ct_dnat
 ct_dnat;
     encodes as ct(table=19,zone=NXM_NX_REG11[0..15],nat)